diff options
| -rw-r--r-- | docs/apps.md | 40 | ||||
| -rw-r--r-- | docs/mediacenter.md | 43 | ||||
| -rw-r--r-- | docs/musicbay.md | 34 | ||||
| -rw-r--r-- | docs/photos.md | 23 | ||||
| -rw-r--r-- | docs/refactoring-search.md | 10 | ||||
| -rw-r--r-- | packages/meshbay-hub/src/meshbay_hub/api/webapp.py | 1 | ||||
| -rw-r--r-- | packages/meshbay-hub/tests/conftest.py | 27 | ||||
| -rw-r--r-- | packages/meshbay-hub/tests/test_asset_versioning.py | 27 |
8 files changed, 200 insertions, 5 deletions
diff --git a/docs/apps.md b/docs/apps.md index 1cd6339..ef8cc1a 100644 --- a/docs/apps.md +++ b/docs/apps.md @@ -80,6 +80,39 @@ needs — a new app does not get a bespoke prop list. Notable ones: | `transportRef`, `gekRef` | refs to the live MNP transport and the imported group key | never state — a ref, so reconnects don't force a re-render of every app | | `mayUpload` | `memberUpload || isNodeAdmin`, computed once | Files' toolbar and Chat's composer both gate on it; a second derivation would eventually disagree with the first | +### 2b. The same app, rendered by the Search page + +Videos, Music and Photos are mounted twice: by `group-page.js` for one group, +and by `search-page.js` across every group the reader belongs to. The second +caller passes the same prop shape, and the difference lives entirely on the +**entries**, in underscore-prefixed fields the group page never sets: + +| Field | What it is | +|---|---| +| `groupId`, `groupName`, `groupOwner` | which group serves this entry | +| `_tRef`, `_gRef` | that group's transport and key — read as `entry._tRef \|\| transportRef`, which is why a single-group mount needs no special case | +| `_connGen` | bumped when that group reconnects; use it as a refetch key so a tile recovers instead of staying a spinner | +| `_sources` | every group that has this file, after the de-duplication below | + +**A file shared by two groups is one entry, not two** (`source-merge.js`, +`docs/refactoring-search.md`). Entries are folded on their content hash and one +source is resolved per *unit* — a film, a show, an album — using each app's own +grouping function to decide what a unit is. Two consequences for a new app: + +- if it renders a group name, use **`SourceTag`** from `group-name.js` rather + than `entry.groupName`: a merged entry has several groups and must say + `N sources` instead of naming one. Pass it the **whole unit** (a show's + episodes, an album's tracks), not the entry the card was drawn from — that + entry is usually chosen for its thumbnail, and would under-report; +- if it needs a merge unit key of its own, add a `<name>Units()` helper to + `search-page.js` that calls the app's **exported** grouping function. Never + re-derive the keys there: a copy keeps agreeing until one of them changes, + and the symptom is a show whose episodes stream from two different nodes. + +The Files explorer is deliberately **not** merged — there each group is a +top-level folder, and merging would remove a file from one of them. +`test_search_files_unmerged.py` refuses a build that changes this. + An app that needs **local** state (Files' `selecting`/`sortKey`/`currentPath`, for instance) owns it itself with `useState`, same as before the split. One thing worth keeping if you add a tab with a notion of "current location within @@ -160,8 +193,11 @@ registry, so a newly-registered app gets a checkbox for free. 5. **`webapp.py`'s `_ASSETS`** tuple: add the new file. This is the cache-busting hash's input list — a file imported by the page but missing here can change without the served URL changing, which is the exact bug - class `test_asset_versioning.py` exists for. Forgetting this step is - silent: nothing errors, a browser just keeps an old copy. + class `test_asset_versioning.py` exists for. Forgetting this step used to be + silent: nothing errors, a browser just keeps an old copy. It is now caught — + `test_every_static_script_participates_in_the_fingerprint` holds `_ASSETS` + to every `.js` in `static/` (`sw.js` excepted, unversioned on purpose). + Written after `source-merge.js` shipped missing from the list. 6. **Test coverage that scans the file set**: `test_hook_ordering.py` (`STATIC_FILES`) and `test_transport_contracts.py` (`test_no_setter_survives_the_state_it_belonged_to`, `SPLIT_FILES`) walk a diff --git a/docs/mediacenter.md b/docs/mediacenter.md index dbdf85c..d714366 100644 --- a/docs/mediacenter.md +++ b/docs/mediacenter.md @@ -904,6 +904,49 @@ only when there is nothing else, and the lowest *number* rather than the first entry so it does not quietly depend on `buildSeasons` keeping its sort. `test_video_default_season.py` — no input it takes can carry a thumbnail. +### 10.6 The same film twice in the cross-group Search view (2026-09-02) + +An operator hosting two groups gave both the *same* video directory — which is +the point of having two groups: different people are invited to different +libraries, and one library may be in several of them. **Search files** then +showed every film as two poster cards and every episode twice in the season +list, one copy badged per group. Flat list too. + +Not a Videos bug. Inside a group it cannot happen: `GroupIndex` is keyed by +blake3, so the same bytes at two paths are already one entry. `search-page.js` +concatenates *N* independently keyed indexes into one list, and that is where +the duplication is born. + +`source-merge.js` folds entries on the content hash and resolves **one source +per unit** — a film, a whole show — rather than per file: a season split across +two nodes would open two connections and two metadata lookups for one show. A +group hosted by the reader's own node wins (read from `handshake_ack`'s +`is_node_admin`, which the node computes from its own record of who it belongs +to, never a hub claim); failing that the pick is `hash(unitKey + userId)`, +stable for one reader across renders and reloads — a source that changed +mid-stream would tear down the connection under a film that is playing — and +spread across readers. + +The units come from **`groupVideoEntries` itself**, called on the un-merged +list purely to learn them, never a second copy of its keys in the Search page. +A copy would keep agreeing until one of them changed, and the symptom would be +a show whose episodes stream from two different nodes. + +Two consequences worth knowing: + +- **An operator's "Fix match" and "Rematch" go to the chosen source's node.** + On the operator's own libraries that is their node, which is what they mean. + On a merged entry they do not host, the override lands on whichever group was + picked — and the other source keeps its own match. +- **`PosterGrid.mergedShows` still merges two differently-parsed titles once + both resolve to the same TMDB id.** Those were two units when the source was + picked, so a merged card can hold two sources. Left alone deliberately: + re-picking under a card the reader is looking at is worse than a mixed one. + +Full design, the adversary this names, and what must not change: +`docs/refactoring-search.md`. `test_search_source_merge.py` holds the rules, +`test_search_media_merge.py` holds this symptom end to end. + ## 11. Acceptance before shipping 1. Re-run the §3 validation (real TMDB calls, same corpus, same script diff --git a/docs/musicbay.md b/docs/musicbay.md index 2ca8b23..d9794d2 100644 --- a/docs/musicbay.md +++ b/docs/musicbay.md @@ -434,6 +434,40 @@ actually implement (§3.2), not a property the protocol gives for free. | M5 | Gapless playback, crossfade, lyrics, waveform seek preview | Nice-to-haves, no architectural prerequisite from this plan either way | | M6 | Photos app | Out of scope of this document, per `apps.md`'s own list — unaffected by anything here | +## 9b. One album, not two, in the cross-group Search view (2026-09-02) + +Reported against Videos and true here by construction: an operator hosting two +groups that share one directory saw every track listed twice inside one album. +Inside a group it cannot happen (`GroupIndex` is keyed by blake3); the Search +page concatenates *N* indexes, and that is where the duplication is born. + +The fix is `source-merge.js`, applied identically to all three media views — +fold on the content hash, resolve **one source per unit**. For Music the unit +is an album, and a track loose enough to carry no artist at all is a unit of +its own (or it would be dropped from the merge entirely, since +`groupMusicEntries` never puts it in an album bucket). + +**`foldKey` is exported for this**, and the reason is worth keeping. §5.1's +grouping folds case and `&`/`and` for the *key* while keeping the first-seen +spelling for display — and which group is seen first is whichever index +happened to arrive first. Keying a merge unit on the display strings would +therefore let the chosen source change between page loads. The folded key is +the one grouping actually uses, and is stable. + +The units come from `groupMusicEntries` itself, called on the un-merged list +purely to learn them — never a second copy of its keys in the Search page. + +**The player needed no change.** `music-player.js` resolves a connection from +`entry.groupId` (§2.3), and a merged entry carries exactly one. The queue built +in `search-page.js`'s `onPreview` needed none either: it filters by `groupId` +and is reachable only from the Files explorer, which is deliberately *not* +merged — there each group is a folder and merging would remove a track from +one of them. + +Design and the adversary it names: `docs/refactoring-search.md`. +`test_search_media_merge.py` covers the album cases, including a differently- +cased tag not splitting the unit. + ## 10. Acceptance before shipping 1. Tag-coverage measurement against a real local library (not committed — diff --git a/docs/photos.md b/docs/photos.md index ed4a826..c00a7e0 100644 --- a/docs/photos.md +++ b/docs/photos.md @@ -456,6 +456,29 @@ than Videos, not more. | P4 | Wrap-around vs. stop-at-ends for next/previous at album boundaries | UI choice, mirror whatever `video-player.js`'s own controls already do for consistency | | P5 | Album cover selection (always "first photo" vs. an operator/member choice) | Product call; "first photo, stable" is a reasonable, zero-config default and is what this document assumes | +## 10b. One photo, not two, in the cross-group Search view (2026-09-02) + +Reported against Videos and true here by construction: two groups sharing one +directory listed every photo twice inside one album. `source-merge.js` folds +entries on the content hash and resolves one source per album; the units come +from `groupPhotoAlbums` itself (exported for this), called on the un-merged +list purely to learn them. + +**One consequence is deliberate and is not a bug.** Albums are keyed by +directory (§2.5), so two groups whose roots have *different basenames* put the +same photo in two differently-named albums, and the merge — scoped to a unit — +leaves it in both. That is correct: they are two albums. Only same-named albums +collapse, which is the reported shape. Videos and Music do not have this case, +their units being title- and tag-based rather than path-based. + +The album card's source badge counts the union over **the album**, not the +cover photo: the cover is `photos.find((p) => p.thumb_hash) || photos[0]`, so +an album in two groups whose cover sits in only one would have claimed a single +source. + +Design: `docs/refactoring-search.md`. `test_search_media_merge.py` covers both +the collapsing and the non-collapsing case. + ## 11. Acceptance before shipping 1. Orientation correction verified against a real EXIF-rotated phone photo diff --git a/docs/refactoring-search.md b/docs/refactoring-search.md index 2210397..d39bd36 100644 --- a/docs/refactoring-search.md +++ b/docs/refactoring-search.md @@ -1,6 +1,7 @@ # Refactor: one file, one entry — merging duplicate sources in the Search view -> Status: **phases 1–8 built** (2026-09-02), 9 (the doc pass) outstanding. +> 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, @@ -335,6 +336,11 @@ be easy to ship and hard to notice. 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. --- @@ -352,7 +358,7 @@ Each phase is independently testable and leaves the tree working. | 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/` | +| 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. diff --git a/packages/meshbay-hub/src/meshbay_hub/api/webapp.py b/packages/meshbay-hub/src/meshbay_hub/api/webapp.py index 6dfd3ed..a1807ea 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/webapp.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/webapp.py @@ -34,6 +34,7 @@ _ASSETS = ("style.css", "keyderive.js", "crypto.js", "transport.js", "app.js", # app.js or group-page.js, so a change to any of them is a change # to what the browser must fetch. "icon.js", "file-utils.js", "hub-client.js", "apps.js", + "source-merge.js", "chat-app.js", "files-app.js", "video-player.js", "video-app.js", "music-app.js", "music-player.js", "photos-app.js", "group-settings.js", "group-page.js", diff --git a/packages/meshbay-hub/tests/conftest.py b/packages/meshbay-hub/tests/conftest.py index 2769f7e..cb595c5 100644 --- a/packages/meshbay-hub/tests/conftest.py +++ b/packages/meshbay-hub/tests/conftest.py @@ -96,3 +96,30 @@ def _skip_email_verification(monkeypatch): monkeypatch.setattr( "meshbay_hub.api.users._create_and_send_verification", _noop) monkeypatch.setattr("meshbay_hub.mail._send", lambda msg: True) + + +@pytest.fixture(autouse=True) +def _no_cleanup_task(monkeypatch): + """ + Do not run the maintenance loop under test. + + `create_app`'s lifespan starts `cleanup_loop` as an asyncio task, so every + test — each of which enters that lifespan — ran a purge pass concurrently + with its own requests. On SQLite `:memory:` that is not merely noisy: the + engine uses a **StaticPool**, one connection for the whole process, so the + request's session and the cleanup task's session interleave their + transactions on the *same* connection. A registration could commit and then + not be visible to the login three lines later, which surfaced as + `401 Invalid credentials` for an account created moments before, in about + one run of `test_node_ws_auth.py` in four. + + The purge itself is not at fault and this is not a production condition: + the DELETE was measured removing 0 rows, and PostgreSQL gives every session + its own connection. What is removed here is the second user of the shared + one. Tests that want the maintenance behaviour call the `purge_*` functions + directly, which is how they are covered. + """ + async def _noop(get_session): + return + + monkeypatch.setattr("meshbay_hub.tasks.cleanup.cleanup_loop", _noop) diff --git a/packages/meshbay-hub/tests/test_asset_versioning.py b/packages/meshbay-hub/tests/test_asset_versioning.py index e5889f0..3d84d99 100644 --- a/packages/meshbay-hub/tests/test_asset_versioning.py +++ b/packages/meshbay-hub/tests/test_asset_versioning.py @@ -23,7 +23,7 @@ import re import pytest from fastapi.testclient import TestClient -from meshbay_hub.api.webapp import ASSET_V, STATIC_DIR, _asset_version +from meshbay_hub.api.webapp import _ASSETS, ASSET_V, STATIC_DIR, _asset_version from meshbay_hub.app import create_app @@ -101,3 +101,28 @@ def test_the_fingerprint_follows_the_content(tmp_path, monkeypatch): finally: target.write_bytes(original) assert _asset_version() == before, "the fingerprint is not reproducible" + + +def test_every_static_script_participates_in_the_fingerprint(): + """ + `_ASSETS` is hand-maintained, and forgetting an entry fails silently: the + file is imported by the page, so the browser fetches it, but it does not + feed the content hash — so a change confined to that one file ships at the + URL a cache already holds. Nothing errors, and the symptom is a fix that + "doesn't work" on exactly the machines that visited before. + + Found by `source-merge.js`, added to the Search view and left out of the + list. It happened to be harmless that day because `search-page.js` changed + in the same commit and *is* listed — which is the worst way for this to go + unnoticed. The checklist in docs/apps.md §4 step 5 names this trap; this + enforces it instead of relying on remembering. + + `sw.js` is the one deliberate exclusion — the service worker is served + unversioned on purpose (`test_the_service_worker_is_not_versioned`). + """ + on_disk = {p.name for p in STATIC_DIR.glob("*.js")} - {"sw.js"} + missing = sorted(on_disk - set(_ASSETS)) + assert not missing, ( + f"static scripts missing from webapp._ASSETS: {missing}. A change to " + "one of these will not move the asset URL, so a browser that cached " + "the page keeps running the old copy") |