Skip to content

impl(web): add /series/<id> page for viewing a media series#78

Draft
andykais-claude wants to merge 3 commits into
mainfrom
cursor/rebase-series-view-page-caeb
Draft

impl(web): add /series/<id> page for viewing a media series#78
andykais-claude wants to merge 3 commits into
mainfrom
cursor/rebase-series-view-page-caeb

Conversation

@andykais-claude

@andykais-claude andykais-claude commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds a new /series/<series_id> page to @forager/web for viewing the contents of a media series, mirroring the /browse layout but driving results from forager.series.search. Originally a rebase of #68 onto main; now also incorporates the design-review feedback on the shared-component abstraction.

Design review resolution

Addresses the browsable shared-component review:

  • Naming — renamed the pattern from browse_like/BrowseLike to browsable/Browsable ($lib/components/browsable/, BrowsableController).
  • Hole 1 (duplicated query-param managers) — added BrowsableQueryParams (extends the existing BaseQueryParams) which factors out the duplicated parse_url/serialize/merge/onMount+popstate and shared query-building helpers (parse_tags, apply_common_filters, media-type mapping). Browse and series managers now implement only route-specific bits (DEFAULTS, URL_PARAM_MAP, execute_search, contextual_query). TagQueryParams still extends BaseQueryParams directly. The popstate listener is now cleaned up once, in the base.
  • Hole 2 (untyped contract) — deleted the any-typed BrowseLikeQueryParams interface; introduced a concrete BrowsableSearchParams interface (generic over the route's sort union) so the shared components' field bindings are compile-checked.
  • Hole 3 (browse-only params in shared SearchResults) — removed all group_by/mode references from the shared component; the grouped-tile link is now supplied by the browse route via a snippet.
  • Hole 5 (boolean route flags) — replaced show_series_index/show_series_link with a single tile_footer?: Snippet<[MediaViewRune]>; browse supplies a "View series"/grouped-link footer, series supplies a #index footer.
  • Hole 6 (unsafe series_index double-cast)series_index is now a typed field on MediaViewRune, populated from the typed forager.series.search result in MediaListRune rather than cast on read.
  • Hole 7 (duplicated page chrome) — extracted the shared shell (Header+SearchParams, MediaDetails/MediaView/MediaList, Footer, svelte:window) into BrowsableShell.svelte, rendered by thin browse/series +page.svelte markers.

Note on Hole 7 (shared shell vs. layout group)

I implemented the shell as a shared component rendered by each thin page rather than the literal (browsable)/+layout.svelte group. Sibling routes under a shared SvelteKit layout keep the same layout instance across client-side navigation, so a controller built in the layout would not rebuild when navigating /browse/series/<id> (breaking the "View series" link and the queryparams onMount). Rendering a shared shell from each page dedupes the chrome identically, keeps route-specific snippets fully typed against each concrete controller, preserves the routes → $lib dependency direction, and avoids the layout-persistence pitfall.

Testing

  • deno task --cwd packages/web build — succeeds.
  • deno task --cwd packages/cli ... gui boots with --check (deno's typechecker, which resolves @forager/core) passing.
  • deno run -A npm:svelte-check — 115 errors, down from 125 pre-refactor; all remaining are pre-existing module-resolution / action-typing errors. No new real errors.
  • Manual GUI test: /browse renders the series tile with the View series link (navigates to /series/4); /series/4 shows #0/#1/#2 labels; ?group_by=artist renders grouped tiles with working group-name links; media-type filter and media viewer work.
Open in Web Open in Cursor 

@andykais andykais changed the title impl(web): add /series/&lt;id&gt; page for viewing a media series impl(web): add /series/<id>; page for viewing a media series Jul 7, 2026
@andykais andykais changed the title impl(web): add /series/<id>; page for viewing a media series impl(web): add /series/<id> page for viewing a media series Jul 7, 2026
cursoragent and others added 2 commits July 8, 2026 03:11
Moves the media browsing UI components (MediaList, MediaDetails,
MediaView, Footer, SearchParams, SearchResults, SearchLink,
MediaDetailEntry) and their supporting runes (dimensions,
media_selections) out of the route-local browse folder into
$lib/components/browse_like and $lib/runes so they can be reused
by the upcoming /series/<series_id> route. Introduces a
BrowseLikeController base with a BrowseLikeQueryParams contract
that shared components rely on. Widens MediaListRune to accept a
new 'series_search' paginate type.

Co-authored-by: andykais-claude <andykais-claude@users.noreply.github.com>
Brings the /series/<id> route query params in line with the browse
route after main's media_type search filter (#71). Maps the
image/video/audio dropdown values to the canonical uppercase
query.media_type filter now supported by forager.series.search, in
addition to the existing 'animated' handling.

Co-authored-by: andykais-claude <andykais-claude@users.noreply.github.com>
@cursor cursor Bot force-pushed the cursor/rebase-series-view-page-caeb branch from a512790 to c7eecf1 Compare July 8, 2026 03:13
@cursor

cursor Bot commented Jul 15, 2026

Copy link
Copy Markdown

Design review: the browsable shared-component abstraction

Concrete, apply-ready fixes for the shared-component design. Problem statement + recommended fix per item.

Naming: rename the pattern from browse_like/BrowseLike to browsable/Browsable everywhere it still applies — folder $lib/components/browse_like/$lib/components/browsable/, BrowseLikeControllerBrowsableController, etc.

Out of scope here (handled separately): the "god object" controller prop (follow-up PR), the popstate/lifecycle leak (auto-resolved by Hole 1), throw vs error() param validation, and the legacy svelte:window on:keydown|capture syntax.


Hole 1 — Browsable query-param managers duplicate logic instead of reusing the existing base

Problem: routes/browse/runes/queryparams.svelte.ts and routes/series/[series_id]/runes/queryparams.svelte.ts both extends Rune and re-implement parse_url / serialize / write_url / submit / goto / onMount+popstate / merge. They are ~90% identical and already diverging (e.g. series has a bespoke #parse_tag; browse has extend/group-by; serialize return types disagree). Meanwhile $lib/runes/base_queryparams.svelte.ts (BaseQueryParams) already factors all of this out and is used by TagQueryParams.

Fix:

  • Add an intermediate abstract class BrowsableQueryParams<TParams extends BrowsableSearchParams> extends BaseQueryParams<TParams> in $lib/runes/, adding the members the shared browsable components need but tags doesn't: merge(), contextual_query (abstract), human_readable_summary, and the shared execute_search scaffolding.
  • Make QueryParamsManager (browse) and SeriesQueryParamsManager extend BrowsableQueryParams, implementing only route-specific bits (DEFAULTS, URL_PARAM_MAP, execute_search, contextual_query). TagQueryParams keeps extending BaseQueryParams directly.
  • This deletes ~250 duplicated lines; the popstate listener leak (no cleanup returned from onMount) and the onMount-in-constructor coupling get fixed once, in the base.

Hole 2 — the shared contract is untyped (any), defeating the components' type safety

Problem: BrowseLikeQueryParams in base_controller.ts is generic over Record<string, any> with contextual_query: any. But SearchParams.svelte binds concrete fields (draft.search_string, .sort, .order, .media_type, .unread_only, .stars, .stars_equality, .filepath) that the contract never guarantees — a missing/renamed field is a silent runtime undefined, not a compile error.

Fix:

  • Delete the BrowseLikeQueryParams interface. Define a concrete BrowsableSearchParams interface with the fields every browsable route shares (the ones listed above). Type the base controller's queryparams as BrowsableQueryParams<BrowsableSearchParams> (from Hole 1).
  • Browse's params extend it with search_mode/group_by; series' params extend it as-is (narrow sort per route). Remove all any.

Hole 3 — shared SearchResults leaks browse-only params

Problem: $lib/components/browsable/SearchResults.svelte reads queryparams.current.group_by and calls queryparams.merge({ mode: 'media', ... }). group_by/mode don't exist on the series schema; it only "works" on /series because a series search never returns grouped results (dead branch there).

Fix: Remove all group_by/mode references from the shared component. The grouped-tile link is browse-specific — provide it as a snippet supplied by the browse route (see Hole 5), so the shared component stays route-agnostic.

Hole 5 — boolean flag props encode "which route am I"

Problem: MediaList/SearchResults take show_series_index and show_series_link — two mutually-exclusive booleans that really mean "browse vs series." Dead branches now, combinatorial growth with each new browsable route.

Fix: Replace both with a single tile_footer?: Snippet<[MediaViewRune]> prop, rendered via {@render tile_footer?.(result)}. Browse passes a "View series" link snippet (and its grouped-tile link snippet, per Hole 3); series passes a #{index} snippet. SearchResults knows nothing about series. (Matches the Snippets docs: pass content, not flags.)

Hole 6 — series_index reached via unsafe double-cast

Problem: media_view_rune.svelte.ts does (this.media as unknown as { series_index?: number }).series_index, bypassing the type system for a field that exists only on series-search responses — against the repo's type-safe / error-loudly ethos.

Fix: Discriminate MediaListRune's Result union so series-search results carry series_index in the type, and expose it without casting. If core's return type omits it, thread it through the forager.series.search output type rather than casting.

Hole 7 — shared page chrome duplicated across both +page.svelte (use a layout)

Problem: routes/browse/+page.svelte and routes/series/[series_id]/+page.svelte duplicate ~90% of markup (outer wrapper, Header+SearchParams, grid-cols-[auto_1fr] with MediaDetails/MediaView/MediaList, Footer, svelte:window).

Fix — layout-group approach:

routes/(browsable)/
  +layout.svelte                     # shared shell
  browse/+page.svelte                # /browse (thin)
  series/[series_id]/+page.svelte    # /series/[id] (thin)
  • (browsable)/+layout.svelte reads data.config (already provided by the root +layout.server.ts, so it flows down to this group) plus page.route.id / page.params, builds the correct controller (BrowseController vs SeriesController(config, series_id)), and renders the entire shell — passing {controller} as a prop to the shared components (prop-passing is preserved; the context migration is the deferred god-object follow-up).
  • Key SvelteKit constraint: data/context flow downward (layout → page), never page → layout. So every route-specific piece must be resolved in the layout, not the page: the controller, the page title (fetch the series title here, guarded by route kind), the sort-option set, whether to show the browse-only search-mode/group-by filters, and the per-tile footer.
    • Model the data-like differences (sort options list, title, show-search-controls flag) as a typed BrowsableRouteConfig selected by page.route.id, kept in $lib/components/browsable/.
    • Define the markup differences (the tile_footer snippet, browse extra_filters, grouped-tile link) as snippets inside the layout, chosen by route kind.
  • The two +page.svelte files become thin route markers. URLs are unchanged — the (browsable) parens group is invisible in the path.

After the move, everything under the old $lib/components/browse_like/ lives in $lib/components/browsable/.

Applies the PR #78 design review feedback:

- Rename the browse_like/BrowseLike pattern to browsable/Browsable
  (/components/browsable, BrowsableController).
- Hole 1: add BrowsableQueryParams (extends BaseQueryParams) that factors
  out the duplicated URL parse/serialize/merge and shared query-building
  helpers; browse + series managers now only implement route-specific bits.
  The popstate listener is cleaned up once in BaseQueryParams.
- Hole 2: replace the any-typed BrowseLikeQueryParams contract with a
  concrete BrowsableSearchParams interface (generic over the sort union).
- Hole 3: remove group_by/mode references from the shared SearchResults.
- Hole 5: replace show_series_index/show_series_link boolean props with a
  single tile_footer snippet supplied per route.
- Hole 6: expose series_index via a typed field set from the series-search
  output type instead of an unsafe double-cast.
- Hole 7: extract the duplicated page chrome into a shared BrowsableShell
  component rendered by thin browse/series +page markers.

Also fixes a few incidental pre-existing type issues in touched files
(SelectInput options/onchange, MediaListRune constructor args).

Co-authored-by: andykais-claude <andykais-claude@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants