diff --git a/docs/specs/scoped-search.md b/docs/specs/scoped-search.md index eaae19c3..1706a2ca 100644 --- a/docs/specs/scoped-search.md +++ b/docs/specs/scoped-search.md @@ -8,8 +8,14 @@ > is being moved into Rust. The **user-facing behaviour and UX in this spec are > unchanged**; only where the scope→item-type mapping and result bucketing live > changes. Read the boundary spec before touching search code. +> +> **Progress:** the scope→item-type mapping now lives in Rust +> (`SearchScope::item_types()`); the frontend sends an opaque scope. Result-side +> bucketing (`GROUP_ITEM_TYPES`) is still frontend-side — see +> [scoped-search-boundary-implementation.md](scoped-search-boundary-implementation.md) +> §Stage 2. -**Status:** Implemented (boundary revision pending — see banner above) +**Status:** Implemented (boundary revision: query side done, result side pending) **Scope:** Frontend only. No Rust changes required. *(Revised — see banner.)* **Requirements:** UR-049 → DR-063, DR-064, DR-065; UR-050 → DR-066, DR-067 (see [requirements.md](../requirements.md)). diff --git a/src-tauri/src/commands/repository.rs b/src-tauri/src/commands/repository.rs index eee6c1e2..cc6dbdab 100644 --- a/src-tauri/src/commands/repository.rs +++ b/src-tauri/src/commands/repository.rs @@ -395,7 +395,12 @@ pub struct SearchUpdateEvent { pub result: SearchResult, } -/// Search for items +/// Search for items. +/// +/// Resolves `SearchOptions::scope` into concrete Jellyfin item types before +/// dispatching, so scope taxonomy stays in Rust. +/// +/// TRACES: UR-049, UR-050 | DR-063 #[tauri::command] #[specta::specta] pub async fn repository_search( @@ -408,6 +413,16 @@ pub async fn repository_search( ) -> Result { let repo = manager.0.get(&handle).ok_or("Repository not found")?; + // Expand the opaque scope into item types HERE — once, before the cache and + // server paths diverge — so both phases filter identically. Doing it later + // (or in only one path) makes offline results disagree with online ones. + // The frontend sends `scope` and never names a Jellyfin item type for + // search; see docs/specs/scoped-search-boundary.md. + let options = options.map(|mut o| { + o.resolve_scope(); + o + }); + // Phase 1: instant local results from the cache (downloaded content) so the // UI can render immediately while the server is still being queried. let mut cache_result = repo diff --git a/src-tauri/src/repository/types.rs b/src-tauri/src/repository/types.rs index 61d5c0ec..1a8784fe 100644 --- a/src-tauri/src/repository/types.rs +++ b/src-tauri/src/repository/types.rs @@ -294,6 +294,53 @@ pub struct GetItemsOptions { pub genres: Option>, } +/// An opaque search scope the frontend selects; Rust owns what it *means*. +/// +/// The expansion table below is Jellyfin domain vocabulary: it changes when +/// Jellyfin adds or renames an item type, never when the UI is redesigned. It +/// previously lived in the frontend (`searchScope.ts`), which is the boundary +/// leak documented in docs/specs/scoped-search-boundary.md. The frontend now +/// sends the enum and never names an item type in connection with search. +/// +/// TRACES: UR-049 | DR-063 +#[derive(specta::Type, Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum SearchScope { + All, + Music, + Movies, + Tv, +} + +impl SearchScope { + /// The Jellyfin item types this scope requests, or `None` for `All`. + /// + /// `All` returns `None` rather than the union of every listed type on + /// purpose: an explicit `includeItemTypes` list filters out anything not + /// named in it, so a union would silently drop People, folders and any type + /// nobody enumerated. Callers must omit the filter entirely on `None`. + /// + /// TRACES: UR-049 | DR-063 + pub fn item_types(self) -> Option> { + match self { + SearchScope::All => None, + SearchScope::Music => Some( + ["MusicAlbum", "MusicArtist", "Audio", "Playlist"] + .into_iter() + .map(String::from) + .collect(), + ), + SearchScope::Movies => Some(vec!["Movie".to_string()]), + SearchScope::Tv => Some( + ["Series", "Episode"] + .into_iter() + .map(String::from) + .collect(), + ), + } + } +} + /// Options for search queries #[derive(specta::Type, Debug, Clone, Serialize, Deserialize, Default)] #[serde(rename_all = "camelCase")] @@ -304,6 +351,28 @@ pub struct SearchOptions { pub include_item_types: Option>, #[serde(skip_serializing_if = "Option::is_none")] pub search_term: Option, + /// Opaque scope selected by the UI. When set it **wins** over + /// `include_item_types`, which remains for the non-search `get_items` + /// callers that legitimately request a single concrete type. + #[serde(skip_serializing_if = "Option::is_none")] + pub scope: Option, +} + +impl SearchOptions { + /// Expand `scope` into `include_item_types` in place. + /// + /// Call this once, in the search command, *before* dispatching to the + /// cache and server paths — both already honour `include_item_types`, and + /// resolving in one place keeps online and offline results identical. + /// + /// TRACES: UR-049 | DR-063 + pub fn resolve_scope(&mut self) { + if let Some(scope) = self.scope { + // `All` yields None, which clears the filter — the correct + // behaviour, not an omission. + self.include_item_types = scope.item_types(); + } + } } /// Playback information @@ -455,6 +524,131 @@ impl MeaningfulContent for PlaylistCreatedResult { } } +#[cfg(test)] +mod search_scope_tests { + use super::*; + + /// Music expands to the four Jellyfin types that make up the category. + /// + /// This table is the domain vocabulary that used to live in the frontend + /// (`searchScope.ts`'s `SCOPE_ITEM_TYPES`) — the boundary leak that + /// docs/specs/scoped-search-boundary.md was written about. + /// + /// @req-test: UT-089 - SearchScope expands to Jellyfin item types + #[test] + fn music_scope_expands_to_music_item_types() { + assert_eq!( + SearchScope::Music.item_types(), + Some(vec![ + "MusicAlbum".to_string(), + "MusicArtist".to_string(), + "Audio".to_string(), + "Playlist".to_string(), + ]) + ); + } + + /// @req-test: UT-089 - SearchScope expands to Jellyfin item types + #[test] + fn movies_scope_expands_to_movie_only() { + assert_eq!( + SearchScope::Movies.item_types(), + Some(vec!["Movie".to_string()]) + ); + } + + /// @req-test: UT-089 - SearchScope expands to Jellyfin item types + #[test] + fn tv_scope_expands_to_series_and_episode() { + assert_eq!( + SearchScope::Tv.item_types(), + Some(vec!["Series".to_string(), "Episode".to_string()]) + ); + } + + /// `All` must send NO filter — not the union of the other scopes. + /// + /// Sending a union would silently drop every type nobody enumerated + /// (Person, folders, …), which an explicit `includeItemTypes` list filters + /// out. This is why `item_types()` returns Option rather than Vec. + /// + /// @req-test: UT-090 - All scope sends no item-type filter + #[test] + fn all_scope_sends_no_filter() { + assert_eq!(SearchScope::All.item_types(), None); + } + + /// Scope wins over an explicitly supplied include_item_types. + /// + /// @req-test: UT-091 - Scope takes precedence over include_item_types + #[test] + fn resolve_scope_overrides_include_item_types() { + let mut options = SearchOptions { + include_item_types: Some(vec!["Movie".to_string()]), + scope: Some(SearchScope::Music), + ..Default::default() + }; + options.resolve_scope(); + + assert_eq!( + options.include_item_types, + Some(vec![ + "MusicAlbum".to_string(), + "MusicArtist".to_string(), + "Audio".to_string(), + "Playlist".to_string(), + ]) + ); + } + + /// `All` clears any include_item_types so no filter reaches the query. + /// + /// @req-test: UT-090 - All scope sends no item-type filter + #[test] + fn resolve_all_scope_clears_include_item_types() { + let mut options = SearchOptions { + include_item_types: Some(vec!["Movie".to_string()]), + scope: Some(SearchScope::All), + ..Default::default() + }; + options.resolve_scope(); + + assert_eq!(options.include_item_types, None); + } + + /// With no scope set, include_item_types passes through untouched — the + /// non-search `getItems` callers rely on this. + /// + /// @req-test: UT-091 - Scope takes precedence over include_item_types + #[test] + fn resolve_without_scope_preserves_include_item_types() { + let mut options = SearchOptions { + include_item_types: Some(vec!["MusicAlbum".to_string()]), + scope: None, + ..Default::default() + }; + options.resolve_scope(); + + assert_eq!( + options.include_item_types, + Some(vec!["MusicAlbum".to_string()]) + ); + } + + /// The frontend sends the enum as camelCase over IPC. + /// + /// @req-test: UT-089 - SearchScope expands to Jellyfin item types + #[test] + fn scope_deserializes_from_camel_case() { + let options: SearchOptions = + serde_json::from_str(r#"{"scope": "music", "limit": 10}"#).unwrap(); + assert!(matches!(options.scope, Some(SearchScope::Music))); + + let all: SearchOptions = serde_json::from_str(r#"{"scope": "all"}"#).unwrap(); + assert!(matches!(all.scope, Some(SearchScope::All))); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/lib/api/bindings.ts b/src/lib/api/bindings.ts index bb0df990..cb5d9710 100644 --- a/src/lib/api/bindings.ts +++ b/src/lib/api/bindings.ts @@ -1290,7 +1290,12 @@ async repositoryGetGenres(handle: string, parentId: string | null) : Promise { return await TAURI_INVOKE("repository_search", { handle, query, options, requestId }); @@ -2517,11 +2522,29 @@ failed: number } /** * Options for search queries */ -export type SearchOptions = { limit?: number | null; includeItemTypes?: string[] | null; searchTerm?: string | null } +export type SearchOptions = { limit?: number | null; includeItemTypes?: string[] | null; searchTerm?: string | null; +/** + * Opaque scope selected by the UI. When set it **wins** over + * `include_item_types`, which remains for the non-search `get_items` + * callers that legitimately request a single concrete type. + */ +scope?: SearchScope | null } /** * Search result with pagination */ export type SearchResult = { items: MediaItem[]; totalRecordCount: number } +/** + * An opaque search scope the frontend selects; Rust owns what it *means*. + * + * The expansion table below is Jellyfin domain vocabulary: it changes when + * Jellyfin adds or renames an item type, never when the UI is redesigned. It + * previously lived in the frontend (`searchScope.ts`), which is the boundary + * leak documented in docs/specs/scoped-search-boundary.md. The frontend now + * sends the enum and never names an item type in connection with search. + * + * TRACES: UR-049 | DR-063 + */ +export type SearchScope = "all" | "music" | "movies" | "tv" /** * Security status info */ diff --git a/src/lib/stores/library.ts b/src/lib/stores/library.ts index 3936bd7f..ea4c096a 100644 --- a/src/lib/stores/library.ts +++ b/src/lib/stores/library.ts @@ -5,7 +5,7 @@ import { writable, derived } from "svelte/store"; import { listen, type UnlistenFn } from "@tauri-apps/api/event"; import type { Library, MediaItem, SearchResult, Genre } from "$lib/api/types"; import type { SearchOptions } from "$lib/api/bindings"; -import { scopeItemTypes, type SearchScope } from "$lib/utils/searchScope"; +import type { SearchScope } from "$lib/utils/searchScope"; import { auth } from "./auth"; /** @@ -227,12 +227,13 @@ function createLibraryStore() { /** * Search the library, optionally narrowed to a scope. * - * `scope` is additive and defaults to `all`, which sends no - * `includeItemTypes` at all — see scopeItemTypes() for why that differs from - * listing every type. Both the online and offline repository paths already - * honour the filter. + * The scope is sent **opaque**; Rust expands it into Jellyfin item types + * (`SearchScope::item_types()`) before the cache and server paths diverge, so + * online and offline results filter identically. `all` resolves to no filter + * at all — not the union of the other scopes, which would drop People and + * folders. * - * TRACES: UR-049 | DR-065 + * TRACES: UR-049 | DR-063, DR-065 */ async function search(query: string, scope: SearchScope = "all") { // Bump the request id for every call (including clears) so any in-flight @@ -259,10 +260,9 @@ function createLibraryStore() { // Phase 1: the command resolves with instant local-cache results. The // merged (cache + server) union arrives later via the `search-event` // listener above, tagged with this same requestId. - const itemTypes = scopeItemTypes(scope); - const options: SearchOptions = { limit: 10000 }; - // Omit the key entirely for the `all` scope rather than sending null. - if (itemTypes) options.includeItemTypes = itemTypes; + // Send the opaque scope; Rust expands it to item types. The frontend + // never names a Jellyfin item type in connection with search. + const options: SearchOptions = { limit: 10000, scope }; const result = await Promise.race([ repo.search(query, options, requestId), diff --git a/src/lib/stores/librarySearchScope.test.ts b/src/lib/stores/librarySearchScope.test.ts index 4490babe..871ca1f6 100644 --- a/src/lib/stores/librarySearchScope.test.ts +++ b/src/lib/stores/librarySearchScope.test.ts @@ -32,35 +32,43 @@ describe("library.search scoping", () => { library.clearSearch(); }); - it("omits includeItemTypes entirely for the default (all) scope", async () => { + // The frontend sends the OPAQUE scope and never names a Jellyfin item type. + // Expansion (music → MusicAlbum/MusicArtist/Audio/Playlist) is asserted in + // Rust — see `search_scope_tests` in src-tauri/src/repository/types.rs. + // Asserting item types here would mean the frontend knows the taxonomy again, + // which is the leak docs/specs/scoped-search-boundary.md exists to prevent. + + it("sends the default (all) scope and never an item-type list", async () => { await library.search("office"); const options = searchMock.mock.calls[0][1]; + expect(options.scope).toBe("all"); expect(options).not.toHaveProperty("includeItemTypes"); expect(options.limit).toBe(10000); }); - it("forwards music item types when scoped to music", async () => { + it("sends the opaque scope when scoped to music", async () => { await library.search("office", "music"); - expect(searchMock.mock.calls[0][1].includeItemTypes).toEqual([ - "MusicAlbum", - "MusicArtist", - "Audio", - "Playlist", - ]); + const options = searchMock.mock.calls[0][1]; + expect(options.scope).toBe("music"); + expect(options).not.toHaveProperty("includeItemTypes"); }); - it("forwards tv item types when scoped to tv", async () => { + it("sends the opaque scope when scoped to tv", async () => { await library.search("office", "tv"); - expect(searchMock.mock.calls[0][1].includeItemTypes).toEqual(["Series", "Episode"]); + const options = searchMock.mock.calls[0][1]; + expect(options.scope).toBe("tv"); + expect(options).not.toHaveProperty("includeItemTypes"); }); - it("forwards movie item types when scoped to movies", async () => { + it("sends the opaque scope when scoped to movies", async () => { await library.search("office", "movies"); - expect(searchMock.mock.calls[0][1].includeItemTypes).toEqual(["Movie"]); + const options = searchMock.mock.calls[0][1]; + expect(options.scope).toBe("movies"); + expect(options).not.toHaveProperty("includeItemTypes"); }); it("stores results and the query on success", async () => { diff --git a/src/lib/utils/searchScope.test.ts b/src/lib/utils/searchScope.test.ts index 81e822a1..716a1f13 100644 --- a/src/lib/utils/searchScope.test.ts +++ b/src/lib/utils/searchScope.test.ts @@ -9,7 +9,6 @@ import { resolveSearchScope, searchRouteUrl, shouldNavigateToSearch, - scopeItemTypes, type SearchGroupId, } from "./searchScope"; @@ -62,25 +61,12 @@ describe("resolveSearchScope", () => { }); }); -describe("scopeItemTypes", () => { - it("omits the key entirely for the all scope", () => { - // `all` must send no includeItemTypes — an explicit union would silently - // drop types nobody enumerated (Person, folders). - expect(scopeItemTypes("all")).toBeUndefined(); - }); - - it("maps each narrow scope to its item types", () => { - expect(scopeItemTypes("music")).toEqual(["MusicAlbum", "MusicArtist", "Audio", "Playlist"]); - expect(scopeItemTypes("movies")).toEqual(["Movie"]); - expect(scopeItemTypes("tv")).toEqual(["Series", "Episode"]); - }); - - it("returns a fresh array callers cannot mutate into the table", () => { - const first = scopeItemTypes("movies")!; - first.push("Series"); - expect(scopeItemTypes("movies")).toEqual(["Movie"]); - }); -}); +// NOTE: the former `scopeItemTypes` suite moved to Rust — see +// `search_scope_tests` in src-tauri/src/repository/types.rs. The scope → +// item-type expansion is domain vocabulary and is no longer reachable from the +// frontend, so testing it here would mean re-introducing the leak to test it. +// The "fresh array" test is gone because `item_types()` returns an owned Vec, +// making the aliasing bug it guarded structurally impossible. describe("normalizeGroupOrder", () => { it("returns the default for missing or non-array input", () => { diff --git a/src/lib/utils/searchScope.ts b/src/lib/utils/searchScope.ts index 7bbc9164..b1c08270 100644 --- a/src/lib/utils/searchScope.ts +++ b/src/lib/utils/searchScope.ts @@ -8,7 +8,11 @@ // // TRACES: UR-049, UR-050 | DR-063, DR-066, DR-067 -export type SearchScope = "all" | "music" | "movies" | "tv"; +// Sourced from Rust via the generated bindings — the backend owns what a scope +// *means* (which Jellyfin item types it covers). Naming an opaque variant is +// presentation; knowing its expansion is domain vocabulary and stays in Rust. +export type { SearchScope } from "$lib/api/bindings"; +import type { SearchScope } from "$lib/api/bindings"; export const SEARCH_SCOPES: readonly SearchScope[] = ["all", "music", "movies", "tv"]; @@ -19,29 +23,11 @@ export const SCOPE_LABELS: Record = { tv: "TV", }; -/** - * Jellyfin item types requested for each scope. - * - * `all` is deliberately absent: sending no `includeItemTypes` is *not* the same - * as sending the union of the lists below — types nobody enumerated here - * (Person, folders, …) would be filtered out by an explicit list. - */ -const SCOPE_ITEM_TYPES: Record, string[]> = { - music: ["MusicAlbum", "MusicArtist", "Audio", "Playlist"], - movies: ["Movie"], - tv: ["Series", "Episode"], -}; - -/** - * Item types to send with a scoped search, or `undefined` for the `all` scope - * so the caller omits the key entirely. - * - * TRACES: UR-049 | DR-063 - */ -export function scopeItemTypes(scope: SearchScope): string[] | undefined { - if (scope === "all") return undefined; - return [...SCOPE_ITEM_TYPES[scope]]; -} +// NOTE: the scope → Jellyfin item-type mapping deliberately does NOT live here. +// It is domain vocabulary and lives in Rust (`SearchScope::item_types()` in +// repository/types.rs); the frontend sends the opaque scope and the backend +// expands it. Re-introducing a `{ music: ["MusicAlbum", …] }` table in this file +// is the boundary leak documented in docs/specs/scoped-search-boundary.md. /** * Resolve the scope a search started from a given route should default to.