Merge branch 'refactor/groups-phase1'

Groups refactor, phases 1-3.

The root model replaces the old `upload` flag and group-wide `member_upload`
with per-root `writable`/`removable`/`ejected`, carried by a `RootSet` that
both front doors — the loopback API and signed MNP — reach through the same
`ops` functions. MNP goes to 1.1, additively: the roots table now rides on
`index_delta`, so a root added, removed, ejected or plugged reaches every
connected client instead of only whoever reloaded.

The group UI becomes a plugin architecture: an application is a registry
entry in `apps.js` plus its own files, with directories stored generically
by `ops.set_app_directories` under whatever the app is called. A reference
application, hidden behind `?dev=1`, is what makes that claim testable —
adding it is what found the two places still naming apps by hand.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us

1 files changed, 383 insertions, 0 deletions

diff --git a/packages/meshbay-hub/tests/test_app_settings_plugin.py b/packages/meshbay-hub/tests/test_app_settings_plugin.py
new file mode 100644
index 0000000..b8afc23
--- /dev/null
+++ b/packages/meshbay-hub/tests/test_app_settings_plugin.py
@@ -0,0 +1,383 @@
+"""
+Adding an application must not mean editing the pages that render it.
+
+That is the whole claim of the plugin architecture, and it is the kind of claim
+that decays silently: the first special case for one app reads as harmless, and
+by the third the loop is a lookup table with a default branch. These tests are
+what makes the claim checkable.
+
+They are source-reading, which is weak evidence and the only kind available for
+the SPA. Where a stronger check exists it is used instead — `test_spa_syntax`
+parses every module, and `test_hook_ordering` catches the temporal-dead-zone
+fault this refactor's new import graph could otherwise reintroduce.
+"""
+
+import re
+from pathlib import Path
+
+import pytest
+
+STATIC = Path(__file__).resolve().parents[1] / "src" / "meshbay_hub" / "static"
+APPS = STATIC / "apps.js"
+GROUP_SETTINGS = STATIC / "group-settings.js"
+GROUP_PAGE = STATIC / "group-page.js"
+SETTINGS_UI = STATIC / "settings-ui.js"
+FOLDER_TREE = STATIC / "folder-tree.js"
+TRANSPORT = STATIC / "transport.js"
+# parents[2] is `packages/` — the tests live at
+# packages/meshbay-hub/tests/, so [0] is tests, [1] the package, [2] packages.
+# Got this wrong once and the two cross-package checks below skipped silently,
+# which is worse than not having them: a green run that measured nothing.
+NODE_SERVER = (Path(__file__).resolve().parents[2] / "meshbay-node" / "src"
+               / "meshbay_node" / "transport" / "webrtc_server.py")
+
+PANES = ["chat-app-settings.js", "video-app-settings.js",
+         "music-app-settings.js", "photos-app-settings.js"]
+
+pytestmark = pytest.mark.skipif(not APPS.exists(),
+                                reason="SPA sources unavailable")
+
+
+def _component(source: str, name: str) -> str:
+    start = source.index(f"\nfunction {name}(")
+    end = source.find("\nfunction ", start + 1)
+    return source[start:end if end != -1 else len(source)]
+
+
+def _code_only(source: str) -> str:
+    """
+    The same source with comments removed.
+
+    A prose explanation of what moved out of a file is not the file naming an
+    app — and the note recording *why* TMDB is no longer here is exactly the
+    kind of comment this codebase wants kept. Crude on purpose: it does not
+    understand strings containing `//`, which for these files is fine and for
+    a parser would be a second implementation of one.
+    """
+    source = re.sub(r"/\*.*?\*/", "", source, flags=re.S)
+    return re.sub(r"^\s*//.*$", "", source, flags=re.M)
+
+
+# ── The registry is the only place an app is named ──────────────────────────
+
+def test_the_settings_page_names_no_application():
+    """
+    The apps loop renders whatever the registry holds. A branch on `'video'`
+    here is the first step back to the 1338-line page this replaced, where
+    every app's settings were inlined and the file grew with each one.
+    """
+    panel = _code_only(_component(GROUP_SETTINGS.read_text(encoding="utf-8"),
+                                  "GroupSettingsPanel"))
+    for app in ("'video'", "'music'", "'photo'",
+                "tmdb", "musicbrainz", "TMDB"):
+        assert app not in panel, (
+            f"group-settings.js still names {app} — an app's own settings "
+            f"belong in its settings file")
+
+
+def test_the_settings_page_renders_the_registry():
+    panel = _component(GROUP_SETTINGS.read_text(encoding="utf-8"),
+                       "GroupSettingsPanel")
+    assert "configurableApps()" in panel
+    assert "app.Settings" in panel, "the registry's component is not rendered"
+
+
+def test_every_registered_app_has_a_key_the_node_would_accept():
+    """
+    The registry key is the identifier everywhere: the tab, `apps_enabled`,
+    the `app_directories` op, and the roster row the directories live in. A
+    key here that `ALLOWED_APPS` does not have is an app whose settings are
+    refused by the node with no clue why.
+    """
+    node = NODE_SERVER
+    if not node.exists():
+        pytest.skip("the node package is not in this checkout")
+    m = re.search(r"ALLOWED_APPS = frozenset\(\{([^}]*)\}\)",
+                  node.read_text(encoding="utf-8"))
+    assert m, "ALLOWED_APPS moved"
+    allowed = set(re.findall(r"'([^']+)'|\"([^\"]+)\"", m.group(1)))
+    allowed = {a or b for a, b in allowed}
+
+    keys = set(re.findall(r"\{ key: '([^']+)'", APPS.read_text(encoding="utf-8")))
+    assert keys, "no app keys found — did the registry's shape change?"
+    assert keys <= allowed, (
+        f"registered apps the node would refuse: {sorted(keys - allowed)}")
+
+
+# ── One contract, every pane ────────────────────────────────────────────────
+
+@pytest.mark.parametrize("pane", PANES)
+def test_every_pane_takes_the_same_props(pane):
+    """
+    A pane that reached for something else would make the loop that renders
+    them conditional, which is the same thing as the page naming apps again.
+    """
+    source = (STATIC / pane).read_text(encoding="utf-8")
+    m = re.search(r"function \w+Settings\(\{([^}]*)\}\)", source)
+    assert m, f"{pane}: no settings component with a destructured props object"
+    props = {p.strip() for p in m.group(1).split(",") if p.strip()}
+    allowed = {"roots", "dirs", "settings", "saveDirectories",
+               "transport", "signFn"}
+    assert props <= allowed, (
+        f"{pane} takes props outside the shared contract: "
+        f"{sorted(props - allowed)}")
+
+
+@pytest.mark.parametrize("pane", PANES)
+def test_no_pane_imports_the_page_that_renders_it(pane):
+    """
+    `group-settings` → `apps` → a pane → `group-settings` is a cycle, and ES
+    modules answer it with a temporal-dead-zone ReferenceError at first render
+    rather than an import error — the component simply does not appear. That
+    is why the shared widgets live in `settings-ui.js`.
+    """
+    source = (STATIC / pane).read_text(encoding="utf-8")
+    assert "group-settings.js" not in source, (
+        f"{pane} imports the page that renders it — that is an import cycle")
+
+
+@pytest.mark.parametrize("pane", PANES)
+def test_every_pane_owns_its_own_busy_state(pane):
+    """
+    One shared flag would disable every section while any one of them saves,
+    and attribute one section's error message to another.
+    """
+    source = (STATIC / pane).read_text(encoding="utf-8")
+    assert "useSaver()" in source
+
+
+# ── The folder picker ───────────────────────────────────────────────────────
+
+def test_the_picker_asks_the_node_for_nothing():
+    """
+    The tree is built from paths the client already holds. A fetch here would
+    be a folder-browsing protocol, which this deliberately is not: what it
+    shows is what the group's index contains, and a folder the node never
+    indexed does not exist as far as the group is concerned.
+    """
+    source = FOLDER_TREE.read_text(encoding="utf-8")
+    for forbidden in ("hubFetch", "fetch(", "platform.node", "transport."):
+        assert forbidden not in source, (
+            f"folder-tree.js reaches for {forbidden} — it is meant to be "
+            f"derived from the index the client already has")
+
+
+def test_a_read_only_root_cannot_be_chosen_as_a_destination():
+    """
+    Chat's attachment folder is the one directory that gets written to, and
+    the node refuses a read-only root for it. Letting the picker offer one
+    would move that refusal to the moment somebody sends a file.
+    """
+    source = FOLDER_TREE.read_text(encoding="utf-8")
+    picker = _component(source, "FolderTreePicker")
+    assert "requireWritable" in picker
+    assert "root.writable" in picker, (
+        "writability is not consulted when deciding what is selectable")
+
+    chat = (STATIC / "chat-app-settings.js").read_text(encoding="utf-8")
+    assert "requireWritable=${true}" in chat, (
+        "Chat's directory picker does not require a writable root")
+
+
+def test_the_picker_can_clear_a_selection():
+    """
+    Confirming with nothing chosen is how an app's directories are unset, and
+    an OK disabled on an empty selection would make that impossible without
+    another control.
+    """
+    picker = _component(FOLDER_TREE.read_text(encoding="utf-8"),
+                        "FolderTreePicker")
+    ok = picker[picker.index("folder_tree.confirm") - 400:
+                picker.index("folder_tree.confirm")]
+    assert "disabled" not in ok
+
+
+# ── The generic op ──────────────────────────────────────────────────────────
+
+def test_the_directory_op_is_signed_and_names_its_app():
+    """
+    An operator shown "Media/Films" alone cannot tell which application is
+    about to be pointed at it, and two apps' challenges would be
+    indistinguishable — so the app is in the signed subject, and both sides
+    build it the same way.
+    """
+    transport = TRANSPORT.read_text(encoding="utf-8")
+    body = transport[transport.index("async setAppDirectories("):]
+    body = body[:body.index("\n  async ", 1)]
+    assert "admin_challenge" in body and "_authorizeAdminOp" in body
+    assert "${appKey}:${clean.join(',')}" in body
+
+    node = NODE_SERVER
+    if node.exists():
+        assert 'f"{app}:{\',\'.join(clean)}"' in node.read_text(encoding="utf-8"), (
+            "the node builds a different subject than the client signs")
+
+
+def test_the_page_performs_exactly_one_app_specific_operation():
+    """
+    Pointing an app at folders is what every app has, so the page does it.
+    Anything one app alone needs — a TMDB key, a link-preview switch — the
+    pane does with the transport it is given. An app that only wants
+    directories therefore touches neither file.
+    """
+    panel = _component(GROUP_SETTINGS.read_text(encoding="utf-8"),
+                       "GroupSettingsPanel")
+    calls = set(re.findall(r"transport\.(set\w+)\(", panel))
+    # The page's own settings, which belong to no app: which apps are enabled
+    # at all, and how hard the node works watching its disk.
+    page_level = {"setAppsEnabled", "setScanSettings"}
+    # Both are the same generic operation; the second is what a node too old
+    # for it understands, chosen by version rather than by app.
+    generic = {"setAppDirectories", "setAppDirectoriesLegacy"}
+    assert calls - page_level == generic, (
+        f"the settings page performs app-specific operations: "
+        f"{sorted(calls - page_level - generic)}")
+
+
+# ── The apps read a list ────────────────────────────────────────────────────
+
+@pytest.mark.parametrize("app,prop", [
+    ("video-app.js", "videoDirectories"),
+    ("music-app.js", "musicDirectories"),
+    ("photos-app.js", "photoDirectories"),
+])
+def test_each_app_takes_a_list_of_directories(app, prop):
+    """
+    Videos and Music took a single folder, so a library spread over two drives
+    could not be described at all — the operator's only recourse was to point
+    the app at a parent containing both, which pulls in everything else too.
+    """
+    source = (STATIC / app).read_text(encoding="utf-8")
+    assert prop in source
+    for singular in ("videoRoot", "audioRoot"):
+        assert singular not in source, (
+            f"{app} still reads {singular} — one shape per idea")
+
+
+def test_an_older_node_still_fills_the_lists():
+    """
+    A node speaking MNP 1.0 sends `video_root`, not `video_directories`.
+    Reading the missing plural as "nothing configured" would empty a working
+    Videos tab on every group hosted by a node that has not been upgraded.
+    """
+    page = GROUP_PAGE.read_text(encoding="utf-8")
+    block = page[page.index("setAppDirectories({"):]
+    block = block[:block.index("setChatDirectory")]
+    assert "ack.video_root" in block and "ack.audio_root" in block
+    assert "ack.photo_roots" in block
+
+
+def test_the_search_cache_reads_both_shapes():
+    """
+    The cross-group index cache lives in IndexedDB and outlives a deploy, so a
+    reader opening Search after this ships still has entries written by the
+    previous version. Reading only the new shape empties their results with
+    nothing to distinguish it from "nothing matched".
+    """
+    source = (STATIC / "search-page.js").read_text(encoding="utf-8")
+    fn = _component(source, "cachedDirs")
+    assert "legacyKey" in fn
+    assert "videoRoot" in source and "audioRoot" in source and "photoRoots" in source
+
+
+# ── Where a result is reported ──────────────────────────────────────────────
+
+def test_the_directory_result_is_reported_below_the_controls():
+    """
+    It was rendered first, between the section's intro and the table header —
+    above everything the eye had already moved past by the time it appeared.
+    It belongs under the button that caused it, which is the last thing in the
+    section.
+    """
+    table = _component(GROUP_SETTINGS.read_text(encoding="utf-8"),
+                       "SharedDirectoriesTable")
+    # Two of them: the early return for a group with no directories yet, and
+    # the real one. Both report, and both must report last — keyed on the
+    # wrapper rather than on `return`, of which there are more.
+    blocks = table.split('<div class="shared-directories-table">')[1:]
+    assert len(blocks) == 2, "the section's shape changed; this test is stale"
+
+    for block in blocks:
+        assert block.index("${addControls}") < block.index("${message}"), (
+            "the result is rendered above the Add button rather than below it")
+
+    with_table = next(b for b in blocks if "<table" in b)
+    assert with_table.index("<table") < with_table.index("${message}"), (
+        "the result is rendered above the table")
+
+
+def test_a_refusal_does_not_look_like_a_footnote():
+    """
+    Every message here was a `settings-hint` — dim grey body text — so "two
+    roots would both be called uploads" read as an aside about the section
+    rather than as the reason nothing happened.
+    """
+    table = _component(GROUP_SETTINGS.read_text(encoding="utf-8"),
+                       "SharedDirectoriesTable")
+    block = table[table.index("const message ="):]
+    block = block[:block.index("`;") + 2]
+    assert "error-msg" in block
+    assert "role=" in block and "alert" in block, (
+        "a refusal that appears after a click has to be announced, not just "
+        "drawn")
+
+
+# ── Saving, and being able to tell ──────────────────────────────────────────
+
+PANE_FLAGS = {"chat-app-settings.js": "dirty",
+              "video-app-settings.js": "dirsDirty",
+              "music-app-settings.js": "dirsDirty",
+              "photos-app-settings.js": "dirty"}
+
+
+@pytest.mark.parametrize("pane", sorted(PANE_FLAGS))
+def test_save_is_a_button_and_not_dim_text(pane):
+    """
+    It was `btn btn-small btn-secondary`, and there is no `.btn` rule in the
+    stylesheet at all — so it took `.btn-secondary`: no background, a
+    transparent border, dim grey text. Enabled it already looked like a
+    disabled control, and disabled it was that at 40% opacity.
+
+    "You cannot always click Save, you do not notice, and it does not work" is
+    one sentence describing all of that.
+    """
+    source = (STATIC / pane).read_text(encoding="utf-8")
+    assert 'class="app-save"' in source, f"{pane}'s Save is not the shared control"
+    assert "btn-secondary" not in source, (
+        f"{pane}'s Save is still styled as dim text")
+
+
+def test_the_disabled_state_is_visually_distinct():
+    css = (STATIC / "style.css").read_text(encoding="utf-8")
+    rule = css[css.index(".app-save {"):]
+    rule = rule[:rule.index("}", rule.index(".app-save:disabled")) + 1]
+    assert "var(--accent)" in rule, "an enabled Save has no fill"
+    disabled = rule[rule.index(".app-save:disabled"):]
+    assert "background: none" in disabled and "--text-dim" in disabled, (
+        "disabled differs from enabled by opacity alone, which is what made "
+        "it unreadable")
+
+
+def test_a_saved_setting_reaches_the_page_that_renders_the_pane():
+    """
+    Why Chat was the systematic case.
+
+    `_dispatch` resolves an admin ack against the pending request and returns —
+    right for an op whose caller already knows the value it chose. Chat's pane
+    calls `transport.setChatDirectory` itself, so nothing told `group-page`
+    anything: the node saved it, every *other* connected client learned it from
+    the broadcast, and the one that asked went on showing an unsaved-looking
+    draft. Clicking Save again just re-sent it.
+    """
+    transport = TRANSPORT.read_text(encoding="utf-8")
+    block = transport[transport.index("const BROADCAST_ACK_TYPES"):]
+    block = block[:block.index("]);") + 3]
+    for ack in ("chat_directory_ack", "chat_link_preview_ack",
+                "app_directories_ack"):
+        assert ack in block, f"{ack} is swallowed by its own request"
+
+    assert "_replayBroadcast" in transport
+    replay = transport[transport.index("function _replayBroadcast"):]
+    replay = replay[:replay.index("\n}") + 2]
+    for cb in ("_onChatDirectory", "_onChatLinkPreview", "_onAppDirectories"):
+        assert cb in replay, f"{cb} is never called for the requester"