diff options
21 files changed, 670 insertions, 246 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: diff --git a/packages/meshbay-hub/src/meshbay_hub/static/group-name.js b/packages/meshbay-hub/src/meshbay_hub/static/group-name.js index af8879c..f109da7 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/group-name.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/group-name.js @@ -1,4 +1,6 @@ import { html } from './vendor/htm-preact.js'; +import { t } from './i18n.js'; +import { sourceLabel } from './source-merge.js'; /** * A group's name with the `@owner` handle under it. @@ -20,3 +22,36 @@ export function GroupName({ name, owner, inline = false }) { </span> `; } + +/** + * Which group serves an entry, or how many groups have it. + * + * The Search view merges a file several groups share into one entry, so the + * badge under a card cannot always name a group. One source keeps naming it + * and keeps linking to it; more than one becomes a count, and *which* one was + * picked is deliberately not shown (docs/refactoring-search.md §5.5). + * + * `entries` is the whole unit — every episode of a show, every track of an + * album — not the entry the card was drawn from; `sourceLabel` explains why. + * Renders nothing at all on the single-group Group page, where an entry + * carries no group and there is only ever one source anyway. + * + * It lives here rather than in `source-merge.js` because that module is + * executed standalone by its test and must keep importing nothing; and here + * rather than in one of the three apps that need it, because a copy apiece is + * three chances to disagree about what a merged card says. + * + * A `div` by default: the three card badges rely on `text-overflow: ellipsis`, + * which does nothing on an inline box. `link` gives the flat row's inline + * pill instead. + */ +export function SourceTag({ entries, cls, link = false }) { + const { count, name, groupId } = sourceLabel(entries); + if (count > 1) return html`<span class=${cls}>${t('search.n_sources', { n: count })}</span>`; + if (!name) return null; + if (link && groupId) { + return html`<a href="#/group/${groupId}" class=${cls} + onClick=${(e) => e.stopPropagation()}>${name}</a>`; + } + return html`<div class=${cls}>${name}</div>`; +} diff --git a/packages/meshbay-hub/src/meshbay_hub/static/locales/de.js b/packages/meshbay-hub/src/meshbay_hub/static/locales/de.js index f118ffe..b66c847 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/de.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/de.js @@ -616,6 +616,7 @@ export default { 'search.hint': 'Tippen Sie einen Begriff ein, um in allen Ihren Gruppen zu suchen.', 'search.indexing': 'Indexierung von {done} / {total} Gruppen …', 'search.unreachable': { one: '{n} Gruppe nicht erreichbar', other: '{n} Gruppen nicht erreichbar' }, + 'search.n_sources': { one: '{n} Quelle', other: '{n} Quellen' }, 'search.view_files': 'Dateien', 'search.view_videos': 'Videos', 'search.view_music': 'Musik', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/locales/en.js b/packages/meshbay-hub/src/meshbay_hub/static/locales/en.js index b55359b..898f7f4 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/en.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/en.js @@ -690,6 +690,7 @@ export default { 'search.hint': 'Type to search file names across all your groups.', 'search.indexing': 'Indexing {done} / {total} groups...', 'search.unreachable': { one: '{n} group unreachable', other: '{n} groups unreachable' }, + 'search.n_sources': { one: '{n} source', other: '{n} sources' }, 'search.view_files': 'Files', 'search.view_videos': 'Videos', 'search.view_music': 'Music', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/locales/es.js b/packages/meshbay-hub/src/meshbay_hub/static/locales/es.js index 8e90dca..8d06a2d 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/es.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/es.js @@ -612,6 +612,7 @@ export default { 'search.hint': 'Escriba un término para buscar en todos sus grupos.', 'search.indexing': 'Indexando {done} / {total} grupos…', 'search.unreachable': { one: '{n} grupo inalcanzable', other: '{n} grupos inalcanzables' }, + 'search.n_sources': { one: '{n} fuente', other: '{n} fuentes' }, 'search.view_files': 'Archivos', 'search.view_videos': 'Vídeos', 'search.view_music': 'Música', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/locales/fr.js b/packages/meshbay-hub/src/meshbay_hub/static/locales/fr.js index 995d4b3..1999e7e 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/fr.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/fr.js @@ -615,6 +615,7 @@ export default { 'search.hint': 'Saisissez un terme pour rechercher dans tous vos groupes.', 'search.indexing': 'Indexation de {done} / {total} groupes…', 'search.unreachable': { one: '{n} groupe injoignable', other: '{n} groupes injoignables' }, + 'search.n_sources': { one: '{n} source', other: '{n} sources' }, 'search.view_files': 'Fichiers', 'search.view_videos': 'Vidéos', 'search.view_music': 'Musique', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/locales/it.js b/packages/meshbay-hub/src/meshbay_hub/static/locales/it.js index 467521f..52e18a8 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/it.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/it.js @@ -614,6 +614,7 @@ export default { 'search.hint': 'Digita un termine per cercare in tutti i tuoi gruppi.', 'search.indexing': 'Indicizzazione di {done} / {total} gruppi…', 'search.unreachable': { one: '{n} gruppo non raggiungibile', other: '{n} gruppi non raggiungibili' }, + 'search.n_sources': { one: '{n} fonte', other: '{n} fonti' }, 'search.view_files': 'File', 'search.view_videos': 'Video', 'search.view_music': 'Musica', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/locales/ja.js b/packages/meshbay-hub/src/meshbay_hub/static/locales/ja.js index 89e8bdd..c0560b3 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/ja.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/ja.js @@ -602,6 +602,7 @@ export default { 'search.hint': '入力してすべてのグループを検索します。', 'search.indexing': '{done} / {total} グループをインデックス中…', 'search.unreachable': { other: '{n} グループに接続できません' }, + 'search.n_sources': { other: '{n} 個のソース' }, 'search.view_files': 'ファイル', 'search.view_videos': '動画', 'search.view_music': '音楽', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/locales/nl.js b/packages/meshbay-hub/src/meshbay_hub/static/locales/nl.js index 1ed0362..da6dc04 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/nl.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/nl.js @@ -616,6 +616,7 @@ export default { 'search.hint': 'Typ een zoekterm om in al uw groepen te zoeken.', 'search.indexing': '{done} / {total} groepen indexeren…', 'search.unreachable': { one: '{n} groep onbereikbaar', other: '{n} groepen onbereikbaar' }, + 'search.n_sources': { one: '{n} bron', other: '{n} bronnen' }, 'search.view_files': 'Bestanden', 'search.view_videos': 'Video\'s', 'search.view_music': 'Muziek', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/locales/pl.js b/packages/meshbay-hub/src/meshbay_hub/static/locales/pl.js index f71d048..29feebe 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/pl.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/pl.js @@ -634,6 +634,7 @@ export default { 'search.hint': 'Wpisz frazę, aby wyszukać we wszystkich grupach.', 'search.indexing': 'Indeksowanie {done} / {total} grup…', 'search.unreachable': { one: '{n} grupa nieosiągalna', few: '{n} grupy nieosiągalne', many: '{n} grup nieosiągalnych', other: '{n} grupy nieosiągalnej' }, + 'search.n_sources': { one: '{n} źródło', few: '{n} źródła', many: '{n} źródeł', other: '{n} źródła' }, 'search.view_files': 'Pliki', 'search.view_videos': 'Filmy', 'search.view_music': 'Muzyka', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/locales/pt-BR.js b/packages/meshbay-hub/src/meshbay_hub/static/locales/pt-BR.js index 1919edd..d2fc355 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/pt-BR.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/pt-BR.js @@ -613,6 +613,7 @@ export default { 'search.hint': 'Digite um termo para pesquisar em todos os seus grupos.', 'search.indexing': 'Indexando {done} / {total} grupos…', 'search.unreachable': { one: '{n} grupo inacessível', other: '{n} grupos inacessíveis' }, + 'search.n_sources': { one: '{n} fonte', other: '{n} fontes' }, 'search.view_files': 'Arquivos', 'search.view_videos': 'Vídeos', 'search.view_music': 'Música', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/locales/zh-CN.js b/packages/meshbay-hub/src/meshbay_hub/static/locales/zh-CN.js index 489311e..6840774 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/zh-CN.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/zh-CN.js @@ -589,6 +589,7 @@ export default { 'search.hint': '输入关键词搜索所有群组。', 'search.indexing': '正在索引 {done} / {total} 个群组…', 'search.unreachable': { other: '{n} 个群组无法连接' }, + 'search.n_sources': { other: '{n} 个来源' }, 'search.view_files': '文件', 'search.view_videos': '视频', 'search.view_music': '音乐', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/music-app.js b/packages/meshbay-hub/src/meshbay_hub/static/music-app.js index 418da89..616f169 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/music-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/music-app.js @@ -5,6 +5,7 @@ import { t } from './i18n.js'; import { Icon } from './icon.js'; import { MediaThumb, LazyTile } from './video-app.js'; import { formatTime } from './music-player.js'; +import { SourceTag } from './group-name.js'; // -- Music -------------------------------------------------------------------- // @@ -264,9 +265,7 @@ function AlbumCard({ album, transportRef, gekRef, musicbrainzEnabled, onOpen }) <div class="music-card-info"> <div class="music-card-title">${album.album}</div> <div class="music-card-sub">${album.artist}</div> - ${repTrack.groupName && html` - <div class="music-card-group">${repTrack.groupName}</div> - `} + <${SourceTag} entries=${album.tracks} cls="music-card-group" /> </div> </div> `; @@ -525,4 +524,10 @@ function MusicApp({ `; } -export { MusicApp, groupMusicEntries, bumpMusicMetaGeneration }; +// foldKey rides along for the Search page's merge unit keys +// (docs/refactoring-search.md §5.2). An album's *display* strings are the +// first-seen spelling, and which group is seen first is the order its index +// happened to arrive in — so keying a unit on them would let the chosen source +// change between page loads. The folded key is the one grouping actually used, +// and is stable. +export { MusicApp, groupMusicEntries, bumpMusicMetaGeneration, foldKey }; diff --git a/packages/meshbay-hub/src/meshbay_hub/static/photos-app.js b/packages/meshbay-hub/src/meshbay_hub/static/photos-app.js index 1755392..e6f3482 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/photos-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/photos-app.js @@ -8,6 +8,7 @@ import { } from './file-utils.js'; import { transfers } from './transfers.js'; import { MediaThumb, LazyTile } from './video-app.js'; +import { SourceTag } from './group-name.js'; // ── Photos ─────────────────────────────────────────────────────────────────── // @@ -93,9 +94,7 @@ function AlbumCard({ album, transportRef, gekRef, onOpen }) { <div class="photo-album-sub"> ${year}${year ? ' · ' : ''}${t('photo.n_photos', { n: album.photos.length })} </div> - ${cover.groupName && html` - <div class="photo-card-group">${cover.groupName}</div> - `} + <${SourceTag} entries=${album.photos} cls="photo-card-group" /> </div> </div> `; diff --git a/packages/meshbay-hub/src/meshbay_hub/static/search-page.js b/packages/meshbay-hub/src/meshbay_hub/static/search-page.js index 5897acb..608bb81 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/search-page.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/search-page.js @@ -9,8 +9,8 @@ import { } from './hub-client.js'; import { FilesPanel, FilePreview } from './files-app.js'; import { VideoApp, groupVideoEntries } from './video-app.js'; -import { MusicApp } from './music-app.js'; -import { PhotosApp } from './photos-app.js'; +import { MusicApp, groupMusicEntries, foldKey } from './music-app.js'; +import { PhotosApp, groupPhotoAlbums } from './photos-app.js'; import { VideoPlayer } from './video-player.js'; import { transfers } from './transfers.js'; import { mergeUnitEntries } from './source-merge.js'; @@ -233,6 +233,30 @@ function videoUnits(entries) { ]; } +// An album is a unit, and so is a track loose enough to have no artist at all. +// `groupMusicEntries` has already folded the two groups' copies into one album +// object, so the key only has to name it stably — hence `foldKey` over the +// display strings, which are whichever spelling arrived first. +function musicUnits(entries) { + const { tracks, albums } = groupMusicEntries(entries, SEARCH_AUDIO_ROOT); + return [ + ...albums.map((a) => ({ + key: `album:${foldKey(a.artist)}/${foldKey(a.album)}`, + entries: a.tracks, + })), + ...tracks.map((e) => ({ key: `track:${e.id}`, entries: [e] })), + ]; +} + +// A photo album is its directory, which is already prefixed per group when the +// entries are built — so two groups whose roots have different basenames stay +// two albums, and the same photo belongs in both. Only same-named albums +// collapse, which is the reported shape. +function photoUnits(entries) { + return groupPhotoAlbums(entries, SEARCH_PHOTO_ROOTS) + .map((a) => ({ key: `album:${a.dir}`, entries: a.photos })); +} + function SearchPage({ token, username, userId, groups, onPlayQueue, userPrefs }) { const [indexedGroups, setIndexedGroups] = useState(new Map()); const [progress, setProgress] = useState({ done: 0, total: 0, unreachable: [] }); @@ -254,6 +278,8 @@ function SearchPage({ token, username, userId, groups, onPlayQueue, userPrefs }) // key, so a group's posters/thumbnails recover the moment it reconnects // (after a pool eviction) instead of staying stuck on a spinner. const connGenRef = useRef(new Map()); + // groupIds whose connection failed — see markGroupDown below. + const downGroups = useRef(new Set()); const mountedRef = useRef(true); const [connectionGen, setConnectionGen] = useState(0); const debounceRef = useRef(null); @@ -301,6 +327,27 @@ function SearchPage({ token, username, userId, groups, onPlayQueue, userPrefs }) // -- Connection management -- + // A group whose connection failed stops being chosen as a merged entry's + // source, so a unit fails over to another group that has the file + // (docs/refactoring-search.md §5.4). Without this the merge could make a + // file *less* available than it was before it, which would be a regression + // dressed as a feature. + // + // Held in a ref and read live by `mergeOpts.isDown`; what actually rebuilds + // the entry lists is the `connectionGen` bump, which they already depend on. + // Eviction is deliberately not a failure — `_onEvict` only drops the recorded + // connection, and the group is picked again as readily as before. + const markGroupDown = useCallback((groupId, down) => { + let changed; + if (down) { + changed = !downGroups.current.has(groupId); + downGroups.current.add(groupId); + } else { + changed = downGroups.current.delete(groupId); + } + if (changed && mountedRef.current) setConnectionGen((g) => g + 1); + }, []); + const connectGroup = useCallback(async (groupId) => { if (groupConns.current.has(groupId)) { const c = groupConns.current.get(groupId); @@ -308,7 +355,14 @@ function SearchPage({ token, username, userId, groups, onPlayQueue, userPrefs }) } if (!poolRef.current) throw new Error('no pool'); const bundleKey = session.bundleKey || await _loadBundleKey(); - const conn = await poolRef.current.connect(groupId, token, bundleKey, username, userId); + let conn; + try { + conn = await poolRef.current.connect(groupId, token, bundleKey, username, userId); + } catch (e) { + markGroupDown(groupId, true); + throw e; + } + markGroupDown(groupId, false); // A concurrent caller for the same group (several tiles mounting at once) // may already have recorded this exact transport while we awaited. Only @@ -339,7 +393,7 @@ function SearchPage({ token, username, userId, groups, onPlayQueue, userPrefs }) // module-wide bumps stay for an operator's TMDB override/rematch only. setConnectionGen((g) => g + 1); return entry; - }, [token, username, userId]); + }, [token, username, userId, markGroupDown]); // Warm up connections as soon as indexing finishes so thumbnails start // loading before the user switches views — but only up to the pool's @@ -431,6 +485,11 @@ function SearchPage({ token, username, userId, groups, onPlayQueue, userPrefs }) const data = indexedGroups.get(gid); return !!(data && data.isNodeAdmin); }, + // Read live off the ref rather than captured: what rebuilds the lists is + // the `connectionGen` bump markGroupDown fires, and they already depend on + // it. Putting the set in this memo's own dependencies would only add a + // second reason to rebuild the same thing. + isDown: (gid) => downGroups.current.has(gid), }), [indexedGroups, userId]); // Videos view: pre-filtered by videoRoot, path-prefixed, then merged so a @@ -460,7 +519,7 @@ function SearchPage({ token, username, userId, groups, onPlayQueue, userPrefs }) return mergeUnitEntries(videoUnits(result), mergeOpts); }, [indexedGroups, q, matchesQuery, connectionGen, mergeOpts]); - // Music view: pre-filtered by audioRoot, path-prefixed + // Music view: pre-filtered by audioRoot, path-prefixed, then merged per album const musicEntries = useMemo(() => { const result = []; for (const [groupId, data] of indexedGroups) { @@ -483,10 +542,10 @@ function SearchPage({ token, username, userId, groups, onPlayQueue, userPrefs }) }); } } - return result; - }, [indexedGroups, q, matchesQuery, connectionGen]); + return mergeUnitEntries(musicUnits(result), mergeOpts); + }, [indexedGroups, q, matchesQuery, connectionGen, mergeOpts]); - // Photos view: pre-filtered by photoRoots, path-prefixed + // Photos view: pre-filtered by photoRoots, path-prefixed, then merged per album const photoEntries = useMemo(() => { const result = []; for (const [groupId, data] of indexedGroups) { @@ -510,8 +569,8 @@ function SearchPage({ token, username, userId, groups, onPlayQueue, userPrefs }) }); } } - return result; - }, [indexedGroups, q, matchesQuery, connectionGen]); + return mergeUnitEntries(photoUnits(result), mergeOpts); + }, [indexedGroups, q, matchesQuery, connectionGen, mergeOpts]); // -- Callbacks -- diff --git a/packages/meshbay-hub/src/meshbay_hub/static/source-merge.js b/packages/meshbay-hub/src/meshbay_hub/static/source-merge.js index ccc4cfd..2c08e06 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/source-merge.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/source-merge.js @@ -136,24 +136,41 @@ function mergeUnitEntries(units, opts) { } /** - * What the group badge should say for one entry. + * What the group badge should say — for one entry, or for a whole unit. + * + * Takes a list, because a card stands for a unit while the entry it is drawn + * from is one file. A show's poster entry is picked for its *thumbnail* + * (`episodes.find((e) => e.thumb_hash)`), so reading the badge off it would + * report that one episode's sources: a show in two groups whose cover episode + * sits in only one of them would claim a single source. The union over the + * unit is the number the reader is actually being told. * * Returns `{ count, name, groupId }` rather than a rendered string: this module * is executed standalone under node by its test, so it holds no reference to * `i18n.js`. A caller renders `name` (a link to `groupId`) when `count` is 1, - * and a plural of `count` otherwise — which group was picked is deliberately + * and a plural of `count` above that — which group was picked is deliberately * never shown. * - * An entry with no `_sources` at all is the single-group Group page, where - * there is one source by construction and no badge is drawn. + * `count: 0` is an entry with no group at all, which is the single-group Group + * page: there is one source by construction and no badge is drawn there. */ -function sourceLabel(entry) { - const sources = (entry && entry._sources) || []; - if (sources.length > 1) return { count: sources.length, name: '', groupId: null }; +function sourceLabel(entries) { + const list = Array.isArray(entries) ? entries : [entries]; + const groups = new Map(); + for (const e of list) { + if (!e) continue; + // An un-merged entry carries no `_sources`; it is its own single source. + const sources = (e._sources && e._sources.length) + ? e._sources + : (e.groupId ? [_source(e)] : []); + for (const s of sources) if (!groups.has(s.groupId)) groups.set(s.groupId, s); + } + if (groups.size > 1) return { count: groups.size, name: '', groupId: null }; + const only = groups.values().next().value; return { - count: 1, - name: (entry && entry.groupName) || '', - groupId: (entry && entry.groupId) || null, + count: groups.size, + name: (only && only.groupName) || '', + groupId: (only && only.groupId) || null, }; } diff --git a/packages/meshbay-hub/src/meshbay_hub/static/video-app.js b/packages/meshbay-hub/src/meshbay_hub/static/video-app.js index 19b0f0f..4f52b16 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/video-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/video-app.js @@ -4,6 +4,7 @@ import { import { t } from './i18n.js'; import { Icon } from './icon.js'; import { formatSize, pipelinedDownload } from './file-utils.js'; +import { SourceTag } from './group-name.js'; // ── Videos ─────────────────────────────────────────────────────────────────── // @@ -319,6 +320,7 @@ function useSeasonMeta(transportRef, tmdbId, season, active) { function PosterCard({ title, subtitle, repEntry, transportRef, gekRef, onOpen, groupKey, onMetaResolved, onNeedConn, + sourceEntries, }) { const tRef = repEntry._tRef || transportRef; const gRef = repEntry._gRef || gekRef; @@ -402,9 +404,7 @@ function PosterCard({ ${confident && meta.first_air_date ? yearOf(meta.first_air_date) : ''} ${subtitle ? ` · ${subtitle}` : ''} </div> - ${repEntry.groupName && html` - <div class="video-card-group">${repEntry.groupName}</div> - `} + <${SourceTag} entries=${sourceEntries || repEntry} cls="video-card-group" /> </div> `} </div> @@ -892,6 +892,7 @@ function PosterGrid({ <${PosterCard} title=${e.display_title || e.name} subtitle=${formatDuration(e.duration)} repEntry=${e} groupKey=${`movie:${e.id}`} + sourceEntries=${e} transportRef=${transportRef} gekRef=${gekRef} onNeedConn=${onNeedConn} onOpen=${() => (tmdbEnabled @@ -925,6 +926,7 @@ function PosterGrid({ <${PosterCard} title=${s.title} subtitle=${subtitle} repEntry=${repEntry} + sourceEntries=${s.episodes} groupKey=${s.title} onMetaResolved=${handleMetaResolved} onNeedConn=${onNeedConn} @@ -983,10 +985,7 @@ function FlatMovieRow({ entry, transportRef, gekRef, onPreview, seasonContext, o ${' · '}${formatSize(entry.size)} </div> </div> - ${entry.groupName && html` - <a href="#/group/${entry.groupId}" class="badge search-group-badge" - onClick=${(e) => e.stopPropagation()}>${entry.groupName}</a> - `} + <${SourceTag} entries=${entry} cls="badge search-group-badge" link=${true} /> </div> `; } diff --git a/packages/meshbay-hub/tests/test_search_files_unmerged.py b/packages/meshbay-hub/tests/test_search_files_unmerged.py new file mode 100644 index 0000000..6956dde --- /dev/null +++ b/packages/meshbay-hub/tests/test_search_files_unmerged.py @@ -0,0 +1,80 @@ +""" +The Files explorer in the Search view is not merged, and must not become so. + +The Search view folds a file several groups share into one entry, so a film +shared by two groups is one poster instead of two. The Files tab is the one +place where that would be wrong: there each group is a top-level folder, the +two copies live in two different folders, and walking into one is how a member +browses *that group*. Merging would silently delete one of the two branches of +the tree. + +This is the guarantee a later refactor is most likely to break — the four entry +lists in `search-page.js` are near-identical, and tidying them into one shared +builder is the obvious cleanup. It would also be the last thing anyone tests by +hand, because the Files tab looks unchanged until you notice a group's folder +has fewer files in it than the group does. + +So: read the source, and refuse a build where `fileEntries` goes through the +merge. Weak evidence, and the only kind available for the SPA — but the failure +it guards against is a one-line edit, which is exactly what a source-reading +test catches well. + +See docs/refactoring-search.md §6.1. +""" + +import re +from pathlib import Path + +import pytest + +STATIC = Path(__file__).resolve().parents[1] / "src" / "meshbay_hub" / "static" +SEARCH_PAGE = STATIC / "search-page.js" + +pytestmark = pytest.mark.skipif( + not SEARCH_PAGE.exists(), reason="the SPA sources are not available") + +MERGE_CALL = "mergeUnitEntries" + + +def _memo(name): + """The body of `const <name> = useMemo(() => { ... }, [...]);`.""" + src = SEARCH_PAGE.read_text() + m = re.search( + r"^ const " + re.escape(name) + r" = useMemo\(\(\) => \{.*?^ \}, \[.*?\]\);", + src, re.M | re.S) + assert m, ( + f"{name} is no longer a useMemo where this test reads it — the Files " + "explorer's exemption from the merge is untested until this is fixed") + return m.group(0) + + +def test_the_files_list_is_not_merged(): + body = _memo("fileEntries") + assert MERGE_CALL not in body, ( + "fileEntries now goes through the source merge. The Files explorer " + "shows one folder per group and a member navigates into it; merging " + "two groups' copies of a file would remove it from one of those " + "folders. See docs/refactoring-search.md §6.1") + + +@pytest.mark.parametrize("name", ["videoEntries", "musicEntries", "photoEntries"]) +def test_the_media_lists_are_merged(name): + """ + The other half of the check. Without it, deleting the merge outright would + leave the test above passing and saying nothing. + """ + assert MERGE_CALL in _memo(name), ( + f"{name} no longer goes through the source merge — a file shared by " + "two groups is two entries again") + + +def test_the_files_list_still_carries_one_group_per_entry(): + """ + What makes the explorer work: the path is prefixed with the group's name, + so the top level of the tree is the set of groups. A merged entry could not + be prefixed with anything, having several. + """ + body = _memo("fileEntries") + assert "data.groupName + (e.path ? '/' + e.path : '')" in body, ( + "fileEntries no longer prefixes paths with the group name — the " + "explorer's per-group top level is what this whole exemption is for") diff --git a/packages/meshbay-hub/tests/test_search_media_merge.py b/packages/meshbay-hub/tests/test_search_media_merge.py new file mode 100644 index 0000000..0312c57 --- /dev/null +++ b/packages/meshbay-hub/tests/test_search_media_merge.py @@ -0,0 +1,363 @@ +""" +The reported symptom, end to end: one library shared by two groups. + +`test_search_source_merge.py` holds the merging rules in isolation. This holds +the thing an operator actually saw — a node hosting two groups that were given +the same video directory, and a Search view showing every film as two poster +cards and every episode twice inside a show. + +Three pieces have to agree for that to come out right, and each lives in a +different file: + + * the application's own grouping (`groupVideoEntries`, `groupMusicEntries`, + `groupPhotoAlbums`) turns entries into films, shows, albums; + * `videoUnits` / `musicUnits` / `photoUnits` (search-page.js) turn those into + merge units; + * `mergeUnitEntries` (source-merge.js) folds them on the content hash. + +All of them are read out of their real sources here rather than restated. Each +pipeline is assembled the way `search-page.js` assembles it, and the result is +passed through the grouping a second time — which is what the application does +with it — so what this counts is what the grid renders. + +`t()` is stubbed to return its key: `groupMusicEntries` uses it for the two +placeholder album names, and a string is not what is under test here. + +See docs/refactoring-search.md. +""" + +import json +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + +STATIC = Path(__file__).resolve().parents[1] / "src" / "meshbay_hub" / "static" +VIDEO_APP = STATIC / "video-app.js" +MUSIC_APP = STATIC / "music-app.js" +PHOTOS_APP = STATIC / "photos-app.js" +SEARCH_PAGE = STATIC / "search-page.js" +MERGE = STATIC / "source-merge.js" + +pytestmark = pytest.mark.skipif( + shutil.which("node") is None or not MERGE.exists(), + reason="node or the SPA sources are not available") + +EXPORT = re.compile(r"^export \{[^}]*\};?\s*$", re.M) + + +def _block(path, header): + """One top-level `function name(...) {` ... `}` read out of a module.""" + src = path.read_text() + m = re.search(r"^" + re.escape(header) + r".*?^\}", src, re.M | re.S) + assert m, ( + f"{header} is no longer where this test reads it from in {path.name} — " + "the Search view's de-duplication is untested until this is fixed") + return m.group(0) + + +def _const(name): + m = re.search(r"^const " + re.escape(name) + r" = .*?;$", + SEARCH_PAGE.read_text(), re.M) + assert m, f"{name} moved — the search-page units cannot be lifted" + return m.group(0) + + +@pytest.fixture(scope="module") +def pipeline(): + """Everything the three views need, in one script.""" + return "\n".join([ + "const t = (k) => k;", + EXPORT.sub("", MERGE.read_text()), + _block(VIDEO_APP, "function underVideoRoot(entry, videoRoot) {"), + _block(VIDEO_APP, "function buildSeasons(episodes) {"), + _block(VIDEO_APP, "function groupVideoEntries(entries, videoRoot) {"), + _block(MUSIC_APP, "function foldKey(s) {"), + _block(MUSIC_APP, "function underAudioRoot(entry, audioRoot) {"), + _block(MUSIC_APP, "function groupMusicEntries(entries, audioRoot) {"), + _block(PHOTOS_APP, "function underAnyPhotoRoot(entry, photoRoots) {"), + _block(PHOTOS_APP, "function groupPhotoAlbums(entries, photoRoots) {"), + _const("SEARCH_VIDEO_ROOT"), + _const("SEARCH_AUDIO_ROOT"), + _const("SEARCH_PHOTO_ROOTS"), + _block(SEARCH_PAGE, "function videoUnits(entries) {"), + _block(SEARCH_PAGE, "function musicUnits(entries) {"), + _block(SEARCH_PAGE, "function photoUnits(entries) {"), + ]) + + +def _node(tmp_path, pipeline, body): + script = tmp_path / "case.js" + script.write_text(f"{pipeline}\n{body}\n") + out = subprocess.run( + ["node", str(script)], capture_output=True, text=True, timeout=30) + assert out.returncode == 0, out.stderr + return json.loads(out.stdout) + + +def _grid(tmp_path, pipeline, entries, salt="reader", local=()): + """What the poster grid ends up with, after the merge and VideoApp's own + regrouping.""" + body = f""" + const raw = {json.dumps(entries)}; + const local = new Set({json.dumps(list(local))}); + const merged = mergeUnitEntries(videoUnits(raw), {{ + salt: {json.dumps(salt)}, + isLocal: (g) => local.has(g), + }}); + const {{ movies, shows }} = groupVideoEntries(merged, SEARCH_VIDEO_ROOT); + console.log(JSON.stringify({{ + movies: movies.map((e) => ({{ + id: e.id, title: e.display_title || e.name, groupId: e.groupId, + sources: e._sources.length, + }})), + shows: shows.map((s) => ({{ + title: s.title, + groups: [...new Set(s.episodes.map((e) => e.groupId))].sort(), + seasons: s.seasons.map((x) => ({{ + season: x.season, + episodes: x.episodes.map((e) => `S${{e.season}}E${{e.episode}}`), + }})), + }})), + }})); + """ + return _node(tmp_path, pipeline, body) + + +def _albums(tmp_path, pipeline, entries, salt="reader", local=()): + """What the album grid ends up with, after the merge and MusicApp's own + regrouping.""" + body = f""" + const raw = {json.dumps(entries)}; + const local = new Set({json.dumps(list(local))}); + const merged = mergeUnitEntries(musicUnits(raw), {{ + salt: {json.dumps(salt)}, + isLocal: (g) => local.has(g), + }}); + const {{ albums, tracks }} = groupMusicEntries(merged, SEARCH_AUDIO_ROOT); + console.log(JSON.stringify({{ + albums: albums.map((a) => ({{ + artist: a.artist, album: a.album, + groups: [...new Set(a.tracks.map((e) => e.groupId))].sort(), + tracks: a.tracks.map((e) => e.name), + }})), + loose: tracks.map((e) => e.name), + }})); + """ + return _node(tmp_path, pipeline, body) + + +def _photo_albums(tmp_path, pipeline, entries, salt="reader", local=()): + """What the photo album grid ends up with, after the merge and PhotosApp's + own regrouping.""" + body = f""" + const raw = {json.dumps(entries)}; + const local = new Set({json.dumps(list(local))}); + const merged = mergeUnitEntries(photoUnits(raw), {{ + salt: {json.dumps(salt)}, + isLocal: (g) => local.has(g), + }}); + console.log(JSON.stringify( + groupPhotoAlbums(merged, SEARCH_PHOTO_ROOTS).map((a) => ({{ + dir: a.dir, + groups: [...new Set(a.photos.map((e) => e.groupId))].sort(), + photos: a.photos.map((e) => e.name), + }})))); + """ + return _node(tmp_path, pipeline, body) + + +# The shape of one shared library: a film, and a two-season show. Invented +# titles — the real one this was found against is nobody's business here. +def _library(group): + """`path` is already prefixed the way search-page.js prefixes it.""" + def entry(file_id, name, **kw): + return { + "id": file_id, "name": name, "type": "video", + "path": "__search__/shows", "size": 1, + "groupId": group, "groupName": group.upper(), "groupOwner": "someone", + "_tRef": f"t:{group}", "_gRef": f"g:{group}", "_connGen": 1, + **kw, + } + files = [entry("film1", "a-film.mkv", display_title="Some Film")] + for season in (1, 2): + for ep in (1, 2, 3): + files.append(entry( + f"s{season}e{ep}", f"show.s0{season}e0{ep}.mkv", + display_title="Some Saga", season=season, episode=ep)) + return files + + +def test_a_shared_library_is_listed_once(tmp_path, pipeline): + """ + The bug as reported: two groups, one directory, everything twice. + """ + both = _library("demo35") + _library("media") + grid = _grid(tmp_path, pipeline, both) + + assert [m["title"] for m in grid["movies"]] == ["Some Film"] + assert grid["movies"][0]["sources"] == 2 + + assert len(grid["shows"]) == 1 + show = grid["shows"][0] + assert [s["season"] for s in show["seasons"]] == [1, 2] + for season in show["seasons"]: + assert season["episodes"] == [ + f"S{season['season']}E{n}" for n in (1, 2, 3)], ( + "an episode is listed more than once — this is the reported bug, " + "in the season list under the synopsis") + + +def test_a_show_streams_from_one_source(tmp_path, pipeline): + """A season split across two nodes would open two connections and two + metadata lookups for one show.""" + both = _library("demo35") + _library("media") + show = _grid(tmp_path, pipeline, both)["shows"][0] + assert len(show["groups"]) == 1 + + +def test_the_operators_own_node_serves_it(tmp_path, pipeline): + """Both groups are on the operator's node in the reported case; when only + one is, that one is the source.""" + both = _library("remote") + _library("mine") + grid = _grid(tmp_path, pipeline, both, local=["mine"]) + assert grid["movies"][0]["groupId"] == "mine" + assert grid["shows"][0]["groups"] == ["mine"] + + +def test_one_group_is_unchanged(tmp_path, pipeline): + """The overwhelmingly common case: nothing to merge, nothing different.""" + grid = _grid(tmp_path, pipeline, _library("solo")) + assert [m["title"] for m in grid["movies"]] == ["Some Film"] + assert grid["movies"][0]["sources"] == 1 + assert grid["movies"][0]["groupId"] == "solo" + show = grid["shows"][0] + assert show["groups"] == ["solo"] + assert sum(len(s["episodes"]) for s in show["seasons"]) == 6 + + +def test_an_episode_only_one_group_has_is_kept(tmp_path, pipeline): + """ + Merging must never subtract. A group holding one extra episode contributes + it, whichever source the show settled on. + """ + extra = _library("media") + extra.append({ + "id": "s2e4", "name": "show.s02e04.mkv", "type": "video", + "path": "__search__/shows", "size": 1, + "groupId": "media", "groupName": "MEDIA", "groupOwner": "someone", + "display_title": "Some Saga", "season": 2, "episode": 4, + }) + grid = _grid(tmp_path, pipeline, _library("demo35") + extra) + season2 = [s for s in grid["shows"][0]["seasons"] if s["season"] == 2][0] + assert season2["episodes"] == ["S2E1", "S2E2", "S2E3", "S2E4"] + + +# ── Music ──────────────────────────────────────────────────────────────────── + +def _record(group, tracks=5): + """One album, tagged, under the search audio root.""" + return [{ + "id": f"t{n}", "name": f"{n:02d}-track.flac", "type": "audio", + "path": "__search__/music/some-band/a-record", "size": 1, + "artist": "Some Band", "album": "A Record", "track_no": n, + "groupId": group, "groupName": group.upper(), "groupOwner": "someone", + "_tRef": f"t:{group}", "_gRef": f"g:{group}", "_connGen": 1, + } for n in range(1, tracks + 1)] + + +def test_an_album_shared_by_two_groups_lists_each_track_once(tmp_path, pipeline): + grid = _albums(tmp_path, pipeline, _record("demo35") + _record("media")) + assert len(grid["albums"]) == 1 + album = grid["albums"][0] + assert len(album["tracks"]) == 5, ( + "a track is listed more than once — the same bug as the Videos view, " + "inside an album") + assert album["groups"] == ["demo35"] or album["groups"] == ["media"] + + +def test_one_group_of_music_is_unchanged(tmp_path, pipeline): + grid = _albums(tmp_path, pipeline, _record("solo")) + assert len(grid["albums"]) == 1 + assert len(grid["albums"][0]["tracks"]) == 5 + assert grid["albums"][0]["groups"] == ["solo"] + + +def test_a_differently_cased_tag_does_not_split_the_unit(tmp_path, pipeline): + """ + `groupMusicEntries` folds case for grouping but keeps the first-seen + spelling for display, and which group is seen first is whichever index + arrived first. `musicUnits` keys on the folded form for exactly that + reason — on the display strings, the chosen source could change between + page loads. + """ + other = _record("media") + for track in other: + track["artist"] = "SOME BAND" + track["album"] = "a record" + grid = _albums(tmp_path, pipeline, _record("demo35") + other) + assert len(grid["albums"]) == 1 + assert len(grid["albums"][0]["tracks"]) == 5 + + +def test_a_track_only_one_group_has_is_kept(tmp_path, pipeline): + extra = _record("media") + extra.append({ + "id": "t9", "name": "09-bonus.flac", "type": "audio", + "path": "__search__/music/some-band/a-record", "size": 1, + "artist": "Some Band", "album": "A Record", "track_no": 9, + "groupId": "media", "groupName": "MEDIA", "groupOwner": "someone", + }) + grid = _albums(tmp_path, pipeline, _record("demo35") + extra) + assert len(grid["albums"][0]["tracks"]) == 6 + + +def test_an_untagged_track_is_its_own_unit(tmp_path, pipeline): + """A track with no artist at all never reaches an album bucket, so it has + to be a unit of its own or it would be dropped from the merge entirely.""" + loose = [{ + "id": "x1", "name": "unknown.mp3", "type": "audio", + "path": "__search__/music", "size": 1, + "groupId": g, "groupName": g.upper(), + } for g in ("demo35", "media")] + grid = _albums(tmp_path, pipeline, _record("demo35") + loose) + assert grid["loose"] == ["unknown.mp3"] + + +# ── Photos ─────────────────────────────────────────────────────────────────── + +def _photos(group, root="pics", album="a-trip", n=4): + return [{ + "id": f"p{i}", "name": f"IMG_{i:04d}.jpg", "type": "image", + "path": f"__search_photos__/{root}/{album}", "size": 1, + "groupId": group, "groupName": group.upper(), "groupOwner": "someone", + "_tRef": f"t:{group}", "_gRef": f"g:{group}", "_connGen": 1, + } for i in range(1, n + 1)] + + +def test_a_photo_album_shared_by_two_groups_shows_each_photo_once(tmp_path, pipeline): + albums = _photo_albums(tmp_path, pipeline, _photos("demo35") + _photos("media")) + assert len(albums) == 1 + assert len(albums[0]["photos"]) == 4 + assert len(albums[0]["groups"]) == 1 + + +def test_two_differently_named_albums_both_keep_the_photo(tmp_path, pipeline): + """ + Documented consequence, not a bug: photo albums are keyed by directory, so + two groups whose roots have different basenames are two albums, and a + photo in both belongs in both. + """ + albums = _photo_albums( + tmp_path, pipeline, _photos("demo35", root="pics") + _photos("media", root="images")) + assert len(albums) == 2 + assert all(len(a["photos"]) == 4 for a in albums) + + +def test_one_group_of_photos_is_unchanged(tmp_path, pipeline): + albums = _photo_albums(tmp_path, pipeline, _photos("solo")) + assert len(albums) == 1 + assert albums[0]["groups"] == ["solo"] + assert len(albums[0]["photos"]) == 4 diff --git a/packages/meshbay-hub/tests/test_search_source_merge.py b/packages/meshbay-hub/tests/test_search_source_merge.py index 078c38d..cadf095 100644 --- a/packages/meshbay-hub/tests/test_search_source_merge.py +++ b/packages/meshbay-hub/tests/test_search_source_merge.py @@ -328,13 +328,35 @@ def test_source_label(tmp_path, module_source): _sources: [{ groupId: 'g1', groupName: 'G1' }] }; const many = { groupId: 'g1', groupName: 'G1', _sources: [{ groupId: 'g1' }, { groupId: 'g2' }] }; + const unmerged = { groupId: 'g9', groupName: 'G9' }; const bare = { }; console.log(JSON.stringify( - [one, many, bare].map(sourceLabel))); + [one, many, unmerged, bare].map((e) => sourceLabel(e)))); """ - one, many, bare = _run(tmp_path, module_source, body) + one, many, unmerged, bare = _run(tmp_path, module_source, body) assert one == {"count": 1, "name": "G1", "groupId": "g1"} # Which group was picked is deliberately not shown once there are several. assert many == {"count": 2, "name": "", "groupId": None} - # The single-group Group page, where entries carry no sources at all. - assert bare == {"count": 1, "name": "", "groupId": None} + # An entry that never went through the merge is its own single source. + assert unmerged == {"count": 1, "name": "G9", "groupId": "g9"} + # The single-group Group page, where entries carry no group at all. + assert bare == {"count": 0, "name": "", "groupId": None} + + +def test_source_label_counts_the_unit_not_the_cover(tmp_path, module_source): + """ + A card stands for a unit; the entry it is drawn from is one file. A show's + poster entry is chosen for its *thumbnail*, so a show in two groups whose + cover episode sits in only one of them would have claimed a single source. + """ + body = """ + const cover = { id: 'e1', groupId: 'aa', groupName: 'AA', + _sources: [{ groupId: 'aa', groupName: 'AA' }] }; + const rest = { id: 'e2', groupId: 'aa', groupName: 'AA', + _sources: [{ groupId: 'aa' }, { groupId: 'bb' }] }; + console.log(JSON.stringify( + [sourceLabel(cover), sourceLabel([cover, rest])])); + """ + alone, unit = _run(tmp_path, module_source, body) + assert alone["count"] == 1 + assert unit["count"] == 2 diff --git a/packages/meshbay-hub/tests/test_search_video_merge.py b/packages/meshbay-hub/tests/test_search_video_merge.py deleted file mode 100644 index e9ab258..0000000 --- a/packages/meshbay-hub/tests/test_search_video_merge.py +++ /dev/null @@ -1,185 +0,0 @@ -""" -The reported symptom, end to end: one library shared by two groups. - -`test_search_source_merge.py` holds the merging rules in isolation. This holds -the thing an operator actually saw — a node hosting two groups that were given -the same video directory, and a Search view showing every film as two poster -cards and every episode twice inside a show. - -Three pieces have to agree for that to come out right, and each lives in a -different file: - - * `groupVideoEntries` (video-app.js) turns entries into films and shows; - * `videoUnits` (search-page.js) turns those into merge units; - * `mergeUnitEntries` (source-merge.js) folds them on the content hash. - -All three are read out of their real sources here rather than restated. The -pipeline is assembled the way `search-page.js` assembles it, and then the -result is passed through `groupVideoEntries` a second time — which is what -`VideoApp` does with it — so what this counts is what the grid renders. - -See docs/refactoring-search.md. -""" - -import json -import re -import shutil -import subprocess -from pathlib import Path - -import pytest - -STATIC = Path(__file__).resolve().parents[1] / "src" / "meshbay_hub" / "static" -VIDEO_APP = STATIC / "video-app.js" -SEARCH_PAGE = STATIC / "search-page.js" -MERGE = STATIC / "source-merge.js" - -pytestmark = pytest.mark.skipif( - shutil.which("node") is None or not MERGE.exists(), - reason="node or the SPA sources are not available") - -EXPORT = re.compile(r"^export \{[^}]*\};?\s*$", re.M) - - -def _block(path, header): - """One top-level `function name(...) {` ... `}` read out of a module.""" - src = path.read_text() - m = re.search(r"^" + re.escape(header) + r".*?^\}", src, re.M | re.S) - assert m, ( - f"{header} is no longer where this test reads it from in {path.name} — " - "the Search view's de-duplication is untested until this is fixed") - return m.group(0) - - -@pytest.fixture(scope="module") -def pipeline(): - root = re.search(r"^const SEARCH_VIDEO_ROOT = .*?;$", SEARCH_PAGE.read_text(), re.M) - assert root, "SEARCH_VIDEO_ROOT moved — videoUnits cannot be lifted" - return "\n".join([ - EXPORT.sub("", MERGE.read_text()), - _block(VIDEO_APP, "function underVideoRoot(entry, videoRoot) {"), - _block(VIDEO_APP, "function buildSeasons(episodes) {"), - _block(VIDEO_APP, "function groupVideoEntries(entries, videoRoot) {"), - root.group(0), - _block(SEARCH_PAGE, "function videoUnits(entries) {"), - ]) - - -def _grid(tmp_path, pipeline, entries, salt="reader", local=()): - """What the poster grid ends up with, after the merge and VideoApp's own - regrouping.""" - body = f""" - const raw = {json.dumps(entries)}; - const local = new Set({json.dumps(list(local))}); - const merged = mergeUnitEntries(videoUnits(raw), {{ - salt: {json.dumps(salt)}, - isLocal: (g) => local.has(g), - }}); - const {{ movies, shows }} = groupVideoEntries(merged, SEARCH_VIDEO_ROOT); - console.log(JSON.stringify({{ - movies: movies.map((e) => ({{ - id: e.id, title: e.display_title || e.name, groupId: e.groupId, - sources: e._sources.length, - }})), - shows: shows.map((s) => ({{ - title: s.title, - groups: [...new Set(s.episodes.map((e) => e.groupId))].sort(), - seasons: s.seasons.map((x) => ({{ - season: x.season, - episodes: x.episodes.map((e) => `S${{e.season}}E${{e.episode}}`), - }})), - }})), - }})); - """ - script = tmp_path / "case.js" - script.write_text(f"{pipeline}\n{body}\n") - out = subprocess.run( - ["node", str(script)], capture_output=True, text=True, timeout=30) - assert out.returncode == 0, out.stderr - return json.loads(out.stdout) - - -# The shape of one shared library: a film, and a two-season show. Invented -# titles — the real one this was found against is nobody's business here. -def _library(group): - """`path` is already prefixed the way search-page.js prefixes it.""" - def entry(file_id, name, **kw): - return { - "id": file_id, "name": name, "type": "video", - "path": "__search__/shows", "size": 1, - "groupId": group, "groupName": group.upper(), "groupOwner": "someone", - "_tRef": f"t:{group}", "_gRef": f"g:{group}", "_connGen": 1, - **kw, - } - files = [entry("film1", "a-film.mkv", display_title="Some Film")] - for season in (1, 2): - for ep in (1, 2, 3): - files.append(entry( - f"s{season}e{ep}", f"show.s0{season}e0{ep}.mkv", - display_title="Some Saga", season=season, episode=ep)) - return files - - -def test_a_shared_library_is_listed_once(tmp_path, pipeline): - """ - The bug as reported: two groups, one directory, everything twice. - """ - both = _library("demo35") + _library("media") - grid = _grid(tmp_path, pipeline, both) - - assert [m["title"] for m in grid["movies"]] == ["Some Film"] - assert grid["movies"][0]["sources"] == 2 - - assert len(grid["shows"]) == 1 - show = grid["shows"][0] - assert [s["season"] for s in show["seasons"]] == [1, 2] - for season in show["seasons"]: - assert season["episodes"] == [ - f"S{season['season']}E{n}" for n in (1, 2, 3)], ( - "an episode is listed more than once — this is the reported bug, " - "in the season list under the synopsis") - - -def test_a_show_streams_from_one_source(tmp_path, pipeline): - """A season split across two nodes would open two connections and two - metadata lookups for one show.""" - both = _library("demo35") + _library("media") - show = _grid(tmp_path, pipeline, both)["shows"][0] - assert len(show["groups"]) == 1 - - -def test_the_operators_own_node_serves_it(tmp_path, pipeline): - """Both groups are on the operator's node in the reported case; when only - one is, that one is the source.""" - both = _library("remote") + _library("mine") - grid = _grid(tmp_path, pipeline, both, local=["mine"]) - assert grid["movies"][0]["groupId"] == "mine" - assert grid["shows"][0]["groups"] == ["mine"] - - -def test_one_group_is_unchanged(tmp_path, pipeline): - """The overwhelmingly common case: nothing to merge, nothing different.""" - grid = _grid(tmp_path, pipeline, _library("solo")) - assert [m["title"] for m in grid["movies"]] == ["Some Film"] - assert grid["movies"][0]["sources"] == 1 - assert grid["movies"][0]["groupId"] == "solo" - show = grid["shows"][0] - assert show["groups"] == ["solo"] - assert sum(len(s["episodes"]) for s in show["seasons"]) == 6 - - -def test_an_episode_only_one_group_has_is_kept(tmp_path, pipeline): - """ - Merging must never subtract. A group holding one extra episode contributes - it, whichever source the show settled on. - """ - extra = _library("media") - extra.append({ - "id": "s2e4", "name": "show.s02e04.mkv", "type": "video", - "path": "__search__/shows", "size": 1, - "groupId": "media", "groupName": "MEDIA", "groupOwner": "someone", - "display_title": "Some Saga", "season": 2, "episode": 4, - }) - grid = _grid(tmp_path, pipeline, _library("demo35") + extra) - season2 = [s for s in grid["shows"][0]["seasons"] if s["season"] == 2][0] - assert season2["episodes"] == ["S2E1", "S2E2", "S2E3", "S2E4"] |