diff options
| author | Christophe Besson <cbesson@gmail.com> | 2026-09-02 14:16:50 +0200 |
|---|---|---|
| committer | Christophe Besson <cbesson@gmail.com> | 2026-09-02 14:16:50 +0200 |
| commit | 15d3eec914cf5e474e66f615c9fbaf602eebe575 (patch) | |
| tree | 734584e3206e7cda0401804d8eba942557a5d013 /docs | |
| parent | 16b5e5c463ec998c6434bbf2ef39113ed01da801 (diff) | |
| download | meshbay-15d3eec914cf5e474e66f615c9fbaf602eebe575.tar.gz | |
docs: plan for merging duplicate sources in the Search view
A file shared by two groups is two entries in the cross-group Search
view: one film shows as two poster cards, one episode twice in a show's
list, one track twice in an album. Within a group this cannot happen —
GroupIndex is keyed by blake3 — so the duplication is created by the
Search page concatenating N independently keyed indexes.
The plan: merge on the content hash, choose one source per logical unit
(film, show, album), prefer a group hosted by the local node, otherwise
pick deterministically per user, and fail over when the chosen source is
unreachable. The Files explorer stays navigable per group and is not
merged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AbwJDbNTkiRUh7HTWEoyss
Diffstat (limited to 'docs')
| -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 | |