feat(client): add library page with five browse lenses#541
Conversation
Port the Nama library design into a new /library feature: a counted header with a stats spine, lens tabs, title search, and a faceted filter popover, over five lenses (A→Z, Timeline, Collections, Servers, Quality). Reuses the shared MediaRowCard and shadcn tokens; data is mocked behind the React Query layer pending the unified media API.
|
Cloudflare preview: https://app-pr-541.omid-ocean.workers.dev |
Fallow combined reportFound 25 findings. Details
Generated by fallow. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Removed `home_card_kind` and `library_kind` from home and library message files. - Introduced new `media_kind` definitions in `media/en.json` and `media/fa.json`. - Updated references in components and tests to use the new `media_kind` instead of the removed definitions. - Refactored library filter popover to use a `ToggleGroup` for facet selection instead of custom pills. - Created a reusable `LensPage` component to handle empty states and content rendering for library lenses. - Improved filtering logic by introducing `useFilteredLibraryItems` hook for better separation of concerns.
github-actions
Bot
added
status: ready-for-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dfbee8f8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review: feat(client) — library page with five browse lensesGood PR overall. Architecture is clean: flat feature folder, URL-owned filter state, Suspense-reads everywhere, Two blocking issues before merge; four non-blocking suggestions. 🚨 BLOCKING 1 — No library design docThe library feature is substantive new product surface: five lenses, URL-filter state, faceted popover, scroll-spy rail, fanned collection cards. Per Rule 13 and the review checklist, every subsystem change needs a linked The PR links epic #491 (an issue), and the only candidate doc — Required: add 🚨 BLOCKING 2 — Counts endpoint deleted, design doc not updated
The deletion also drops
Required: update §A2/§A6 in the design doc (in this PR) to remove the counts endpoint and explain the rationale. Or restore the endpoint and handle the watchlist header change separately. Non-blocking suggestions
Changeset note (non-blocking)Two Summary: fix the two blocking items (design doc for library feature, design doc sync for counts deletion) and this is ready. |
Code ReviewOverviewPR adds the Library page (five browse lenses: A→Z, Timeline, Collections, Servers, Quality) with faceted filters + URL-persisted state, plus removes the counts subsystem server-side (no-display-counts redesign) and unifies the watchlist/library tab chrome into a shared Large diff but tightly scoped. Filtering logic, grouping, URL round-trip, and schema coercion are all unit-tested. Design docs for watchlist (rev 10) and media pipeline consolidation are updated in the same commit. BLOCKING — Design doc for Library featurePR description links epic #491 but no design document. Per Rule 13 and review checklist item 6:
The Library page is a new subsystem: five distinct lenses, dedicated sub-routes, its own query-key factory, facet engine, search, and URL schema. That warrants a design doc. The counts-removal side is covered by the two updated docs, but the Library surface has no spec. Required before merge: create Code Quality
SecurityNo concerns. All user-supplied input (search term, filter params) stays client-side. Performance
Testing
Documentation
Fallow Baseline
Multiple ChangesetsFour changesets. Each covers a distinct logical change (library page, lens routes, tab unification, counts drop) per CLAUDE.md. No issue. SummarySolid implementation. Filtering, grouping, URL state, i18n, and the shared tab component are all well-executed. One hard blocker: add a Library design doc before merge. Three non-blocking notes posted as inline comments ( |
Group items without a year into a trailing "unknown" bucket instead of dropping them, so a yearless-only filter set renders rather than leaving the /library/timeline route blank. Addresses Codex review.
Make MediaRowCard/MediaCardLink href optional and skip the anchor when it is undefined, so a null detail href no longer renders a clickable href="#" that jumps to the page top. Library cards now pass undefined on a format mismatch. Addresses review.
…lter cast Initialize the A-Z rail's active letter to the first group so it highlights on load, and document the intentionally-inert collection card and the strict:false search cast. Addresses review nits.
- use-library-filters: re-validate search via schema safeParse instead of an unchecked cast, so out-of-route mounts degrade explicitly to EMPTY_FILTERS. - lens-tabs: document the search-carry axis list as a maintenance point. - az-lens: explain the scroll-spy rootMargin insets. - grouping: note QUALITY_TIERS should grow with new API tiers. - library-error-fallback: explain the fire-and-forget resetQueries. - tests: add a watched-axis filter integration test. Addresses review nits.
Code Review — PR #541: Library page + counts endpoint removalOverallSolid, well-scoped PR. The library feature is cleanly isolated in Design-doc sync ✅Linked doc: Fallow baseline ✅
Security ✅All URL-sourced facet values go through the Zod schema ( Testing ✅
Non-blocking suggestions (inline)See inline comments for specific line-level notes. All are suggestions, none are blockers. |
- decadeOf: guard with `year == null` so a literal year 0 isn't swept into the unknown bucket by the falsy check. - RouteTabs: require `aria-label` at the type level so each navigation landmark is named (both callers already pass it). - lens-tabs: expand the search-carry comment to note why the codec round-trip can't replace the manual axis list. Addresses third review pass.
Code ReviewOverviewPorts the Nama library prototype into a real ✅ Design-doc sync — PASS
✅ Fallow baseline — PASS
Code qualityStrong:
Observations (all non-blocking):
TestingUnit tests cover filtering (each axis independently + intersection), facet counting, grouping (A→Z, decade, quality, server), watched-state classification, URL round-trip, and single-value coercion. Security / PerformanceNo security issues. No unbounded allocations — Verdict: approve after addressing the |
…th a test - indexLetter/titleSort: drop a leading The/A/An so "The Amber Room" files under A and sorts by "Amber", matching Plex/Jellyfin/Emby letter nav. - add a regression test asserting the lens-tab search reducer carries every librarySearchSchema axis, so a new axis can't silently drop on lens switch. - az-lens: extract the scroll-spy top inset to a named constant. Addresses fourth review pass.
Code Review — PR #541: Library page with five browse lensesBlocking checks
OverviewSubstantial, well-scoped feature: ~2500 LOC adding a flat features/library/ folder with five URL-driven lenses (A-Z, Timeline, Collections, Servers, Quality), URL-serialised faceted filters, Suspense + error boundary shells, and parallel Farsi i18n. The counts endpoint removal is clean — docs, server code, client hook, query key, and route-loader prefetch all deleted together with no dangling references. What works well
Issues (all non-blocking)Four inline comments posted — summaries below. 1. Tailwind style inconsistency (library-layout.tsx:19, library-skeleton.tsx:7) 2. Collection count badge vs visible poster count (collections-lens.tsx:113) 3. localeCompare() without locale (grouping.ts:47, also line 125) 4. SCROLL_SPY_TOP_INSET = "-100px" vs top-24 (96 px) (az-lens.tsx:23) Performance note (informational, no action needed now)A-Z, Collections, Servers, and Quality lenses render all items without virtualisation. With the 30-item mock this is invisible, but real catalogs at 500+ titles will be noticeably slower for those lenses. Timeline reuses the existing ScrollRow virtualisation. Worth noting for the epic #491 follow-up when real data lands. Mock fetcher (informational)lib/fetchers.ts intentionally resolves SAMPLE_LIBRARY — clearly documented and the PR test plan checkbox is unchecked. Fine for this pass; the swap point is well-marked. Summary: four small non-blocking issues, no blocking problems. Design docs in sync, fallow baseline clean, changesets correct. Approve after the localeCompare locale fix (highest real-world risk given the Farsi locale shipping with the app); the rest can land as is or as a follow-up. |
…inset - grouping: pass "en" to localeCompare so the A–Z rail keeps English collation on the fa locale build. - layout/skeleton: use max-w-[100rem] to match the watchlist sibling form. - az-lens: align the scroll-spy top inset to the rail's top-24 (96px). - collections: note the count badge is the full curated size by design. Addresses fifth review pass.
github-actions
Bot
removed
the
status: ready-for-review
|
|
||
| /** The quality tiers and servers a facet can offer, derived from the item set. */ | ||
| export function qualitiesOf(item: LibraryItem): string[] { | ||
| return item.tags ?? []; |
There was a problem hiding this comment.
Non-blocking — semantic coupling to tags
qualitiesOf maps item.tags directly to the quality facet. types.ts documents the assumption (tags = quality tiers only), but once the real API ships, tags may carry non-quality metadata (content warnings, network, format flags). Polluted tags would surface as spurious quality pills in the filter popover.
When the fetchLibrary mock is swapped (#491), either:
- verify the API schema ensures
tagscontains only quality tiers, or - rename the field or filter by a known allowlist before passing to this function.
Flagging now so it's in scope for the API-swap PR rather than a silent regression.
| @@ -0,0 +1,12 @@ | |||
| import { SAMPLE_LIBRARY } from "../__fixtures__/library-items.fixture"; | |||
There was a problem hiding this comment.
Non-blocking — fixture ships in the production bundle
SAMPLE_LIBRARY (~488 lines) is imported at a production code path. The fixture won't be tree-shaken because the import lives in fetchers.ts, not under a test guard. When the real API swap happens, the import will be replaced so the fixture becomes unreachable — but until then it inflates the client bundle.
If bundle size is a concern before the swap, wrapping behind a process.env.NODE_ENV === 'development' guard (and a matching lazy import) would keep it out of production builds. Low priority for a mock-first pass; just make sure the API-swap PR removes this import entirely.
| // round-trip would cover this automatically, but TanStack's search | ||
| // reducer typing rejects the helper's return — so it's a manual list.) | ||
| search={(prev) => ({ | ||
| kinds: prev.kinds, |
There was a problem hiding this comment.
Non-blocking — manual sync risk
The comment names the hazard clearly: a new axis added to librarySearchSchema must be mirrored here manually or it silently drops on lens switch. The fix is also in the comment (a codec round-trip), but TanStack's types block it.
One enforcement option that doesn't fight the router typing: add a compile-time exhaustiveness check using satisfies:
search={(prev) => {
// Exhaustiveness: if librarySearchSchema grows a new axis, this
// object will fail to satisfy LibrarySearch and surface as a
// type error rather than a silent runtime drop.
const next: LibrarySearch = {
kinds: prev.kinds,
genres: prev.genres,
qualities: prev.qualities,
servers: prev.servers,
watched: prev.watched,
};
return next;
}}LibrarySearch already exists (search.ts exports it). Makes the coupling explicit to the type system at zero runtime cost. Up to you whether the extra line is worth it.
| // Intentionally inert for this pass: the card carries hover affordance but no | ||
| // click target yet. Collection drill-down (a pre-filtered `/library` view) is | ||
| // out of scope here and lands with the collections detail route — don't wire a | ||
| // half-finished handler onto the hover state. |
There was a problem hiding this comment.
Non-blocking — hover affordance on a non-interactive element
CollectionCard applies hover:-translate-y-1 hover:border-primary/40 hover:shadow-hero but has no onClick or href. Users (and AT) will see a <article> that visually reacts to pointer but does nothing on activation. Two options:
- Reduce the affordance until the drill-down route lands — keep the
from-card to-card/55gradient and shadow but drop the translate/border-color transition. The "it looks clickable" cue stays off until it IS clickable. - Add
cursor-defaultandaria-disabledto be explicit that the hover is cosmetic.
The comment explains the intent correctly; this is a UX/accessibility heads-up for the first real users hitting the page.
| key={group.key} | ||
| id={anchorId(group.key)} | ||
| data-letter={group.key} | ||
| className="scroll-mt-28" |
There was a problem hiding this comment.
Non-blocking — scroll-mt-28 (112 px) vs SCROLL_SPY_TOP_INSET (−96 px)
scroll-mt-28 = 7rem = 112 px scroll margin; SCROLL_SPY_TOP_INSET = 96 px observer top inset. The two values serve different purposes so they don't need to be identical, but the comment on SCROLL_SPY_TOP_INSET says it "matches the rail's top-24 (96px)" — top-24 is the sticky nav offset, and scroll-mt-28 should also match the nav height (so jumped-to sections clear the nav). If the nav is truly 96 px, scroll-mt-24 (96 px) would be the precise match. The 16 px extra in scroll-mt-28 is effectively padding above each jumped-to heading — fine aesthetically, but the two constants now describe three different heights (nav = 96px, scroll-margin = 112px, observer top = 96px). Consider making this intentional padding explicit in a comment, or aligning scroll-mt-24 with the nav token.
Code ReviewWhat PR doesPorts the Nama Library prototype into a real ✅ Fallow baseline — PASS
✅ Design-doc sync — PASSThree design docs updated in the same PR, all consistent with the code:
No public-surface extension without a matching doc edit. ✓ Code qualitySolid overall. The feature follows the established flat feature-folder layout, query-keys factory, Suspense read pattern, and URL-as-state for filters. A few things worth noting:
SecurityNothing concerning. URL search params go through Performance
TestingGood unit coverage for the logic-heavy parts: filtering (all axes + intersection), watched-state classification, facet counts, all four grouping strategies including the yearless-timeline edge case. Gaps (non-blocking):
Changesets
Inline commentsFive non-blocking comments posted on specific lines:
No blocking issues. Approve when the changeset version bump is confirmed. |
2 participants
Summary
Ports the Nama "Library" design prototype into a new
/libraryfeature page. The page renders a counted header (eyebrow + title), lens tabs, and a faceted filter popover, then groups the same filtered catalog through five lenses: A→Z (sticky letter rail), Timeline (decade strips), Collections (fanned poster stacks), Servers, and Quality. Data is mocked for now behind the React Query layer; the grouping, filtering, and facet counts are all real and survive the eventual swap to the unified media API.The page reuses the shared
MediaRowCardandMediaCard*primitives, links cards to the existing/media/$mediaType/$mediaIddetail route, and styles everything with shadcn design tokens — no hard-coded colors. The repo's.darktheme already matches the prototype palette, so the port maps cleanly onto existing tokens.While unifying the bucket/lens switcher onto the shared
RouteTabs, the watchlist bucket filter lost its per-bucket count pips (now plain navigable tabs); the backingGET /api/media/countsendpoint had no remaining consumer and was removed. Documented indocs/2026-05-30-media-resource-unification-design.md(§A2RISK-A2-counts) and.changeset/drop-counts-server.md.Linked issue
Relates to epic #491 (media resource unification).
Type of change
Scope
@ent-mcp/clientTest plan
vp checkpasses locally (format, lint, type-check — 0 errors across 1490 files)vp testpasses locally (350 files / 2569 tests; newlibrary-data.test.ts: 11/11)vp run dev, sign in, navigate to/library, switch lenses, toggle filter pills, click a card → detail page.New unit tests cover the filtering (each facet axis incl. the watched axis + intersection), watched-state classification, facet counts, and the grouping strategies (incl. the yearless-timeline bucket).
Screenshots / recordings
Mock data only; screenshots to follow after manual run.
Checklist
docs/, inline) where relevant — n/a (inline comments only).changeset/library-page.md,@ent-mcp/clientminor)Notes for reviewers
frontend-feature-architecture(rules cited inline): centralizedlib/fetchers.ts, query-keys factory (rule 4), Suspense read (rule 5), page-owned state (rule 8),m.*i18n with enum label functions (rule 9), barrel exports only the public surface (rule 10), tests in__tests__/(rule 11).lib/fetchers.ts) documents the API-swap point for Epic: PR #483 follow-up — media consolidation, frontend dedupe, paraglide variants #491.