diff options
Diffstat (limited to 'docs/refactoring-search.md')
| -rw-r--r-- | docs/refactoring-search.md | 376 |
1 files changed, 376 insertions, 0 deletions
diff --git a/docs/refactoring-search.md b/docs/refactoring-search.md new file mode 100644 index 0000000..08774d1 --- /dev/null +++ b/docs/refactoring-search.md @@ -0,0 +1,376 @@ +# Refactor: one file, one entry — merging duplicate sources in the Search view + +> Status: **planned, not built** (2026-09-02). Branch `feat/search-source-merge`. +> Scope: the cross-group Search page (`static/search-page.js`) and the three +> media applications it reuses (Videos, Music, Photos). The Files **explorer** +> inside Search is explicitly out of scope and must not change. +> +> **The bug, in one sentence:** a file shared by two groups is two entries in the +> Search view, so a film shows twice in the poster grid, an episode twice in a +> show's list, and a track twice in an album. +> +> The convention from draft-v6 is carried forward: **a claim in this document +> must name the adversary it holds against.** + +--- + +## 1. What was observed + +A single node hosts two groups, `demo35` and `media`. Both were given the *same* +directory as their video root — that is the whole point of having two groups: +different people are invited to different libraries, and one library may be in +several of them. + +Everything works per group. In **Search files**, which walks every group the +account belongs to and merges their indexes into one view, every file of that +shared directory is listed twice: + +| View | Symptom | +|---|---| +| Videos — Posters | Two identical cards for the same film, one badged `demo35`, one badged `media` | +| Videos — Flat list | Same, and a show folder that expands to each episode twice | +| Videos — detail modal | The season/episode list under the synopsis lists every episode twice | +| Music (not reported, same by construction) | Every track twice inside one album | +| Photos (not reported, same by construction) | Every photo twice inside one album | + +**Files (the explorer) is not affected and must stay that way.** There, each +group is a top-level folder and the two copies live in two different folders — +which is correct and is how a member navigates *per group*. Confirmed in +`search-page.js`'s `fileEntries`, which prefixes every path with the group name +precisely so that navigation works. + +**Within a single group this cannot happen.** `GroupIndex` is keyed by blake3 +(`group_index.py`, `_entries: dict # id → IndexEntry`), so the same bytes at two +paths inside one group are already one entry — the lesson recorded in `CLAUDE.md` +("a content-addressed index cannot represent the same bytes at two paths"). The +duplication is created by the Search page, which concatenates *N* independently +keyed indexes into one list, and by nothing else. + +--- + +## 2. Decision + +1. **Identity is the content hash.** `IndexEntry.id` is blake3 of the file + (`protocol.py:182`). Two entries with the same `id` are the same file, whatever + group announced them, whatever their path. +2. In the three media views, entries sharing an `id` are **merged into one + entry** carrying a list of sources. +3. **One source is chosen per logical unit**, not per file — a movie, a whole + show, a whole album, a whole photo album. Streaming, thumbnails, TMDB / + MusicBrainz metadata and the download link all use that one source. +4. The choice is: **a group hosted by the local node wins**; otherwise a + deterministic pseudo-random pick, stable for one user, spread across users. +5. If the chosen source turns out to be unreachable, the unit **fails over** to + another source that has the file. +6. The badge that today names the group becomes: the group name when there is + exactly one source, `N sources` when there is more than one. **Which** source + was picked is never shown. +7. **The Files explorer is untouched.** Not "mostly untouched" — the merge code + is never called on that path. + +--- + +## 3. Answers to the questions this raised + +Recorded here because each of them changed the plan. + +### 3.1 Merge across nodes, or only within one node? + +**Decided: across all groups**, whatever node hosts them. Merging only groups +that share a `node_id` would fix the reported case with no trust question at all +(same process, same file on disk), but it would deliver nothing else: two +different operators hosting the same film would stay two entries, and the +failover in §2.5 would have nothing to fail over to. + +**The adversary this names.** Chunks are encrypted and authenticated with the +group's GEK (`crypto.js` `deriveChunkKey(gek, fileHashHex, chunkIndex)`), and the +client does **not** re-hash the plaintext against `file_id`. So the GCM tag proves +"encrypted by someone holding this group's GEK for this file id", not "these bytes +hash to this id". After the merge, opening a file the Search view shows may fetch +bytes from a group the reader did not name. + +Bounded by three things, which is why it is accepted rather than blocking: + +- only groups the reader is **already a member of** are ever candidates — the + Search page indexes nothing else; +- an operator of such a group can already serve that reader arbitrary content + *inside their own group*, so no new capability is granted, only a new occasion + to use it; +- the local node wins whenever it is a candidate, which is the reported case and + the common one. + +Not accepted silently: the merged entry shows `N sources`, so a reader can see +that more than one group is involved. Re-hashing the plaintext client-side would +close it properly and is **not** proposed here — blake3 is not in WebCrypto, and +a streamed film is exactly the case where it cannot be done before playback. +Recorded in §9 as still open. + +### 3.2 What does "random" mean, concretely? + +**Decided: deterministic per user.** `Math.random()` re-evaluated during a render +would flip the source mid-stream and re-fetch every thumbnail on each re-render; +re-evaluated once per session it changes on every reload, and any index refetch +has to be careful to preserve it. + +The pick is `sourceIndex = hash(unitKey + userId) % sources.length` over the +sources sorted by group id. Stable for one reader across renders and reloads; +different readers land on different sources, which is what "random" was for. + +### 3.3 What if the chosen source is unreachable? + +**Decided: fail over.** A group whose index could not be fetched at all +contributes no entries and is already excluded (`fetchAllIndexes`'s `unreachable`). +What is new is a group that indexed fine and whose WebRTC connection later fails: +the pick must skip it and the unit must re-resolve. Without this, merging could +make a file *less* available than it is today, which would be a regression +dressed as a feature. + +### 3.4 Which views? + +Videos, Music and Photos. **Not** Files: the explorer stays navigable per group, +as it is today. + +--- + +## 4. Where the duplication actually comes from + +`search-page.js` builds four independent entry lists. Each walks +`indexedGroups` (a `Map` of groupId → `{entries, roots, groupName, groupOwner}`), +filters by that group's own root, and pushes a *copy* of the entry annotated with +its group's connection: + +```js +result.push({ + ...e, + path: SEARCH_VIDEO_ROOT + '/' + e.path, + groupId, groupName, groupOwner, + _tRef: conn ? conn.tRef : null, // which transport fetches this file + _gRef: conn ? conn.gRef : null, // which GEK decrypts it + _connGen: conn ? conn.gen : 0, // refetch key when that transport reconnects +}); +``` + +Everything downstream reads the source off the entry and nothing else: + +| Consumer | Reads | +|---|---| +| `MediaThumb`, `PosterCard`, `FlatMovieRow` | `_tRef` / `_gRef` / `_connGen` | +| `useMediaMeta` (TMDB), `useMusicMeta` | `_tRef`, `entry.id` | +| `VideoPlayer` (streaming) | the modal's transport, set from `connectGroup(entry.groupId)` | +| `music-player.js` | `getConnection(entry.groupId)` (`music-player.js:288`) | +| `downloadEntry` | the modal's transport, same origin | + +**This is the good news, and it decides the shape of the fix.** "The source" is +already one triple of fields on one entry. Producing *one* merged entry with one +source is therefore the whole change on the consumer side — the players, the +downloader and the metadata hooks need no modification at all. + +--- + +## 5. The shape of the fix + +### 5.1 A new module, `static/source-merge.js` + +Pure functions, no Preact, no transport — testable by reading them out of the +file and running them under node, the idiom `test_video_default_season.py` +already uses. + +``` +mergeUnitEntries(units, opts) → { entries, unitSourceByKey } +pickSource(sources, unitKey, salt, isDown) → source +sourceLabel(entry) → { text, groupId | null } +``` + +- `units` is a list of `{ key, entries }`. **The unit lists are produced by the + applications' own grouping functions**, never by a second copy of them (§5.2). +- `opts` carries `salt` (the user id), `isLocal(groupId)` and `isDown(groupId)`. +- Each output entry is one merged `IndexEntry` plus `_sources` (every group that + has it, sorted by group id) and the resolved `groupId` / `_tRef` / `_gRef` / + `_connGen` of its effective source. + +**Field provenance rule: every displayed field comes from the chosen source's +entry, and no field is back-filled from another source.** A `thumb_hash` or a +`display_title` that only one node computed is only fetchable over *that* node's +connection, so borrowing it would produce a poster request the chosen transport +cannot answer. Stated here because "merge two records field by field" is the +obvious thing to write and it is wrong. + +### 5.2 Units come from the real grouping functions + +The unit key must agree with how each application groups, or a show would get one +source and its episodes another. Rather than re-deriving the keys in +`search-page.js` — a copy that keeps passing after the original changes, the trap +`test_video_default_season.py`'s docstring names — the page calls the exported +grouping functions on the **un-merged** list purely to learn the units, merges +within each unit, and hands the merged flat list to the application, which groups +it again exactly as it does today. + +| View | Grouping function | Unit key | +|---|---|---| +| Videos | `groupVideoEntries` (exported) | show: `show:<title>`; movie: `movie:<id>` | +| Music | `groupMusicEntries` (exported) | album: `album:<artist>/<album>`; loose track: `track:<id>` | +| Photos | `groupPhotoAlbums` (**must be exported**, `photos-app.js:29`) | `album:<dir>` | + +Grouping therefore runs twice per recompute. It is a linear pass over an index +already held in memory and already re-run on every keystroke of the filter; the +cost is not worth a duplicated implementation. + +**Consequence to accept for Photos.** Photo albums are keyed by directory. Two +groups whose roots have different basenames put the same photo in two +differently-named albums, and the merge — scoped to a unit — will leave it in +both. That is correct: they *are* two albums. Only same-named albums collapse, +which is the reported shape. + +**Consequence to accept for Videos.** `PosterGrid.mergedShows` merges two +differently-parsed show titles once both resolve to the same TMDB id +(`video-app.js:845`). That happens after metadata arrives, inside the component, +and the two constituents may hold different chosen sources. Left alone: they were +two units when the source was picked, the episode lists are already disjoint, and +re-picking a source under a card the reader is looking at is worse than a mixed +one. + +### 5.3 Choosing the source + +``` +candidates = sources of every file in the unit, minus the ones marked down +local = candidates whose group is hosted by the local node +pool = local.length ? local : candidates +chosen = pool[ hash(unitKey + salt) % pool.length ] // pool sorted by group id +``` + +Then per file in the unit: use `chosen` if that file has it, otherwise re-run the +same rule over that file's own sources. An episode present in only one of the two +groups still plays. + +**"Hosted by the local node" is read from the handshake, not from the hub.** +`handshake_ack` already carries `is_node_admin`, computed by the node from its own +record of who it belongs to and never from a hub claim +(`webrtc_server.py:3916`). `fetchGroupIndex` has the ack in hand and today keeps +only the three root fields from it; it will keep `is_node_admin` too. A hub that +lied about it would only change which of the reader's own groups is preferred, and +the reader is a member of all of them. + +This is a proxy, not the literal question: it says "the operator of the node +serving this group is me", which for a person browsing their own libraries is the +same set. A browser has no other way to know — only the desktop client reaches +the daemon's loopback API. If the node id is wanted later, `fetchGroupIndex` +already knows which one it connected to and can record it at no cost. + +### 5.4 Failover + +`ConnectionPool` gains nothing; `search-page.js` gains a `downGroups` set: + +- `connectGroup(groupId)` records a failure and bumps `connectionGen`, which is + already the signal every entry list recomputes on; +- a later successful connect clears the mark; +- `pickSource` skips marked groups, and falls back to the full candidate list if + every one of them is marked (better a broken tile than a vanished film). + +Two properties this must have and must be tested for: a unit whose chosen source +goes down **re-resolves to another source without a page reload**, and a unit with +one source behaves exactly as it does today. + +### 5.5 The badge + +One helper, `sourceLabel(entry)`, four call sites: + +| File | Line today | Today | After | +|---|---|---|---| +| `video-app.js` | 405 | `repEntry.groupName` | group name, or `N sources` | +| `video-app.js` | 986 | group-name link badge | same, non-clickable when `N > 1` | +| `music-app.js` | 267 | `repTrack.groupName` | same rule | +| `photos-app.js` | 96 | `cover.groupName` | same rule | + +`files-app.js:446` is **not** in this list: the Files explorer is not merged and +its group column keeps naming exactly one group. + +New i18n key `search.n_sources` (`{ one: '{n} source', other: '{n} sources' }`) +in `en.js` and the nine other catalogues — `test_locales.py` holds them to `en`'s +key set and will fail otherwise. + +--- + +## 6. What must not change + +A checklist for the review, not prose. Each of these is a regression that would +be easy to ship and hard to notice. + +1. **The Files explorer.** Same folder tree, same per-group top level, same + badges, same navigation. `fileEntries` keeps building one entry per + (group, file). +2. **The single-group Group page.** `VideoApp`, `MusicApp`, `PhotosApp` are + rendered there with entries that carry no `_sources` and no `_tRef`. Every + changed component must behave identically when those fields are absent — + `entry._tRef || transportRef` is the existing idiom and stays. +3. **Streaming and download.** `VideoPlayer` and `downloadEntry` read the modal's + transport, set from `connectGroup(entry.groupId)`. With one `groupId` per + merged entry this is unchanged by construction — but a merged entry with a + *null* `groupId` would silently break both, so the merge must never emit one. +4. **The music queue.** `onPreview`'s audio branch builds siblings by + `e.path === origPath && e.groupId === groupId` over `allEntries`, which is the + *un-merged* list. It must be rebuilt over the merged music entries, or a queue + will contain each track twice — the exact bug, one layer down. +5. **The connection pool's eviction path.** `_onEvict` drops `groupConns` and + bumps `connectionGen`. Eviction is not failure and must not mark a group down. +6. **TMDB / MusicBrainz overrides.** An operator's "Fix match" and "Rematch" go + to the chosen source's node (`transportRef.current.rematchTmdbMatch`). With the + local node preferred, that is the operator's own node — correct. Worth a line + in `mediacenter.md` §10 all the same: on a file the operator does not host, the + override lands on whichever node was picked. +7. **`cacheGroupIndex`.** The IndexedDB cache is per group and stores raw index + entries. Merged entries must never be written to it. + +--- + +## 7. Execution order + +Each phase is independently testable and leaves the tree working. + +| # | Phase | Files | +|---|---|---| +| 1 | Export `groupPhotoAlbums`; record `is_node_admin` in `fetchGroupIndex`'s result | `photos-app.js`, `search-page.js` | +| 2 | `source-merge.js` — `pickSource`, `mergeUnitEntries`, `sourceLabel`. No caller yet | new file | +| 3 | Tests for phase 2, read out of the source | `test_search_source_merge.py` | +| 4 | Wire the Videos list through the merge | `search-page.js` | +| 5 | Same for Music, including the `onPreview` queue (§6.4) | `search-page.js` | +| 6 | Same for Photos | `search-page.js` | +| 7 | `downGroups` and failover | `search-page.js` | +| 8 | `sourceLabel` at the four display sites; `search.n_sources` in ten catalogues | `video-app.js`, `music-app.js`, `photos-app.js`, `locales/*.js` | +| 9 | Docs: `mediacenter.md` §10, `musicbay.md`, `photos.md`, `apps.md` if the props contract moves | `docs/` | + +Phase 4 alone is enough to confirm the reported bug is gone; phases 5–8 are the +same mechanism applied outward. + +--- + +## 8. Tests + +The SPA has no runtime test harness beyond "read the source and run it under +node", so that is what these are. Source-reading tests are weak evidence and are +the only evidence available here — which is why every one below **re-derives** a +value from the real source rather than restating a constant. + +| Test | Holds | +|---|---| +| `test_search_source_merge.py` | `pickSource` / `mergeUnitEntries` lifted out of `source-merge.js` and executed: a file in two groups yields one entry with two sources; a local source always wins; the pick is stable across calls and varies with the salt; a unit's files share the unit's source; a file the unit's source lacks falls back to its own; a down group is skipped; all-down falls back to the full list | +| extend `test_locales.py` | already fails on a key present in `en.js` and missing elsewhere — no change needed, listed so the ten-catalogue edit is not forgotten | +| `test_search_files_unmerged.py` | reads `search-page.js` and refuses a build where `fileEntries` is fed through the merge — the one guarantee §6.1 makes, and the one a later refactor is most likely to break by tidying the four lists into one | +| extend `test_transport_contracts.py` | the existing "declared vs. called setters" check covers the new state in `search-page.js` for free | + +Beyond the suite, this needs a person: two groups sharing one directory, one +film and one multi-season show, checked in Posters, Flat list, the detail modal, +Music and Photos, plus one playback and one download from a merged entry. The +standing rule from `CLAUDE.md` applies — a stylesheet does not tell you where +anything lands, and neither does a source-reading test tell you what a poster +grid renders. + +--- + +## 9. Still open + +| Item | Status | +|---|---| +| Client-side content verification | **Open.** The merge lets bytes arrive from a group the reader did not name (§3.1). Closing it means re-hashing plaintext against `file_id`, which needs blake3 in the browser and is impossible before playback for a stream. Not attempted here | +| Showing *which* source was picked | Deliberately not shown, per the request. `_sources` is on the entry, so a debug affordance is cheap if it is ever wanted | +| Merging across nodes for *availability* | The failover in §5.4 is per unit and reactive. Preferring a node that is already connected, or one that answered faster, is a further step and is not planned | +| Preferring the local node by `node_id` rather than `is_node_admin` | §5.3. The exact signal exists and costs nothing to record; the proxy is used because it needs no new field on the wire | |