diff options
Diffstat (limited to 'docs/refactoring-search.md')
| -rw-r--r-- | docs/refactoring-search.md | 426 |
1 files changed, 0 insertions, 426 deletions
diff --git a/docs/refactoring-search.md b/docs/refactoring-search.md deleted file mode 100644 index 3172a24..0000000 --- a/docs/refactoring-search.md +++ /dev/null @@ -1,426 +0,0 @@ -# Refactor: one file, one entry — merging duplicate sources in the Search view - -> **Superseded by `MESHBAY_DESIGN.md`.** This was the cross-group source merging; its design -> content now lives in §9.10. -> -> It is kept because code comments, tests and other documents cite its -> sections and its labels, and because it records reasoning a synthesis -> compresses. **Where it disagrees with `MESHBAY_DESIGN.md`, the design -> document is right; where either disagrees with the code, the code is.** -> `MESHBAY_DESIGN.md` §16 maps every section reference here onto its -> replacement, and §13 defines every label. - -> Status: **complete** (2026-09-02) — all nine phases landed. This stays as -> the decision record. -> Branch `feat/search-source-merge`. Videos, Music and Photos are merged, -> failover is in, and a card with several sources says `N sources` instead of a -> group name. Confirmed live for Videos on 2026-09-02: one card per film, -> playback and download work from a merged entry. -> 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) → merged entries -pickSource(sources, unitKey, opts) → source | null -sourceLabel(entries | entry) → { count, name, groupId } -``` - -- `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)`. -- `sourceLabel` returns a shape, not a string: the module imports nothing (its - test executes it standalone), so it holds no reference to `i18n.js`. -- 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 component, `SourceTag`, four call sites: - -| File | Today | After | Counts over | -|---|---|---|---| -| `video-app.js` `PosterCard` | `repEntry.groupName` | group name, or `N sources` | the movie, or the show's episodes | -| `video-app.js` `FlatMovieRow` | group-name link badge | same, non-clickable when `N > 1` | the one entry | -| `music-app.js` `AlbumCard` | `repTrack.groupName` | same rule | the album's tracks | -| `photos-app.js` `AlbumCard` | `cover.groupName` | same rule | the album's photos | - -**`SourceTag` lives in `group-name.js`**, not in `source-merge.js` (which must -keep importing nothing) and not in each of the three apps (a copy apiece is -three chances to disagree about what a merged card says). It renders a `div` by -default — the three card badges rely on `text-overflow: ellipsis`, which does -nothing on an inline box — and the flat row's inline pill on `link`. - -`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 must be rebuilt over the - merged entries or a queue will contain each track twice.~~ **Wrong, checked - 2026-09-02 and left alone.** That branch builds siblings over the un-merged - `allEntries` but filters `e.groupId === groupId`, so a queue only ever holds - one group's copies. It is also reachable only from `FilesPanel`, which is not - merged — the Music view never goes through `onPreview`, it calls `onPlayQueue` - with its own (merged) tracks. Both facts had to be false for the bug to - exist; neither is. -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. -8. **`webapp.py`'s `_ASSETS`.** A new static file missing from it changes - without moving the asset URL, so a browser that cached the page keeps the - old copy — and nothing errors. `source-merge.js` shipped missing from it; - caught afterwards, and `test_every_static_script_participates_in_the_fingerprint` - now holds the list to every `.js` in `static/` so the next one cannot. - ---- - -## 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`, `test_search_video_merge.py` | -| 4 ✅ | Wire the Videos list through the merge | `search-page.js` | -| 5 ✅ | Same for Music (`foldKey` exported — an album's display strings are the first-seen spelling, so keying a unit on them would let the source change between page loads) | `search-page.js`, `music-app.js` | -| 6 ✅ | Same for Photos | `search-page.js` | -| 7 ✅ | `downGroups` and failover | `search-page.js` | -| 8 ✅ | `SourceTag` at the four display sites; `search.n_sources` in ten catalogues | `group-name.js`, `video-app.js`, `music-app.js`, `photos-app.js`, `locales/*.js` | -| 9 ✅ | Docs: `mediacenter.md` §10.6, `musicbay.md` §9b, `photos.md` §10b, `apps.md` §2b + checklist step 5 | `docs/` | - -Phase 4 alone was enough to confirm the reported bug is gone; phases 5–8 are the -same mechanism applied outward. - -**One thing phase 8 changed about the design.** `sourceLabel` was specified to -take one entry. A card stands for a *unit*, and the entry it is drawn from is -picked for its thumbnail — `episodes.find((e) => e.thumb_hash)` — so a show in -two groups whose cover episode sits in only one of them would have said one -source. It takes the whole unit now, and unions. - ---- - -## 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` ✅ | The whole of `source-merge.js` executed standalone — it has no imports precisely so that it can be, and the test refuses a build where it gains one. A file in two groups yields one entry with two sources; a local source always wins, over every salt; the pick is stable across calls, varies with the salt, and spreads one reader across units; source order does not decide it; a unit's files share the unit's source; a file the unit's source lacks falls back with its siblings; a down group is skipped, a down *local* group yields to a live remote, all-down still returns an entry; no field is back-filled from another source | -| `test_search_video_merge.py` ✅ | The reported symptom end to end. `groupVideoEntries` + `buildSeasons` (video-app.js) and `videoUnits` (search-page.js) are lifted from their real sources, the pipeline is assembled as the page assembles it, and the result is re-grouped the way `VideoApp` re-groups it — so what is counted is what the grid renders. Two groups sharing one library give one film card and one show whose seasons hold three episodes, not six; one group is unchanged; an episode only one group has survives | -| 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. It also asserts the other three lists *are* merged, or deleting the merge outright would leave it passing and saying nothing | -| extend `test_transport_contracts.py` | the existing "declared vs. called setters" check covers the new state in `search-page.js` for free | -| `test_search_media_merge.py` ✅ | the same end-to-end shape for Music (an album's tracks listed once; a differently-cased tag not splitting the unit; a bonus track only one group has; an untagged track kept as its own unit) and Photos (one album, each photo once; two differently-named albums each keeping the photo) | - -**Every one of these was checked against the fix removed**, which is where the -first version of "a unit's files share its source" turned out to prove nothing: -with every episode in every group, picking per file and picking per unit give -the same answer — the same key over the same set — so the test passed against a -per-file implementation. It now uses a unit whose files have *unequal* sources, -which is the only shape where the two rules come apart. Five mutations are -caught: dropping the local preference, picking per file, not de-duplicating a -group announcing a file twice, dropping the group-id sort, and hashing the salt -without the unit key. - -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 | |