diff options
Diffstat (limited to 'docs')
| -rw-r--r-- | docs/refactoring-search.md | 66 |
1 files changed, 43 insertions, 23 deletions
diff --git a/docs/refactoring-search.md b/docs/refactoring-search.md index f23e1ef..2210397 100644 --- a/docs/refactoring-search.md +++ b/docs/refactoring-search.md @@ -1,9 +1,10 @@ # Refactor: one file, one entry — merging duplicate sources in the Search view -> Status: **phases 1–4 built** (2026-09-02), 5–9 planned. Branch -> `feat/search-source-merge`. The Videos view is merged and the reported bug is -> gone under test; Music, Photos, failover and the badge are still to come, so -> a merged film still shows its chosen group's name rather than `N sources`. +> Status: **phases 1–8 built** (2026-09-02), 9 (the doc pass) outstanding. +> 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. @@ -179,14 +180,16 @@ 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 } +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. @@ -275,14 +278,20 @@ one source behaves exactly as it does today. ### 5.5 The badge -One helper, `sourceLabel(entry)`, four call sites: +One component, `SourceTag`, four call sites: -| File | Line today | Today | After | +| File | Today | After | Counts over | |---|---|---|---| -| `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 | +| `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. @@ -309,10 +318,14 @@ be easy to ship and hard to notice. 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. +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 @@ -335,15 +348,21 @@ Each phase is independently testable and leaves the tree working. | 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, 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` | +| 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, `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 +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 @@ -358,8 +377,9 @@ value from the real source rather than restating a constant. | `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 | +| `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: |