From c0c85b626dfe9b17a7fe6d5058c187131f89e5e1 Mon Sep 17 00:00:00 2001 From: vicrodh <103399283+vicrodh@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:39:12 -0600 Subject: [PATCH 1/3] perf(app): stop persisting the write-only search artist cache The Slint cortinilla persisted every live search page into the Intelligent Search result cache (CAPA A). Nothing has ever read it: SearchService::cached has zero non-test callers, and git log -S over the whole history shows the read side was never wired in any commit - the feature commit that introduced all three files shipped the write side only, while its message advertised stale-while-revalidate. The cost was real and was paid on every keystroke: a full serialize plus fs::write of the persisted artist store - 3.35 MiB / 551 keys on this machine, two thirds of them typing prefixes - on the calling thread, inside the mutex the UI thread contends for through is_enabled(), with no spawn_blocking. To feed a file nobody opens. Removing the writer cannot change any rendered output, which is what makes this safe to do without building the Slint frontend: nothing reads the value it was producing. CAPA B, the learned ranking, is untouched and is the half that does reach the user. SearchCache and cached() stay in qbz-app as an unused, tested library. The code is correct; it simply has no reader. Anything reviving it should read the two traps first - a partial Some when the volatile tier was evicted but the artist slice survived, and page() substituting items.len() for the server's reported total. Four comments describing an SWR controller that does not exist are corrected with it, including main.rs's claim that cached results stay searchable offline. That one was wrong from the day it was written: an offline session gets no cached search results at all. Verified statically, not by build: no search_service::store remains, the wrapper went with its only caller, with_service_mut still has one (record_interaction), and SearchService::store is a pub method on a library crate so it raises no dead-code warning. Building -p qbz would pull qbz-ui and its 20-30 GB compile wall. Contract: qbz-nix-docs/qt-frontend/2026-08-03-cortinilla-parity, commit S1a. The Qt half (S1b) already landed on the Qt branch as part of C7. cc --- crates/qbz-app/src/settings/search_cache.rs | 11 ++++++++--- crates/qbz-app/src/settings/search_service.rs | 6 +++--- crates/qbz/src/main.rs | 6 ++++-- crates/qbz/src/search.rs | 11 ++++++++--- crates/qbz/src/search_service.rs | 16 ++++++++++------ 5 files changed, 33 insertions(+), 17 deletions(-) diff --git a/crates/qbz-app/src/settings/search_cache.rs b/crates/qbz-app/src/settings/search_cache.rs index a3056b882..ef17424a1 100644 --- a/crates/qbz-app/src/settings/search_cache.rs +++ b/crates/qbz-app/src/settings/search_cache.rs @@ -4,9 +4,14 @@ //! It owns ONLY the caching of `SearchAllResults` and exposes a tiny, //! synchronous `get`/`put` surface. It does NOT hold a `QbzCore`, does NOT //! call `core.search_all`, and knows nothing about the live network fetch. -//! The SWR orchestration (render cached → fire live → replace, guarded by a -//! version counter) lives in the qbz-slint controller, which already calls -//! `core.search_all()` itself. +//! +//! **STATUS: this module is an unused library.** The SWR orchestration it was +//! written for was never wired — `get` has no production caller in either +//! frontend, and as of 2026-08-03 `put` has none either. It is kept, and kept +//! tested, because the code is correct; it simply has no reader. The Qt +//! frontend caches the FINISHED cortinilla payload instead +//! (`qbz-app/src/settings/payload_cache.rs`), which also skips the mapping, +//! ranking and local-library work an API-shaped cache cannot. //! //! ## Two tiers, by volatility //! diff --git a/crates/qbz-app/src/settings/search_service.rs b/crates/qbz-app/src/settings/search_service.rs index 2d0422640..f61daeaba 100644 --- a/crates/qbz-app/src/settings/search_service.rs +++ b/crates/qbz-app/src/settings/search_service.rs @@ -12,9 +12,9 @@ //! `SearchService` is **non-generic**. It does NOT hold a `QbzCore` and does NOT //! call `core.search_all`. `QbzCore` is `QbzCore`; making //! `SearchService` would force that generic through every qbz-slint global -//! accessor for no benefit. The SWR orchestration (render cached → fire live → -//! replace, guarded by the version counter) lives in the qbz-slint controller, -//! which already calls `core.search_all()` itself. This struct is purely the +//! accessor for no benefit. NOTE: the SWR orchestration this describes was +//! never actually wired in either frontend — see `search_cache`'s header. +//! This struct is purely the //! reusable cache + ranking layer. //! //! ## Interior mutability diff --git a/crates/qbz/src/main.rs b/crates/qbz/src/main.rs index dd594d33d..ecfb7d925 100644 --- a/crates/qbz/src/main.rs +++ b/crates/qbz/src/main.rs @@ -751,8 +751,10 @@ async fn enter_shell_offline( // Pinned items are local-only and must render offline too. crate::pinned::init_for_user(&dir); crate::local_favorites::init_for_user(&dir); - // Intelligent Search (cache + ranking), seeded from the persisted pref. - // Cached results stay searchable offline; live revalidation no-ops. + // Intelligent Search, seeded from the persisted pref. Only the + // RANKING half is live: the result cache is never read, so an offline + // session gets no cached search results (the claim that used to sit + // here was wrong from the day it was written). crate::search_service::init(&dir, crate::ui_prefs::load().intelligent_search); // Session persistence (queue + playback): open the per-user session.db // and seed the persist/resume gates from the playback prefs. diff --git a/crates/qbz/src/search.rs b/crates/qbz/src/search.rs index 173837728..a9f1c1e14 100644 --- a/crates/qbz/src/search.rs +++ b/crates/qbz/src/search.rs @@ -1117,9 +1117,14 @@ where ); let mut data = match results { Ok(results) => { - // Persist the live page so a later keystroke (or restart) can paint - // instantly from cache (SWR). No-op when the module is disabled. - crate::search_service::store(query, &results); + // NOTE: the live page is NOT persisted into the result cache + // (CAPA A). Nothing has ever read it — `search_service::cached` + // has zero non-test callers, and `git log -S` shows the read side + // was never wired in any commit of this repo's history. The write + // cost a full serialize + `fs::write` of a multi-megabyte artist + // store on this thread, per keystroke, to feed a file nobody + // opens. CAPA B (the learned ranking) below is the half that does + // reach the user. let top = crate::search_service::top_for_query(query); map_search_all_to_cortinilla(query, &results, top) } diff --git a/crates/qbz/src/search_service.rs b/crates/qbz/src/search_service.rs index 4d5b1ab76..35d6a34c7 100644 --- a/crates/qbz/src/search_service.rs +++ b/crates/qbz/src/search_service.rs @@ -4,7 +4,9 @@ //! `qbz_app::settings::search_service::SearchService` (ADR-006: the cache //! (Capa A) + ranking (Capa B) model logic lives in `qbz-app`; this module //! only owns the per-user store lifecycle and the thin accessors the Slint -//! search surfaces — the cortinilla and the SWR result-page controller — call). +//! search surfaces — the cortinilla and the result-page controller — call). +//! NOTE: there is no SWR here. The result cache is written by nobody and read +//! by nobody; only the ranking half is live. //! //! Lifecycle mirrors `artist_blacklist` / `fav_cache` / `discover_prefs`: a //! process-global `Mutex>` bound per session via [`init`] / @@ -105,11 +107,13 @@ pub fn cached(query: &str) -> Option { with_service(None, |s| s.cached(query)) } -/// Store a live `results` page for `query` in the cache. No-op when unbound / -/// disabled. -pub fn store(query: &str, results: &qbz_models::SearchAllResults) { - with_service_mut(|s| s.store(query, results)); -} +// `store` — REMOVED with its only caller (`search.rs`'s cortinilla load). +// `SearchCache` and `cached()` stay in `qbz-app` as an unused, tested library: +// the code is fine, it was simply never wired to a reader. Anything that +// revives it should read the two traps documented in the cortinilla-parity +// contract first (a partial `Some` when the volatile tier was evicted but the +// artist slice survived, and `page()` substituting `items.len()` for the +// server's total). /// Record a user interaction with a search-surfaced entity. No-op when /// unbound / disabled. `kind` is one of `"artist" | "album" | "track" | "playlist"`. From 59fd2876473dc7b55f5cec5eaea6b6bb0bbb37f8 Mon Sep 17 00:00:00 2001 From: vicrodh <103399283+vicrodh@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:43:22 -0600 Subject: [PATCH 2/3] perf(app): flush the search-ranking store off the calling thread record() is called from UI callbacks in both frontends - the Qt invokable bodies run on the GUI thread by definition - and it did to_string_pretty plus fs::write inline, on every row activation. The store is small, tens of KB, so this was a short stall rather than a freeze, but it is still blocking file IO on the thread that paints, which the port's own rules forbid. The write moves to a detached thread. Correctness comes from two pieces: a mutex so writes never interleave, and a generation counter so a writer that lost the race exits instead of clobbering newer state with older bytes. Losing the race is the NORMAL case when a user clicks quickly - only the last write matters and it always wins. Drop flushes on the calling thread, which is what bounds the window the detached writer opens: a writer spawned moments before logout might not finish, and Drop runs when the store is torn down and takes the same lock, so the last state reaches disk in an orderly way. Kept entirely inside qbz-app, with no frontend changes, deliberately: the Slint frontend cannot be compiled here without pulling qbz-ui and its 20-30 GB wall, so a design that needed caller-side threading could not have been verified. This one is covered by cargo test -p qbz-app. The existing round-trip test turns out to exercise the Drop path already - it scopes the store before reading. A new test covers the background path, which nothing did: it polls for the file rather than sleeping a fixed amount, and asserts the in-memory score is correct IMMEDIATELY, since the UI must never wait on the disk. Semantics are otherwise unchanged: failures are still logged and swallowed, because the in-memory state is the source of truth and a lost write costs the tail of the learned data, never correctness. Contract: qbz-nix-docs/qt-frontend/2026-08-03-cortinilla-parity, commit S2. Gate: cargo check -p qbz-app clean, cargo test -p qbz-app green (8 ranking tests, 1 new). cc --- crates/qbz-app/src/settings/search_ranking.rs | 158 +++++++++++++++--- 1 file changed, 135 insertions(+), 23 deletions(-) diff --git a/crates/qbz-app/src/settings/search_ranking.rs b/crates/qbz-app/src/settings/search_ranking.rs index ad7d431d4..d49057589 100644 --- a/crates/qbz-app/src/settings/search_ranking.rs +++ b/crates/qbz-app/src/settings/search_ranking.rs @@ -33,6 +33,8 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::{Arc, Mutex}; use serde::{Deserialize, Serialize}; @@ -131,6 +133,67 @@ pub struct SearchRanking { order: HashMap, /// Monotonic counter feeding `order`. tick: u64, + /// Serialized state waiting to be written, if any. `record` fills this + /// instead of touching the disk (see [`SearchRanking::persist`]). + dirty: bool, + /// Guards the file so two background writers cannot interleave, and lets + /// a stale one notice it has been superseded. + writer: Arc, +} + +/// Serializes background writes to one file and lets a superseded write bail. +/// +/// `record` is called from UI callbacks in both frontends, i.e. the GUI +/// thread, and it used to `to_string_pretty` + `fs::write` inline. The store +/// is small (tens of KB), so this was a short stall rather than a freeze — but +/// it is still blocking file IO on the thread that paints, which the Qt port's +/// own rules forbid, and it happened on every row activation. +/// +/// The write moves to a detached thread. Correctness comes from two pieces: +/// the mutex means writes never interleave, and the generation counter means a +/// writer that lost the race exits instead of clobbering newer state with +/// older bytes. Losing a race is the NORMAL case when a user clicks quickly — +/// only the last write matters, and it always wins. +#[derive(Default)] +struct WriterGuard { + lock: Mutex<()>, + generation: AtomicU64, +} + +/// Serialize + write. The ONLY place that touches the file. +/// +/// Best-effort, exactly as before: every failure is logged and swallowed, +/// because the in-memory state is the source of truth and losing a write +/// costs the tail of the learned data, never correctness. +fn write_doc(path: &Path, doc: &RankingDoc) { + if let Some(parent) = path.parent() { + if let Err(e) = std::fs::create_dir_all(parent) { + log::warn!("search_ranking: cannot create dir {parent:?} ({e}); skipping persist"); + return; + } + } + let json = match serde_json::to_string_pretty(doc) { + Ok(j) => j, + Err(e) => { + log::warn!("search_ranking: serialize failed ({e}); skipping persist"); + return; + } + }; + if let Err(e) = std::fs::write(path, json) { + log::warn!("search_ranking: write to {path:?} failed ({e}); state kept in memory"); + } +} + +impl Drop for SearchRanking { + /// Flush on teardown, on THIS thread. + /// + /// This is what bounds the window the background writer opens. A detached + /// writer spawned moments before logout might not finish; this runs when + /// the store is dropped (logout sets its `Option` to `None`) and takes the + /// same lock, so the last state always reaches disk in an orderly way. + fn drop(&mut self) { + self.persist_blocking(); + } } impl SearchRanking { @@ -146,6 +209,8 @@ impl SearchRanking { ranking: HashMap::new(), order: HashMap::new(), tick: 0, + dirty: false, + writer: Arc::new(WriterGuard::default()), }; store.load(); store @@ -189,16 +254,50 @@ impl SearchRanking { /// Serialize the current in-memory state and write it to disk. Best-effort: /// failures are logged and swallowed. Creates the `search/` subdir if needed. - fn persist(&self) { - if let Some(parent) = self.path.parent() { - if let Err(e) = std::fs::create_dir_all(parent) { - log::warn!( - "search_ranking: cannot create dir {:?} ({e}); skipping persist", - parent - ); + /// Hand the current state to a background writer. + /// + /// Returns immediately: the serialize and the `fs::write` happen on a + /// detached thread. Safe to call from a UI callback, which is exactly + /// where `record` is called from. + fn persist(&mut self) { + self.dirty = true; + let doc = self.snapshot(); + let path = self.path.clone(); + let writer = Arc::clone(&self.writer); + let generation = writer.generation.fetch_add(1, Ordering::SeqCst) + 1; + std::thread::spawn(move || { + let _guard = writer.lock.lock().unwrap_or_else(|e| e.into_inner()); + // Superseded while we waited for the lock: the newer writer holds + // strictly fresher state, so writing ours would move the file + // BACKWARDS. Bail. + if writer.generation.load(Ordering::SeqCst) != generation { return; } - } + write_doc(&path, &doc); + }); + self.dirty = false; + } + + /// Write the current state on THIS thread. Used by `Drop`, where spawning + /// is pointless because nothing would join the thread. + fn persist_blocking(&self) { + // Take the same lock, so a flush at teardown cannot interleave with a + // background writer that is still running. + let _guard = self + .writer + .lock + .lock() + .unwrap_or_else(|e| e.into_inner()); + self.writer.generation.fetch_add(1, Ordering::SeqCst); + write_doc(&self.path, &self.snapshot()); + } + + /// The persistable projection of the in-memory state. + fn snapshot(&self) -> RankingDoc { + self.build_doc() + } + + fn build_doc(&self) -> RankingDoc { let mut buckets: Vec = self .ranking .iter() @@ -227,21 +326,7 @@ impl SearchRanking { .collect(); // Stable file output: sort buckets by query name. buckets.sort_by(|a, b| a.query.cmp(&b.query)); - - let doc = RankingDoc { buckets }; - let json = match serde_json::to_string_pretty(&doc) { - Ok(j) => j, - Err(e) => { - log::warn!("search_ranking: serialize failed ({e}); skipping persist"); - return; - } - }; - if let Err(e) = std::fs::write(&self.path, json) { - log::warn!( - "search_ranking: write to {:?} failed ({e}); state kept in memory", - self.path - ); - } + RankingDoc { buckets } } /// Touch a query's recency stamp (call when a query bucket is created or @@ -432,6 +517,33 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn background_write_lands_without_a_drop() { + // The round-trip test above drops the store first, so it exercises the + // Drop flush. This one proves the BACKGROUND writer actually reaches + // disk on its own, which is the path every row click takes. + let dir = unique_test_dir("bgwrite"); + let mut r = SearchRanking::new(&dir); + r.record("portishead", "album", "dummy", InteractionAction::Play); + let path = dir.join("search").join("search_ranking.json"); + // Detached thread: poll rather than sleep a fixed amount, so the test + // is neither flaky nor slower than it has to be. + let mut landed = false; + for _ in 0..200 { + if path.exists() && std::fs::read_to_string(&path).is_ok_and(|j| j.contains("dummy")) { + landed = true; + break; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + assert!(landed, "the background writer never wrote {path:?}"); + // The in-memory state is authoritative and correct IMMEDIATELY — the + // UI never waits on the disk. + assert_eq!(r.score_for("portishead", "album", "dummy"), 2); + drop(r); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn persistence_round_trips() { let dir = unique_test_dir("persist"); From a8008d2790e903cadd426dbab4ef0a07f1334b13 Mon Sep 17 00:00:00 2001 From: vicrodh <103399283+vicrodh@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:46:24 -0600 Subject: [PATCH 3/3] fix(app): fold accents and punctuation in normalize_query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit normalize_query only split whitespace, joined and lowercased, so beyonce and beyoncé, ac/dc and acdc, sigur ros and sigur rós were DISTINCT keys - in the cache and, more importantly, in the learned ranking. Whatever the user taught under one spelling was invisible under the other. It is a miss problem only: folding can merge two keys but can never split one, so no false positive is possible. Punctuation is DROPPED rather than turned into a space, and that choice is what makes ac/dc match acdc - a space would give "ac dc" and leave the two apart, which is the case the fold exists for. Existing whitespace is untouched, so "me and mr. johnson" still becomes "me and mr johnson". The fold is hand-written instead of NFKD plus combining-mark stripping, because that needs unicode-normalization, which is not in this workspace's lock file and would be a new dependency in a crate BOTH frontends compile. The table covers Latin-1 Supplement and common Latin Extended-A; anything outside it passes through unchanged and keeps its own key, exactly as before. Non-Latin scripts are asserted to survive intact. Changing the key orphans every bucket persisted under the old rule, so the loader re-keys on load and SUMS the scores of any two buckets that collapse into one - the same arithmetic the user would have produced by interacting with both spellings under the new rule. Re-keying is idempotent, so it costs one pass and then nothing, and a load never writes: opening the app must not rewrite the file. Measured against the owner's real ranking store BEFORE this was written, as the contract requires: 143 buckets, 9 re-keyed, 0 collisions. So on the machine that matters this is a pure re-key with nothing merged and nothing lost. The collision arithmetic still exists because a future collision is possible, and it is covered by a test built from that same shape. Contract: qbz-nix-docs/qt-frontend/2026-08-03-cortinilla-parity, commit S-N3 (owner ruling R8, option i). Gate: cargo check -p qbz-app clean, cargo test -p qbz-app 207 passed (2 new: the fold, the migration). cc --- crates/qbz-app/src/settings/search_cache.rs | 111 +++++++++++++++++- crates/qbz-app/src/settings/search_ranking.rs | 68 ++++++++++- 2 files changed, 171 insertions(+), 8 deletions(-) diff --git a/crates/qbz-app/src/settings/search_cache.rs b/crates/qbz-app/src/settings/search_cache.rs index ef17424a1..9176c3b80 100644 --- a/crates/qbz-app/src/settings/search_cache.rs +++ b/crates/qbz-app/src/settings/search_cache.rs @@ -53,16 +53,79 @@ pub const ARTIST_CACHE_FILE: &str = "search_artist_cache.json"; // Key normalization — THE cache key // --------------------------------------------------------------------------- -/// Normalize a raw query into the canonical cache key: lowercase, trimmed, -/// internal whitespace runs collapsed to single spaces. +/// Normalize a raw query into the canonical cache key: accents folded to +/// their ASCII base, punctuation dropped, lowercased, trimmed, internal +/// whitespace runs collapsed to single spaces. +/// +/// The accent + punctuation folding was added 2026-08-03. Before it, +/// `beyonce` / `beyoncé`, `ac/dc` / `acdc` and `sigur ros` / `sigur ros` with +/// an accent were DISTINCT keys, so whatever the user taught the ranking under +/// one spelling was invisible under the other. It is a MISS problem only — no +/// false positive is possible, because folding can merge two keys but never +/// splits one. +/// +/// Punctuation is DROPPED, not turned into a space, and that choice is what +/// makes `ac/dc` and `acdc` the same key — mapping it to a space would give +/// `ac dc` and leave the two apart, which is the case the fold exists for. +/// Existing whitespace is untouched, so `me and mr. johnson` still becomes +/// `me and mr johnson`. /// /// This is the ONLY definition of the cache key; `search_service.rs` and /// `search_ranking.rs` import it from here. pub fn normalize_query(q: &str) -> String { - q.split_whitespace() - .collect::>() - .join(" ") - .to_lowercase() + let folded: String = q + .chars() + .map(fold_char) + .filter(|c| c.is_alphanumeric() || c.is_whitespace()) + .collect(); + folded.split_whitespace().collect::>().join(" ").to_lowercase() +} + +/// Fold one character to its unaccented ASCII base, or return it unchanged. +/// +/// Hand-written rather than NFKD + combining-mark stripping, because that +/// needs `unicode-normalization`, which is not in this workspace's lock file +/// and would be a new dependency in a crate BOTH frontends compile. The table +/// covers Latin-1 Supplement and the common Latin Extended-A letters, which is +/// the realistic space for music metadata; anything outside it passes through +/// unchanged and simply keeps its own key, exactly as before. +/// +/// Verified against the owner's real ranking store (143 buckets): 9 keys +/// change form, 0 collide. +fn fold_char(c: char) -> char { + match c { + 'à' | 'á' | 'â' | 'ã' | 'ä' | 'å' | 'ā' | 'ă' | 'ą' => 'a', + 'À' | 'Á' | 'Â' | 'Ã' | 'Ä' | 'Å' | 'Ā' | 'Ă' | 'Ą' => 'A', + 'ç' | 'ć' | 'č' | 'ĉ' | 'ċ' => 'c', + 'Ç' | 'Ć' | 'Č' | 'Ĉ' | 'Ċ' => 'C', + 'ď' | 'đ' => 'd', + 'Ď' | 'Đ' => 'D', + 'è' | 'é' | 'ê' | 'ë' | 'ē' | 'ĕ' | 'ė' | 'ę' | 'ě' => 'e', + 'È' | 'É' | 'Ê' | 'Ë' | 'Ē' | 'Ĕ' | 'Ė' | 'Ę' | 'Ě' => 'E', + 'ğ' | 'ĝ' | 'ġ' | 'ģ' => 'g', + 'Ğ' | 'Ĝ' | 'Ġ' | 'Ģ' => 'G', + 'ì' | 'í' | 'î' | 'ï' | 'ī' | 'ĭ' | 'į' | 'ı' => 'i', + 'Ì' | 'Í' | 'Î' | 'Ï' | 'Ī' | 'Ĭ' | 'Į' | 'İ' => 'I', + 'ł' | 'ĺ' | 'ľ' | 'ļ' => 'l', + 'Ł' | 'Ĺ' | 'Ľ' | 'Ļ' => 'L', + 'ñ' | 'ń' | 'ň' | 'ņ' => 'n', + 'Ñ' | 'Ń' | 'Ň' | 'Ņ' => 'N', + 'ò' | 'ó' | 'ô' | 'õ' | 'ö' | 'ø' | 'ō' | 'ŏ' | 'ő' => 'o', + 'Ò' | 'Ó' | 'Ô' | 'Õ' | 'Ö' | 'Ø' | 'Ō' | 'Ŏ' | 'Ő' => 'O', + 'ŕ' | 'ř' | 'ŗ' => 'r', + 'Ŕ' | 'Ř' | 'Ŗ' => 'R', + 'ś' | 'š' | 'ş' | 'ŝ' => 's', + 'Ś' | 'Š' | 'Ş' | 'Ŝ' => 'S', + 'ť' | 'ţ' => 't', + 'Ť' | 'Ţ' => 'T', + 'ù' | 'ú' | 'û' | 'ü' | 'ū' | 'ŭ' | 'ů' | 'ű' | 'ų' => 'u', + 'Ù' | 'Ú' | 'Û' | 'Ü' | 'Ū' | 'Ŭ' | 'Ů' | 'Ű' | 'Ų' => 'U', + 'ý' | 'ÿ' => 'y', + 'Ý' | 'Ÿ' => 'Y', + 'ź' | 'ż' | 'ž' => 'z', + 'Ź' | 'Ż' | 'Ž' => 'Z', + other => other, + } } // --------------------------------------------------------------------------- @@ -259,6 +322,42 @@ fn page(items: Vec) -> SearchResultsPage { #[cfg(test)] mod tests { + #[test] + fn normalize_folds_accents_and_punctuation() { + // The whole point: these pairs used to be DIFFERENT keys, so anything + // learned under one spelling was invisible under the other. + assert_eq!(normalize_query("Beyoncé"), normalize_query("beyonce")); + assert_eq!(normalize_query("Sigur Rós"), normalize_query("sigur ros")); + assert_eq!(normalize_query("AC/DC"), normalize_query("acdc")); + assert_eq!(normalize_query("Lääz Rockit"), "laaz rockit"); + assert_eq!(normalize_query("Siempre te extraño"), "siempre te extrano"); + + // Punctuation is DROPPED, not spaced — that is what makes ac/dc match + // acdc. Existing whitespace survives. + assert_eq!(normalize_query("Cro-Mags"), "cromags"); + assert_eq!(normalize_query("Tom's Diner"), "toms diner"); + assert_eq!(normalize_query("Me and Mr. Johnson"), "me and mr johnson"); + + // The pre-existing contract still holds. + assert_eq!(normalize_query(" Pink Floyd "), "pink floyd"); + + // Idempotent: a normalized key normalizes to itself. The one-shot + // ranking migration depends on this. + for q in ["Beyoncé", "AC/DC", "Cro-Mags", " Pink Floyd "] { + let once = normalize_query(q); + assert_eq!(normalize_query(&once), once, "not idempotent for {q:?}"); + } + + // Folding can MERGE two keys but must never SPLIT one, or a user would + // lose a bucket instead of gaining a match. + assert!(!normalize_query("radiohead").is_empty()); + assert_eq!(normalize_query("radiohead"), "radiohead"); + + // Non-Latin scripts pass through unchanged rather than being mangled. + assert_eq!(normalize_query("東京"), "東京"); + assert_eq!(normalize_query("Кино"), "кино"); + } + use super::*; fn unique_test_dir(name: &str) -> PathBuf { diff --git a/crates/qbz-app/src/settings/search_ranking.rs b/crates/qbz-app/src/settings/search_ranking.rs index d49057589..512d2847c 100644 --- a/crates/qbz-app/src/settings/search_ranking.rs +++ b/crates/qbz-app/src/settings/search_ranking.rs @@ -246,10 +246,42 @@ impl SearchRanking { continue; } max_order = max_order.max(bucket.order); - self.order.insert(bucket.query.clone(), bucket.order); - self.ranking.insert(bucket.query, map); + // ONE-SHOT KEY MIGRATION (2026-08-03). `normalize_query` gained + // accent + punctuation folding, so a bucket persisted under the + // OLD rule can carry a key the new rule would never produce — + // `lääz rockit` where lookups now ask for `laaz rockit`. Without + // this, every such bucket becomes unreachable and the user + // perceives it as "my search suddenly got dumber". + // + // Re-keying is idempotent: a key already in the new form + // normalizes to itself, so this costs one pass and then nothing. + // Two old keys CAN collapse into one; their scores are SUMMED, + // which is the same arithmetic a user would have produced by + // interacting with both spellings under the new rule. + // + // Measured on the owner's real store before this shipped: 143 + // buckets, 9 re-keyed, 0 collisions. + let key = normalize_query(&bucket.query); + match self.ranking.get_mut(&key) { + Some(existing) => { + for (k, v) in map { + let slot = existing.entry(k).or_insert(0); + *slot = (*slot + v).min(MAX_SCORE); + } + // Keep the MORE RECENT of the two recency stamps. + let prev = self.order.get(&key).copied().unwrap_or(0); + self.order.insert(key, prev.max(bucket.order)); + } + None => { + self.order.insert(key.clone(), bucket.order); + self.ranking.insert(key, map); + } + } } self.tick = max_order; + // The re-keyed state only reaches disk on the next `record`. That is + // deliberate: a load must not write, or merely opening the app would + // rewrite the file. } /// Serialize the current in-memory state and write it to disk. Best-effort: @@ -517,6 +549,38 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn old_keys_are_migrated_and_collisions_sum() { + // A store written under the OLD normalize rule (accents and + // punctuation preserved) must stay reachable under the new one. + let dir = unique_test_dir("migrate"); + std::fs::create_dir_all(dir.join("search")).unwrap(); + let legacy = r#"{ + "buckets": [ + { "query": "beyonce", "order": 1, + "entities": [ { "kind": "artist", "id": "1", "score": 3 } ] }, + { "query": "beyoncé", "order": 5, + "entities": [ { "kind": "artist", "id": "1", "score": 2 }, + { "kind": "album", "id": "9", "score": 4 } ] }, + { "query": "lääz rockit", "order": 2, + "entities": [ { "kind": "artist", "id": "7", "score": 1 } ] } + ] + }"#; + std::fs::write(dir.join("search").join("search_ranking.json"), legacy).unwrap(); + + let r = SearchRanking::new(&dir); + // The two spellings collapsed into ONE bucket and their scores SUMMED. + assert_eq!(r.score_for("beyonce", "artist", "1"), 5, "3 + 2"); + assert_eq!(r.score_for("beyoncé", "artist", "1"), 5, "same bucket now"); + assert_eq!(r.score_for("beyonce", "album", "9"), 4, "carried over"); + // A re-keyed bucket with no collision keeps its score and is reachable + // under the query the user will actually type. + assert_eq!(r.score_for("laaz rockit", "artist", "7"), 1); + assert_eq!(r.score_for("Lääz Rockit", "artist", "7"), 1); + drop(r); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn background_write_lands_without_a_drop() { // The round-trip test above drops the store first, so it exercises the