[andykais-ai]

Summary

• implements type resolution for packages/web so deno task --cwd packages/web check (svelte-check) passes with 0 errors, 0 warnings
• implements a Typecheck step in Web CI so PRs get TypeScript coverage
• fixes the genuine type errors surfaced once @forager/core types resolve, rather than silencing them
• rebased onto main (svelte context refactor refactor(web): replace controller prop drilling with svelte context #80, read_lines fix fix(core): stop dropping the first chunk of ffmpeg output in read_lines #90, count-sort fixes fix(web): drop count sort from grouped result links #87/fix(web): drop count sort order from grouped result links #92, svelte 5.56)

Previous attempt: #51.

Approach

svelte-check runs tsc, which cannot resolve Deno-style imports (@forager/core, @std/*, jsr:) or the Deno global. With everything unresolved most types collapsed to any, so errors were masked. Fixing resolution first (expectedly) raised the count before it could go to zero: 139 masked → 242 resolving → 0.

• shared deps are declared once in the root deno.json and materialized into node_modules via jsrDepsInNodeModules, so packages/web/package.json only carries web-specific npm deps
• workspace path aliases live in svelte.config.js under kit.alias, so svelte-kit sync generates them into .svelte-kit/tsconfig.json. There is a single tsconfig.json with no hand-maintained paths or exclude (an exclude override would replace sveltekit's generated service-worker excludes, and the generated include never reaches ../core anyway)
• App.Locals (forager, config) is declared in app.d.ts

Notable behavior fixes (not just typing)

• grouped results: MediaGroupRune resolves media_reference from the group's representative media (group.media[], falling back to the list the rune fetches). It is optional on the base, so consumers must narrow, which turned the previously unguarded media_selections and MediaDetails call sites into compile errors. "Opening" a grouped tile (second click / Enter) is now a no-op instead of throwing from load_detailed_view
• sort=count: SearchParams.sort uses the group-by superset, plain media searches map 'count' back to the default, and the typed merge() keeps main's reset of sort/order when leaving group-by mode
• URL params: parse_url iterates known keys and validates each one, so ?sort=garbage falls back to the default instead of reaching core's zod parser as a raw string. merge() takes a Partial<SearchParams>
• settings: toggle('ui.sidebar.hide') threw because the read switch was missing that key; both switches are now exhaustive at compile time
• RPC surface: media/series/views are exposed as ts-rpc modules so InferSpec resolves (every client.forager.* chain was previously never)
• /files/media_file/[id]: a series id now returns 400 rather than a 500 from undefined.filepath
• reactivity: Tag.svelte's identifier and MediaDetailEntry's inline layout are now $derived (svelte 5.56 flagged them as capturing stale props); intentional one-time captures (page controllers, sidebar initial width) are marked with svelte-ignore

Vision Testing

Automated

• rm -rf node_modules && deno install && deno task --cwd packages/web check → 0 errors, 0 warnings
• deno task --cwd packages/web build → succeeds
• deno check on the core and cli entrypoints, deno task --cwd packages/core lint → pass
• cli tests → 2 passed
• core tests → 33 passed / 5 failed, identical to a clean origin/main checkout on the same VM (local ffmpeg 6.1 vs CI's 9.0: duration precision + animated webp)
• re-verified the read_lines TextDecoderStream cast, verbatimModuleSyntax: false and the DOM.AsyncIterable lib are each still required by removing them one at a time

Manual

Seeded a database with 5 media tagged by genre/animal and drove the GUI in Chrome via CDP:

• browse renders 5 tiles; selecting + opening a jpg shows the media view and the details sidebar, view count goes 1 → 2
• footer thumbnail slider resizes tiles (110 → 180px)
• "Apply to search" on a tag navigates to ?tags=animal:cat (2 results)
• ?group_by=genre&sort=count renders 3 group tiles with thumbnails; double-clicking a group neither throws nor opens the dialog
• the group link is ?tags=genre:cartoon (count sort dropped) and navigates to 3 results
• no page or console errors throughout

browse-smoke-after-rebase.mp4

Media view with details sidebar
Group by genre view

_{To show artifacts inline, enable in settings.}

  

[andykais]

where is this called without arguments?

[cursor]

It's not called with zero args, but it is called with undefined for the first arg. In MediaDetails.svelte, the submit handler does ...media_response.update(media_info, tags) where media_info starts as undefined, and the per-tag remove button calls ...update(undefined, {remove: [...]}). I made the params optional to match forager.media.update(id, media_info?, tags?, editing?) in core, which already treats both as optional. Happy to make them required and pass {} at the call sites instead if you'd prefer the rune signature to be stricter than core.

[andykais]

hmm flagging this one. I need to investigate why this was here. Not sure all the implications of removing this yet. It is a real svelte-check error though

[cursor]

This one is safe to remove. type is not a valid attribute on a <div> (it's only meaningful on <button>, <input>, <style>, <link>, <ol>, etc.), so the browser ignores it entirely — it has zero runtime/behavioral effect here. It was almost certainly a leftover from when this element used to be a <button>. svelte-check flags it because type isn't in HTMLAttributes<HTMLDivElement>. Removing it changes nothing at runtime.

[andykais]

this file has a code smell. It looks like youre leaning hard into MediaViewRune.create only accepting GroupSearchParams. Make that create function accept any input params for media file/series/grouped just like the fetch_params is on main. This module should not try to make one param group more important than the others, it should just pass through params for anything like it was doing before, but with type safety

[cursor]

Reworked. MediaViewRune.create now takes a neutral MediaSearchInput union (PaginatedSearch | PaginatedSearchGroupBy) rather than GroupSearchParams, and media_list_rune passes fetch_params straight through with no cast at the create call. No param group is treated as more important than the others — the file/series runes ignore the params, and only the grouped rune consumes them, narrowing to the group shape it needs ('group_by' in search_params) internally. #saved_params/fetch_params are typed as MediaSearchInput; the only remaining casts are the symmetric ones at the media.search(...) / media.group(...) calls, where the discriminant (params_type) tells us which concrete variant to hand to each API.

[andykais]

These methods should avoid these duck typing checks. In places where we cannot retrieve the expected data, those should be erroring on the child classes. If, for ease of type safety that means the base class just becomes abstract so these methods can be abstract, thats fine by me. I would suggest just making the base classes error though, and adding working implementations on the classes that support it

[cursor]

Agreed — reworked to drop the duck-typing. MediaViewRune is now abstract and its data getters/mutations (media_reference, tags, media_file, thumbnails, preview_thumbnail, update, star, add_view, load_detailed_view, img_fit_classes) throw by default. Each subclass overrides only what it supports:

• Files and series share a MediaReferenceRune base that implements media_reference/tags/thumbnails/preview_thumbnail/add_view.
• MediaFileRune adds media_file/update/star/load_detailed_view/img_fit_classes.
• MediaSeriesRune adds its own load_detailed_view/img_fit_classes.
• MediaGroupRune implements group_metadata/preview_thumbnail/img_fit_classes.

The base class is generic over the response type (MediaViewRune<M extends AnyMediaResponse>) so each subclass's this.media is the concrete response and the overrides are fully typed with no narrowing. Runtime behavior is unchanged (the same operations that used to throw still throw, just with clearer messages).

[andykais]

explain your reasoning here. This if check was here for the query param unread=false. I assume this is removed because that is not part of the queryparams logic anymore.

[cursor]

You're right that this was doing real work, so I reworked it rather than just deleting it. The original inner check (unread_only === 'true' || unread_only === true) existed because parse_url assigned the raw URL string straight to unread_only (via the generic params[params_key] = val branch), so at runtime it was actually a string ('true'/'false') even though SearchParams.unread_only is typed boolean. The outer if (params.unread_only) alone would treat the string 'false' as truthy, which is why the inner === 'true' guard was needed to distinguish unread=false.

svelte-check flagged unread_only === 'true' as a boolean-vs-string no-overlap error, exposing that type/runtime mismatch. Instead of casting, I fixed the root cause: parse_url now coerces the value (params.unread_only = val === 'true') alongside the other typed params (stars, media_type, …), so unread_only is a genuine boolean. execute_search then just does if (params.unread_only) query.unread = true, which preserves the unread=false behavior (it stays unset) without the dead string comparison.

[andykais]

I think this is an unused dependency. Verify that and then drop it.

[cursor]

Verified and dropped. ts-pattern was only referenced in this package.json — nothing under packages/web/src (or packages/core/src) imports it — so it was unused. I'd added it speculatively while wiring up type resolution and it turned out unnecessary. Removed it and re-ran deno install; the lockfile is updated and deno task check + deno task build still pass.

(For the record on the neighbors: zod is used by config.ts and the +server.ts endpoints, @opentelemetry/api by hooks.server.ts, and the @std/* packages both directly and via the @forager/core type graph — so those are kept.)

[andykais]

these dependencies are duplicated from the deno.json. Why is that? I would prefer to declare the dependencies once rather than force me to keep two references of the versions in lockstep. Read recent deno blog posts to see if there is a way to just declare dependencies once.

[cursor]

Good call — fixed so the versions are declared once. Done in d72e0c4.

Why they were duplicated: svelte-check runs tsc, which resolves via node_modules. Deno stores jsr: deps under node_modules/.deno/@jsr+std__* but does not create the top-level node_modules/@std/* symlinks that node/tsc resolution needs, so declaring them only in deno.json left svelte-check unable to find @std/yaml, etc. (I confirmed this: dropping them from package.json with no other change gives 36 Cannot find module '@std/…' errors.) So the package.json copies existed purely to materialize those packages into node_modules for the type check.

The fix (from the deno.json docs → "JSR dependencies in node_modules"): set "jsrDepsInNodeModules": true in the root deno.json. This installs the jsr: deps through JSR's npm-compat registry (jsr:@std/yaml → npm:@jsr/std__yaml) into node_modules alongside the npm packages. Now the @std/*, zod and @torm/sqlite versions live only in the root deno.json imports, and I removed all of them from packages/web/package.json (which now only declares the genuinely web-specific npm deps @andykais/ts-rpc and @opentelemetry/api).

One wrinkle worth flagging: with the flag on, deno install couldn't resolve core's subpath import @std/fmt/bytes (it rewrites the @std/fmt/ prefix to the npm form and fails to URL-parse the bytes subpath). I worked around it with an explicit "@std/fmt/bytes": "jsr:@std/fmt@0.225.6/bytes" import-map entry next to @std/fmt — same version, so still a single source of truth.

Verified with a clean rm -rf node_modules && deno install && deno task --cwd packages/web check (0 errors), plus deno task --cwd packages/web build, deno check on the core/cli entrypoints, and the core test suite (only the pre-existing float/ffmpeg-precision failures remain).

[cursor]

Blocking — this throws at runtime and breaks the whole group-by view.

MediaGroupResponse has no media_reference field, so on main MediaGroupRune.media_reference evaluated to undefined. Two call sites rely on that:

• SearchResults.svelte, in the thumbnail alt text: alt="Thumbnail for {result.media_reference?.title ?? 'media'}". ?. protects against a undefined value, not against a getter that throws, so this now blows up while rendering every grouped tile.
• MediaSelectionsRune.set_current_selection → is_currently_selected(media_response.media_reference.id), which is what runs when you click a group tile.

Reproduced against this branch (deno task --cwd packages/web build:local + forager gui, then /browse?group_by=artist):

Uncaught Error: media_reference is not available on grouped media
    at get media_reference (3.BLltVHdj.js:49:18232)

The page is stuck on loading... with no tiles. The same database and URL renders all three groups correctly on 797e67a (the merge base).

this branch	main
stuck on "loading...", console error	3 group tiles render

(Screenshots are attached to the review summary comment.)

Beyond the immediate crash, the design here is what worries me most in a PR whose stated goal is type safety. Declaring media_reference(): model_types.MediaReference on the base class asserts to the compiler that every MediaViewRune — including MediaGroupRune — always has a media reference. That is exactly the claim that is false, and it is the claim svelte-check would otherwise have used to flag both call sites above. The types got stricter-looking while the program got less safe: a compile-time error was converted into a runtime one.

Suggested direction: don't put these members on the base class at all. media_reference, tags, thumbnails, star, update, add_view and load_detailed_view only exist on MediaReferenceRune, so let them live there. MediaViewRune then carries only what is genuinely common (media_type, media, preview_thumbnail, img_fit_classes), and AnyMediaViewRune consumers are forced to narrow on media_type before touching reference-only data — which is both correct and self-documenting. The two call sites above would then be compile errors that point at the real fix (skip the alt title for groups; make set_current_selection handle or reject grouped results explicitly).

[cursor]

Correction: the two image cells in the table above are dead placeholders, ignore them. The actual before/after screenshots are in the review summary comment on this PR.

[andykais]

So I think something got lost in translating earlier on in this pr. We do support media reference on a MediaGroupResponse. It is in group.media[].media_reference. Is there a reason not to just override the getter to MediaGroupRune?

[cursor]

You're right, and no — no reason not to. Fixed in 8c9123a.

MediaGroupRune now overrides the getter and pulls from the group's media:

/**
 * The media standing in for this group. Core supplies it on `group.media` when the
 * search asks for `grouped_media`, otherwise we fall back to the list this rune
 * fetches itself. Undefined until one of those has loaded.
 */
get #representative_media(): MediaResponse | undefined {
  return this.media.group.media?.at(0) ?? this.grouped_state.media_list.at(0)
}

override get media_reference(): model_types.MediaReference | undefined {
  return this.#representative_media?.media_reference
}

Two notes on the shape of it:

It's | undefined rather than a plain MediaReference. group.media is only populated when the search passes grouped_media.limit (the browse page doesn't), and the fallback list is fetched asynchronously in the constructor, so there's a real window where a group has no media yet. Declaring it non-optional would have put us right back to a getter that can't honor its type. The base declares MediaReference | undefined and MediaReferenceRune (files + series) narrows the override to MediaReference, so AnyMediaViewRune consumers get | undefined and anything that has narrowed on media_type still gets the guaranteed reference.

That made the two unguarded call sites compile errors, which is what I wanted out of this:

• media_selections.is_currently_selected — now takes the rune and compares by reference id when there is one, falling back to rune identity (which is what distinguishes tiles anyway).
• MediaDetails.svelte — the sidebar is entirely media-reference data (tags, stars, title, filepath), so it now derives a selected_media that excludes grouped selections. That also fixes .tags on a grouped selection, which had the same problem.

I also used #representative_media for preview_thumbnail and img_fit_classes, which were each reaching into grouped_state.media_list.at(0) separately.

Verified at runtime, since the report was that the view was broken: seeded a db with 4 files across 3 artist tags and loaded /browse?group_by=artist. All 3 group tiles render with thumbnails and counts, clicking a tile doesn't crash, and the console has no media_reference is not available error.

group-by browse view rendering three artist group tiles with no console errors

While MediaViewRunes was unused I dropped it (per your other comment), and made the erroring base methods async so they reject instead of throwing synchronously past a caller's .catch().

[cursor]

Adversarial review

I reproduced this branch end to end: installed Deno 2.9.0 (matching .github/actions/shared_setup), ran deno task --cwd packages/web check (0 errors, exit 0 — the PR's central claim holds), ran deno task --cwd packages/web build:local, seeded a small database with tags in an artist group, and drove the built GUI in a browser. Then I did the same against the merge base 797e67a for comparison.

The headline finding: group-by browse is broken on this branch

/browse?group_by=artist renders nothing and throws Uncaught Error: media_reference is not available on grouped media. The identical database and URL render correctly on 797e67a. Details and suggested fix are in the inline comment on media_view_rune.svelte.ts; the short version is that MediaGroupResponse has no media_reference, so the new base-class getter turned a benign undefined into a thrown error, and the new type signature is precisely what stops svelte-check from catching it.

This branch — stuck on loading..., console error visible:

group-by view broken on this branch

main (797e67a) — three group tiles render with counts:

group-by view working on main

Two things that undercut the CI step itself

The checked config is not the config anyone's editor uses. tsconfig.svelte.json is a second, divergent copy of tsconfig.json. CI now checks the new one and reports 0 errors; editors, svelte-kit sync, and the Svelte language server still read tsconfig.json, which on this exact branch reports 58 errors in 35 files (svelte-check --tsconfig ./tsconfig.json). So the day this merges, everyone's editor lights up red on code CI calls clean, and the two configs will drift further with every future change. Inline comment has suggestions.

The check silently depends on an undeclared Deno version. jsrDepsInNodeModules: true is what materializes @std/* into node_modules for tsc to find. On Deno 2.5.3 it does not, and deno task check fails with 36 bogus Cannot find module '@std/log'-style errors. It works on 2.9.0. Nothing in deno.json records that floor, and AGENTS.md still says "Deno (canary)".

Logs for both are attached: svelte_check_with_ide_tsconfig_58_errors.log, svelte_check_deno_2_5_3_36_errors.log, svelte_check_deno_2_9_0_passing.log.

Cross-cutting theme

Roughly two dozen as-casts, ! assertions and @ts-ignores were added to reach zero. Individually most are defensible; collectively they're load-bearing, and several sit exactly where an unsound assumption lives (the MediaSearchInput union in media_list_rune, the MutableSettings[K] casts in settings_rune, the ReadableWritablePair cast in core). A green check that rests on casts is weaker than the 139 errors it replaced, because the casts are invisible in future diffs. Where a cast is unavoidable I'd rather see it isolated behind one narrow, named helper than sprinkled at each use site — that way the next person can grep for the unsound spots.

Behavior changes hidden in a typing PR

Three of these are worth calling out because a reviewer scanning for "types only" would skip them: focusable now focuses on mount, unread_only's string-tolerance guard was dropped, and /files/media_file/[id] now 404s for series references. All are probably fine or improvements, but they deserve to be named in the PR description rather than discovered.

Drive-by (pre-existing, not introduced here)

• FileProcessor.#readlines discards the first chunk it reads — see the inline comment; the PR touches those exact lines.
• .github/workflows/web.yml has a stray second checkout repository + setup pair at the end of the test job, orphaned by the commented-out publish block. They ran as steps 6 and 7 in the latest Web CI run.
• SearchLink.svelte still has a console.log({params}) at module scope.
• SettingsRune.get() is missing the 'ui.sidebar.hide' case that set() has, so toggle('ui.sidebar.hide') would throw. Nothing calls it yet.

Individual comments are attached to the relevant lines.

[cursor]

This is the second half of the group-by crash. Widening the parameter from runes.MediaViewRune to runes.AnyMediaViewRune is the right call — MediaListRune.results really can contain MediaGroupRune — but the body was not updated to match. The is_currently_selected(media_response.media_reference.id) call on the next line, and the ?.media_reference?.id read inside is_currently_selected, both assume a media reference exists, and for a grouped rune it does not.

The compiler stays quiet only because the base class now advertises media_reference unconditionally (see my comment on media_view_rune.svelte.ts). If that getter is moved down to MediaReferenceRune, this becomes a compile error and you're forced to decide what clicking a group tile should actually do — my guess is either navigate into the group (the SearchLink behavior already in SearchResults.svelte) or be a no-op, but that's a product call rather than a typing one.

Same question applies to view_media() below: it calls load_detailed_view() and add_view() on whatever is selected, and neither is implemented for grouped runes.

[cursor]

Fixed alongside the getter change (8c9123a). Making media_reference optional on the base did turn this into a compile error, which is what pointed at both lines here.

is_currently_selected now takes the rune rather than an id, so it doesn't have to assume a reference exists:

private is_currently_selected(media_response: runes.AnyMediaViewRune) {
  const current = this.#current_selection.media_response
  if (!current) return false
  // grouped results have no media reference until their media loads, so fall back to
  // rune identity, which is what distinguishes one tile from another anyway
  if (current === media_response) return true
  const current_media_reference_id = current.media_reference?.id
  return current_media_reference_id !== undefined
    && current_media_reference_id === media_response.media_reference?.id
}

Identity is the honest comparison for a grouped tile, and it also means clicking the same tile twice still opens it (the existing behavior) rather than silently doing nothing.

On view_media(): it calls load_detailed_view() / add_view() on the selection, and those are the erroring base implementations for grouped runes. I left the product decision alone rather than guessing — clicking a group tile currently just selects it, and the erroring paths aren't reached because the group tile's actual navigation is the SearchLink in SearchResults.svelte. Verified in a browser: selecting a group tile shows the selection border with no console error. If you want clicking a group to drill into it, that's a small follow-up in set_current_selection.

[cursor]

The biggest future-proofing risk in this PR. This file is a near-copy of tsconfig.json with five extra settings, and nothing keeps the two in sync. CI checks this one; editors, the Svelte language server, and svelte-kit sync all read tsconfig.json. On this exact branch:

$ svelte-check --tsconfig ./tsconfig.json
svelte-check found 58 errors and 0 warnings in 35 files

$ svelte-check --tsconfig ./tsconfig.svelte.json
svelte-check found 0 errors and 0 warnings

So the moment this merges, anyone opening the repo sees 58 red squiggles on code CI calls clean — which is the fastest way to train people to ignore the editor, and to let real errors accumulate unnoticed between CI runs. The drift only widens, since every future paths/lib/alias change has to be made twice by hand and there's no test that catches forgetting.

The five deltas are verbatimModuleSyntax: false, the explicit lib, baseUrl, paths, and exclude. None of them look editor-hostile to me, so the simplest fix is to delete this file and put those settings directly in tsconfig.json, leaving check pointed at the default. If you'd rather keep the split, invert it: make tsconfig.json a thin {"extends": "./tsconfig.svelte.json"} so there is exactly one place the settings live.

Two narrower notes on the contents:

The paths re-declaration is a hand-maintained mirror of SvelteKit's generated config. The comment correctly explains why it's needed, but the list is a snapshot: it covers $lib, $lib/* and $app/types. Whenever svelte-kit sync starts emitting a new alias, or someone adds one via kit.alias in svelte.config.js, resolution silently breaks here only, and only in CI. Worth a line in the comment telling the next person to re-check .svelte-kit/tsconfig.json when they touch aliases.

The exclude does not do what the comment says. exclude only filters the include globs; files reached through an import are still added to the program and still produce diagnostics. That's demonstrable — the file_processor.ts change in this PR exists precisely because a core file reported an error through this config. So packages/core is not actually insulated, and future core-only type errors will surface as Web CI failures. Given that, "../core/**/*" plus "../core/**" (the same glob twice) is mostly decorative and the comment above it is misleading.

[andykais]

Do we need a separate tsconfig.json and tsconfig.svelte.json? If we don't collapse them. If we do, take the extends suggestion above.

[cursor]

Collapsed them — there's a single tsconfig.json again and tsconfig.svelte.json is deleted (8c9123a).

Trying it turned up a better answer than either option, though. Moving the settings into tsconfig.json verbatim made svelte-kit sync complain:

You have specified a baseUrl and/or paths in your tsconfig.json which interferes with SvelteKit's auto-generated tsconfig.json. Remove it to avoid problems with intellisense. For path aliases, use kit.alias instead.

So the workspace aliases now live in svelte.config.js instead:

kit: {
  adapter: adapter(),
  alias: {
    '@forager/core': '../core/src/mod.ts',
    '~': '../core/src',
  },
  ...
}

svelte-kit sync compiles those into .svelte-kit/tsconfig.json next to the $lib/$app entries it already generates, so tsconfig.json needs no baseUrl/paths at all. That kills the hand-maintained paths mirror the review was worried about — there is nothing left to keep in sync, and a future kit.alias addition flows through automatically. The warning is gone too.

On the other two sub-points:

• The exclude no longer claims to insulate core. I dropped the duplicated ../core/**/* glob and rewrote the comment to say what actually happens: it only trims the include globs, and core files reached through an import are still in the program and still report diagnostics. (That's exactly why the file_processor.ts cast is needed — see my reply on that thread.)
• Editor/CI drift is resolved by there being one file. svelte-check --tsconfig ./tsconfig.json is now what CI runs and what editors read.

Verified: deno task --cwd packages/web check → 0 errors, and deno task --cwd packages/web build still succeeds (worth checking, since kit.alias also feeds Vite's resolver, not just TypeScript).

[cursor]

This flag is now load-bearing for the new CI step, but the Deno version it needs is undeclared anywhere.

On Deno 2.5.3, deno install materializes the JSR deps only as node_modules/@jsr/std__*, so tsc cannot resolve the bare @std/* specifiers and deno task --cwd packages/web check exits 1 with 36 errors of the form:

Cannot find module '@std/log' or its corresponding type declarations.
Cannot find module '@std/path' or its corresponding type declarations.
Property 'info' does not exist on type 'Logger'.     # fallout from the above

On Deno 2.9.0 (what .github/actions/shared_setup pins) node_modules/@std appears and the check passes cleanly. So the check is green in CI and broken locally for anyone on an older toolchain, with an error message that points nowhere near the actual cause. AGENTS.md still just says "Deno (canary)".

Cheapest guard is to declare the floor so deno task refuses rather than misleads. Either add a "deno": ">=2.9" engine constraint if the version of Deno you're targeting honors it, or at minimum note the minimum version next to this flag and in AGENTS.md.

Also worth deciding deliberately: this is set at the workspace root, so it changes deno install for core and cli too, purely to satisfy web's typechecker.

[andykais]

This is fine. We already suggest using Canary/latest. I will solidify a stable version in the future

[cursor]

Nit, but the kind that bites later: @std/fmt and @std/fmt/bytes are now two independently pinned entries. Today they agree at 0.225.6; the next time someone bumps @std/fmt they will almost certainly not notice this line, and you'll silently be running two copies of the same package at different versions.

Since this exists to work around a subpath-rewrite gap in jsrDepsInNodeModules, a comment saying so — and saying "keep the version in sync with @std/fmt above" — would make the trap visible. Better still if it can be expressed as "@std/fmt/": "jsr:@std/fmt@0.225.6/" (trailing-slash prefix mapping), which keeps one version literal.

[andykais]

Good suggestion, make it so

[cursor]

Made it so, though not quite the suggested form — the trailing-slash prefix mapping doesn't survive jsrDepsInNodeModules. I tried it:

"@std/fmt/": "jsr:@std/fmt@0.225.6/",

error: Failed to resolve the specifier ""@std/fmt/bytes"" as its after-prefix portion
""bytes"" could not be URL-parsed relative to the URL prefix
"npm:@jsr/std__fmt@0.225.6/" mapped to by the prefix "@std/fmt/"

That's the same failure the explicit entry was working around in the first place — the prefix gets rewritten to its npm form, and the subpath can't be parsed against it.

What did work is dropping the bare @std/fmt entry entirely, since nothing imports it bare (core's only use is @std/fmt/bytes; lib/test/lib/debugger.ts uses a fully-qualified jsr:@std/fmt@0.225.4/colors specifier that doesn't go through the import map). So there's now a single entry and a single version literal, which gets the outcome you were after:

// NOTE mapped as an explicit subpath rather than a bare "@std/fmt" or an "@std/fmt/"
// prefix: `jsrDepsInNodeModules` rewrites those to their npm form
// ("npm:@jsr/std__fmt@0.225.6/"), which cannot resolve a subpath. Any other
// @std/fmt subpath we start importing needs its own entry here.
"@std/fmt/bytes": "jsr:@std/fmt@0.225.6/bytes",

Verified with a clean rm -rf node_modules && deno install plus deno check on core's entrypoint.

[cursor]

This line is a real behavior change, not a typing change, and it's easy to miss in a PR framed as typechecking.

Svelte 5 calls action(node, value) once on mount and only calls update on subsequent value changes (svelte/src/internal/client/dom/elements/actions.js guards the first run with an inited flag). So before this PR the initial parameter was discarded; now it's applied. In SearchResults.svelte the parameter is !current_selection.show && result_index === current_selection.result_index, which is true for the first tile on a fresh page load — so the first search result now grabs DOM focus as soon as the grid mounts, where previously nothing was focused until the user interacted.

That may well be an improvement, but it should be an intentional one. If it isn't wanted, mirror what scrollable.ts does and ignore the initial value.

Which is the other half of this: the sibling action got the exact opposite treatment in the same commit. scrollable(node, _focused) accepts the parameter purely to satisfy use:'s signature and drops it on the floor, while focusable now acts on it. Two adjacent actions, same underlying type error, opposite semantics, no comment explaining why. Whichever is correct, they should agree — and if scrollable genuinely should ignore its initial value, a one-line comment saying "initial scroll is intentionally skipped" would stop the next person from "fixing" the unused _focused.

[andykais]

This change was already discussed and approved above #79 (comment). Previous conversations on github contain valuable context for reviews.

[cursor]

Agreed — left focusable as is, per the earlier thread where the initial-focus behavior was confirmed as the desired fix rather than an accidental one.

I also left scrollable's _focused alone rather than adding a comment there, since that's a behavior question you haven't weighed in on and I didn't want to encode a guess. Say the word if you'd like the one-liner noting the initial scroll is intentionally skipped.

[cursor]

These four as MutableSettings[K] casts turn off the only check that made this switch statement trustworthy, and there's already a live example of what that costs: set() handles five paths, this get() handles four — 'ui.sidebar.hide' is missing, so toggle('ui.sidebar.hide') throws Unexpected path. Nothing calls it today, but ToggleSidebar is a configured keybind, so it's one wiring change away from being reachable. With the casts in place, adding a sixth setting to MutableSettings will compile fine while silently being unreadable here.

The good news is the fix is already in this file — set() solves exactly this problem by projecting to a MutableSettingsUpdate discriminated union so each case narrows update.value properly. The read direction can use the same shape:

type MutableSettingsEntry = {
  [K in keyof MutableSettings]: { path: K; value: MutableSettings[K] }
}[keyof MutableSettings]

#entries(): MutableSettingsEntry[] {
  return [
    { path: 'ui.media_list.thumbnail_size',      value: this.ui.media_list.thumbnail_size },
    { path: 'ui.media_list.thumbnail_shape',     value: this.ui.media_list.thumbnail_shape },
    { path: 'ui.media_view.fit.mode',            value: this.ui.media_view.fit.mode },
    { path: 'ui.search.advanced_filters.hide',   value: this.ui.search.advanced_filters.hide },
    { path: 'ui.sidebar.hide',                   value: this.ui.sidebar.hide },
  ]
}

Annotate that array as satisfies [...]-with-a-length-check, or simply as Record<keyof MutableSettings, ...>-shaped, and a missing path becomes a compile error instead of a runtime throw — which is the outcome this PR is trying to buy.

Minor, in the same vein: this.set(path, !value as MutableSettings[K]) in toggle() is a cast asserting something the typeof value !== 'boolean' guard above it already proved. MutableSettings[K] & boolean or a narrowed local would keep the guard load-bearing.

[andykais]

I do not know if a pattern like this affects svelte's compiler. Check if it matters. If this changes behavior or makes svelte behavior more ambiguous I am willing to lose the extra type safety.

Also just a note that the cast to MutableSettings has nothing to do with the missing key in get(). Thats because this isn't an exhaustive type check. There are other solutions here that don't force rewriting the static returns

[cursor]

Checked, and it does matter — so I kept the static returns and got the exhaustiveness a different way (8c9123a).

On the Svelte question: the suggested #entries() array builds every setting's value eagerly on each call. Svelte 5 tracks $state reads, so a call made inside a $derived/$effect would subscribe to all five settings instead of the one that was asked for — broader and less predictable invalidation, plus an allocation per read. Today get() is only reachable via toggle(), which is called from an event handler in SearchParams.svelte, so nothing would break right now; it's the kind of thing that goes wrong later when someone reads a setting from a derived. Not worth it, so I didn't adopt it.

And you're right that the casts weren't the cause of the missing key — the switch simply wasn't exhaustive in either direction. Fixed both parts without touching the per-case reads:

private get<K extends keyof MutableSettings>(path: K): MutableSettings[K] {
  return this.#read(path) as MutableSettings[K]
}

// NOTE this is deliberately non-generic. With a plain union `path` narrows per case,
// so the default branch can assert exhaustiveness and adding a MutableSettings key
// without a case here fails to compile. Each case still reads exactly one setting,
// so a caller only subscribes to the value it asked for.
#read(path: keyof MutableSettings): MutableSettings[keyof MutableSettings] {
  switch(path) {
    case 'ui.media_list.thumbnail_size': { return this.ui.media_list.thumbnail_size }
    // …
    case 'ui.sidebar.hide': { return this.ui.sidebar.hide }   // was missing
    default: {
      const unhandled: never = path
      throw new Error(`Unexpected path '${unhandled}'`)
    }
  }
}

The generic K was what stopped path from narrowing to never in the default branch; splitting the plain-union read out fixes that, drops the four per-case casts (one remains at the generic boundary), and adds the missing 'ui.sidebar.hide'. I added the same never assertion to set()'s default, which its MutableSettingsUpdate projection already made possible.

I confirmed the guards actually fire by temporarily adding a sixth key ('ui.sidebar.size') — both switches fail to compile until it's handled:

Error: Type '{ path: "ui.sidebar.size"; value: number; }' is not assignable to type 'never'.
Error: Type '"ui.sidebar.size"' is not assignable to type 'never'.

[cursor]

These two casts are the load-bearing ones, and they're asserting something the code doesn't actually guarantee.

#saved_params is a single untagged bag built by {...this.#saved_params, ...params?.params}, so keys from a previous call survive into the next one. params_type is remembered separately in #saved_params_type. Nothing ties the two together: if paginate({type: 'media', ...}) ever runs after a group_by paginate without an intervening clear(), the leftover group_by key rides along into media.search, and parsers.PaginatedSearch is strict, so core rejects it. Today #execute_search always calls clear() first so it isn't reachable — but that's an invariant held by a caller in a different file, which is exactly the kind of thing that quietly stops being true.

Since Input is already a discriminated union, the cleanest fix is to keep the params discriminated too, e.g. store #saved_input: Input | undefined and match on .type, rather than storing a merged object and re-asserting its shape at each call site. That deletes both casts and the MediaSearchInput union in media_view_rune.svelte.ts, which currently exists only to paper over the same erasure.

Separately, the line between these two: fetch_params.limit = fetch_params.limit ?? 30 mutates in place, and when #cursor === undefined fetch_params is this.#saved_params (no spread happens on that branch), so this writes back into saved state. Benign today because the value is idempotent, but it's an aliasing trap for the next person who adds a mutation here. fetch_params = {...fetch_params, limit: fetch_params.limit ?? 30} costs nothing.

[andykais]

I think we can ignore a rewrite here for now. Your suggestion still has to deal with weird polymorphism, it just moves it from this one site.

[cursor]

Agreed, left the structure alone — no discriminated-union rewrite, and MediaSearchInput stays.

I did take the one-line half of that comment, since it isn't a rewrite: fetch_params.limit = fetch_params.limit ?? 30 was mutating in place, and on the no-cursor branch fetch_params is #saved_params, so it was writing back into saved state. Now it copies:

// NOTE not mutated in place: when there is no cursor, fetch_params still aliases
// #saved_params, and writing through it would leak into saved state
fetch_params = {...fetch_params, limit: fetch_params.limit ?? 30}

Same behavior today (the value is idempotent), just removes the aliasing trap.

[cursor]

Worth naming what's happening structurally here: core source is being edited to satisfy a typechecker configuration that lives in web and uses a different lib set than the one core is actually compiled under. deno check on this file was already passing before the change. That's a coupling I'd push back on — it means core's code now carries a workaround for a consumer's build settings, and there's no test or comment that would tell someone deleting this cast why it was needed.

It also contradicts the "exclude": ["../core/**"] in tsconfig.svelte.json, whose comment claims core is out of the checked fileset. exclude doesn't apply to files pulled in via import, which is why this change was necessary at all. If the goal really is to keep core out of web's diagnostics, that needs a different mechanism (a .d.ts boundary, or building core's types), and then this cast can go away.

If it stays, a narrower assertion is safer than the cast on the whole object. as unknown as ReadableWritablePair<string, Uint8Array> at least fails loudly if TextDecoderStream's shape ever diverges further, whereas the current single as will keep compiling while the two types drift.

Unrelated, since you're in this function anyway — I think the loop below drops the first chunk:

let read_result = await reader.read()
while (!read_result.done) {
  read_result = await reader.read()   // first chunk's value is overwritten before use
  buffer += read_result.value ?? ''

The value from the initial read() is never appended. For a short ffprobe/ffmpeg stderr that arrives in one chunk, that means the entire output is discarded. It only goes unnoticed because the caller at line 515 uses this for progress reporting. Pre-existing, not yours, but a good candidate for a follow-up.

[andykais]

Hmm. Alright if this change is actually not necessary, drop it. Ignore the dropped first chunk issue though. I will follow up on that separately.

[cursor]

Tested it, and it's genuinely necessary — so I kept it, but made the comment explain why, since the reason isn't visible from core.

Removing the cast leaves exactly one error in the web check:

$ deno task --cwd packages/web check
Types of property 'writable' are incompatible.
  Type 'WritableStream<BufferSource>' is not assignable to type 'WritableStream<Uint8Array<ArrayBufferLike>>'.
svelte-check found 1 error and 0 warnings in 1 file

The trap is that deno check packages/core/src/lib/file_processor.ts passes either way — Deno's lib types TextDecoderStream compatibly, TypeScript's DOM lib doesn't. So the next person to look at this file has no local signal that removing it breaks anything. The comment now says so explicitly:

// NOTE `deno check` passes without this, but @forager/web type-checks core's source
// under the DOM lib, where it does not. Removing this breaks `deno task --cwd
// packages/web check`.

Ignored the dropped-first-chunk issue as you asked.

One correction on the suggestion attached to that comment: switching to as unknown as ReadableWritablePair<…> would be weaker, not stronger. A direct as still requires the two types to sufficiently overlap and will error if TextDecoderStream's shape diverges; as unknown as disables that check entirely. So I left the single assertion.

Worth flagging that the underlying coupling is real and unresolved: core's source is in web's program (exclude doesn't apply to imported files), so core type errors can surface as Web CI failures. Removing that properly means putting a .d.ts boundary between them or building core's types — a bigger change than this PR, and then this cast can go away. Happy to open a follow-up issue for it if you want it tracked.

[cursor]

Coercing here is the right fix, but it's only one of three doors into params.unread_only, and the other two got looser rather than tighter in this PR.

#execute_search used to read if (params.unread_only === 'true' || params.unread_only === true); it now reads if (params.unread_only). That's honest given the declared boolean type, but the type is only honest for values that came through this branch. merge(partial_params: Partial<Record<string, any>>) still does // @ts-ignore + params[params_key] = val with val: any, so a caller passing {unread: 'false'} writes the string 'false' into a field declared boolean, and the new truthiness check turns it into unread: true — the exact inversion the old guard existed to prevent. No current caller does this (SearchLink only passes mode and tags), so it's latent, not live.

The same @ts-ignore escape hatch covers sort, order, search_mode and group_by in this function's else branch too, so ?sort=garbage still reaches core's zod parser as a raw string. Since you're already special-casing per key here, the durable version is a small per-key parser table — Record<keyof SearchParams, (raw: string) => value> — which makes the @ts-ignore unnecessary and makes "did we handle this param" a compile-time question rather than a reading-comprehension one.

[andykais]

Hmm we should feel comfortable keeping this if/else logic. The changes I would suggest are:

• switch up parse_url to loop over known keys in URL_PARAM_MAP (and include every key in here now)
• handle every case in parse_url explicitly.
• related, merge() should be a type safe Partial on SearchParams rather than a Partial<Record which doesnt do anything

[cursor]

Did all three (8c9123a). The if/else logic is still there, just driven off known keys.

1. URL_PARAM_MAP now has every key, and is typed Record<keyof SearchParams, string> rather than Partial<…>, so adding a param without deciding its URL name is a compile error:

const URL_PARAM_MAP = {
  search_string: 'tags',
  filepath: 'filepath',
  sort: 'sort',
  unread_only: 'unread',
  search_mode: 'mode',
  group_by: 'group_by',
  stars: 'stars',
  stars_equality: 'stars_equality',
  order: 'order',
  media_type: 'type',
} as const satisfies Record<keyof SearchParams, string>

2. parse_url loops the known keys and handles every case explicitly. It reads search.get(URL_PARAM_MAP[key]) instead of iterating whatever the URL happens to contain, so unknown params are ignored rather than written through, and the @ts-ignore is gone. Each case validates: ?sort=garbage or ?type=garbage now falls back to the default instead of reaching core's zod parser as a raw string. The default branch asserts never, so a new param can't silently skip parsing. The reverse map (URL_PARAM_MAP_REVERSED) is deleted — nothing needs it now.

Validators are declared as Record<Union, true> rather than Sets so a value added to any of those unions fails to compile until it's listed.

3. merge() takes Partial<SearchParams> and drops the @ts-ignore and the reverse-name lookup. Its two callers passed URL names, so they're updated to internal names: merge({mode, tags}) → merge({search_mode, search_string}) in SearchResults.svelte, and merge({tags}) → merge({search_string}) in MediaDetails.svelte. search_string is still additive; switching away from group_by still clears group_by.

One real bug this surfaced: the sort dropdown offers <option value="count"> in group_by mode, but SearchParams.sort was typed inputs.PaginatedSearch['sort_by'], which excludes 'count' — so the UI was writing a value the type said was impossible. sort is now inputs.PaginatedSearchGroupBy['sort_by'] (the superset), and #execute_search maps 'count' back to the default when running a plain media search, since core rejects it there. That mirrors what MediaGroupRune already does when it converts group params into a media search.

I also exported SearchParams while I was in here, for the SearchLink comment.

[cursor]

Good change — InferSpec resolving instead of collapsing to never is worth a lot, and I checked these three allowlists against MediaActions, SeriesActions and ViewActions: they're complete as of this commit, so nothing is lost.

The concern is that they're complete as of this commit and nothing enforces it. Adding a method to MediaActions in core will not surface here; the web client just won't be able to call it, and the failure mode is a confusing "property does not exist" at some unrelated call site rather than anything pointing at this file. That's a new maintenance obligation created by a PR about type safety, so it'd be nice to close it with types:

class ForagerMediaApi extends rpc.ApiController<Context> {
  // …
}
// fails to compile if a core action is added without being exposed here
type _assert_media_complete = ForagerMediaApi extends Record<keyof Forager['media'], unknown> ? true : never

or the inverse direction if you'd rather allowlist deliberately and just be told when core grows — either way the drift becomes a compile error instead of a bug report.

Worth noting for the record that this is also a tightening on the wire: previously media = this.context.forager.media exposed whatever the actions object happened to carry, so the RPC surface silently grew with core. Pinning it explicitly is the safer default. delete and reload are both still on the list, so nothing changes about what's reachable today.

[andykais]

I don't care about this. We should feel comfortable only extending the forager api when we need things. I have a long term plan to avoid the need for this static list at all

[cursor]

This creates two sources of truth for the same value: the new defaults constructor argument, and the abstract get DEFAULTS(): TParams on line 41 that parse_url and serialize still read from. TagQueryParams happens to pass the same module-level DEFAULTS const to both, so they agree — but nothing checks that, and a subclass that passes a tweaked object to super() while returning the plain const from DEFAULTS would get initial state and default-elision that disagree, with no error anywhere.

Worth noting the change may not have been necessary: DEFAULTS is a prototype getter, so this.DEFAULTS is already reachable from the base constructor at runtime. If TypeScript was complaining about something else here, that's worth a comment. Otherwise dropping the parameter and going back to {...this.DEFAULTS} keeps it to one definition; if you want the constructor arg, make DEFAULTS a concrete readonly field assigned from it rather than a separate abstract getter.

[andykais]

Note that I believe this was necessary because of typescript (and javascript's) constraints around property getters in base and child class constructors. If this param is not necessary, try to just use the DEFAULTS getter, but I believe we need this.

[cursor]

Your recollection is right — the param is necessary. Reverting to {...this.DEFAULTS} in the constructor gives:

Abstract property 'DEFAULTS' in class 'BaseQueryParams' cannot be accessed in the constructor. ts(2715)

TypeScript blocks it because a subclass's getter would run before the subclass's own field initializers, so it can't guarantee a valid value. (Reachable at runtime via the prototype, as noted, but not something tsc will let through.)

What I did fix is the two-sources-of-truth part: DEFAULTS is no longer a separate abstract getter that subclasses re-declare. It's a concrete readonly field assigned from the constructor argument, so the value passed to super() is the one parse_url/serialize read:

// NOTE assigned from the constructor rather than declared as an abstract getter:
// TypeScript forbids reading an abstract member from the base constructor (ts2715),
// and the defaults are needed here to seed `current`/`draft`.
public readonly DEFAULTS: TParams

constructor(client: BaseController['client'], defaults: TParams) {
  super(client)
  this.DEFAULTS = defaults
  // …
}

TagQueryParams's get DEFAULTS() is deleted; it just passes the const to super() now. One definition, and a subclass can no longer pass one object to super() while returning a different one from DEFAULTS.

[cursor]

The ! is unnecessary and slightly misleading — it tells the reader "trust me" about something the code can actually prove. Assigning to a local makes the narrowing real:

if (API_SINGLETON === undefined) {
  API_SINGLETON = rpc.adapt(Api, params.locals)
}
const api = API_SINGLETON
return await api(params)

Trivial, but this is the kind of ! that survives a later refactor where the guard moves or becomes conditional, and then it's silently lying.

Unrelated but adjacent: this lazily initializes a module-level singleton from the first request's params.locals, and every subsequent request reuses that first request's context. Given the comment above it ("stores stateful information like realtime connections"), that's presumably deliberate — but since locals is per-request and hooks.server.ts writes both forager and config into it, it's worth a word in that comment confirming the captured context is intentionally request-independent.

[andykais]

meh, I guess its worth fixing. Go for it

[cursor]

Done. The suggested const api = API_SINGLETON after the if didn't actually work — TypeScript won't narrow a module-level let inside an async handler, so it still reported Cannot invoke an object which is possibly 'undefined'. ??= does narrow, and it's shorter than what was there:

export const PUT: RequestHandler = async (params) => {
  const api = API_SINGLETON ??= rpc.adapt(Api, params.locals)
  return await api(params)
}

No !, same lazy-once behavior.

On the adjacent observation: the singleton does capture the first request's params.locals and reuse it for every subsequent request. hooks.server.ts assigns the same module-level forager and config into locals on every request, so the captured context is equivalent per-request today — but that's an invariant of hooks.server.ts, not something this file enforces. I left the behavior alone since it looks deliberate given the comment above it; say the word if you'd like a note added there, or the context resolved per-request instead.

[cursor]

Two smaller things in this block, separate from the media_reference problem above.

update's parameters were widened further than needed. MediaDetails.svelte calls update(undefined, {remove: [...]}), which justifies media_info?. Nothing calls it without tags, so tags? is a loosening with no caller behind it — and it makes the no-op rune.update() a valid call that silently issues an empty update. Worth keeping tags required, or giving it a meaningful default.

These declared return types are lies the compiler will believe. Each of these is typed Promise<void> but the body is a synchronous throw, so a caller doing rune.update(...).catch(handle) compiles and then throws synchronously past the handler. If you keep the throwing-base-class shape at all, abstract members (with no body) express the same intent without the mismatch, and get you exhaustiveness for free when a new subclass is added.

Last nit, on line 256-257: MediaViewRunes and AnyMediaViewRune are now two aliases for almost the same union, differing only by MediaGroupRune and by member order. Two similarly-named unions in one module is a coin flip for the next person picking one. MediaViewRunes looks unused after this change — worth deleting it, or renaming the pair to something that states the distinction (MediaReferenceRunes vs AnyMediaViewRune).

[andykais]

Since MediaViewRunes is unused, drop it.

[cursor]

Dropped — confirmed it had no references anywhere in packages/web/src. AnyMediaViewRune is the only union now, so there's no coin flip about which to pick.

For the record on the other two points in that comment, since I touched this block anyway:

• The declared Promise<void> return types with synchronous throw bodies are fixed by making those base methods async, so they reject rather than throwing past a caller's .catch(). That keeps the erroring-base-class shape you asked for rather than switching them to abstract.
• I left update's tags? optional. MediaDetails.svelte builds tags conditionally (let tags: {add: string[]} | undefined) and passes it through, so the call really can be update(media_info, undefined). Making it required would just move the undefined to the call site. Happy to tighten it if you'd rather.

[cursor]

Narrowing before .media_file is correct, and the guard is genuinely needed since forager.media.get returns MediaFileResponse | MediaSeriesResponse. But this is another behavior change worth naming in the PR description: previously requesting a series id from this endpoint threw a TypeError on undefined.filepath and surfaced as a 500; it now returns a 404. Better behavior, just not a typing change.

One thought on the status code: 404 says "no such resource", but the media reference does exist — it's the wrong kind. A 400 (or 409) with the same message would be more accurate and would let a client distinguish "bad id" from "id belongs to a series". Minor, and only matters if anything ever branches on the status.

[andykais]

Agreed. Make this a 400 👍

[cursor]

Changed to 400 👍

// the reference exists, it is just the wrong kind, so this is a bad request rather than
// a missing resource
if (media.media_type !== 'media_file') {
  throw error(400, 'Media reference is not a media file')
}

The genuine "no such media reference" case is still a 404 — that's the NotFoundError catch just above, which is untouched — so a client can now distinguish the two.

[cursor]

Deriving a prop type from the type of a value is indirection that will read as a mistake later — DEFAULTS is a set of default values that happens to be typed SearchParams, and this prop wants SearchParams. The reason for the detour is that SearchParams isn't exported from browse/runes/queryparams.svelte.ts; exporting it and writing params: SearchParams says what's meant and survives someone changing DEFAULTS to a Partial or a factory.

Also flagging the semantic change, since it isn't just a rename: this went from Partial<…> to the full type, so every field is now required. Both current call sites pass complete objects (queryparams.merge(...) and {...queryparams.DEFAULTS, …}) so nothing breaks, and requiring the full shape does match what serialize() consumes. Just noting it's a contract tightening on a public-ish component rather than a no-op.

[andykais]

Sure we can export SearchParams and use that for declared types. That is a fine change

[cursor]

Done — SearchParams is exported from browse/runes/queryparams.svelte.ts and SearchLink declares the prop directly:

import type { SearchParams } from '../runes/queryparams.svelte.ts'

interface Props {
  controller: BrowseController
  params: SearchParams
  …
}

No more deriving the type from the type of the DEFAULTS value. The contract is unchanged from the previous commit (still the full shape, not Partial), and both call sites pass complete objects — queryparams.merge(...) returns a full SearchParams, and MediaDetails spreads {...queryparams.DEFAULTS, search_string}.

[andykais]

Make this a github comment justifying the change, but remove it from code.

[andykais]

is it possible to use inputs. for these?

[andykais]

lets avoid this alias and just reference things directly by model_types.Tag

[andykais]

What is this used for? Is this for resolving @forager/core module references from within @forager/web. That should work, but I assume if we ever introduce another package in this monorepo that also has ~ import aliases, this solution will no longer work. Just confirm that is the case behind your reasoning, and suggest alternatives. Do not make changes here yet.