From ea56b8c79538323875c00db2e7006b255f7cd494 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 17:48:36 +0200 Subject: fix(groups): finish Phase 1 — MNP root management, upload targets, eject state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the Phase 1 commit found the RO/RW model sound but three paths unfinished, each of which broke the flow the phase exists to deliver. Plus 29 test failures it introduced and no coverage for anything it added. Uploads went to the wrong directory. The node read a `root` field on file_upload that no client ever sent, so every upload landed in the first writable root while the Files toolbar offered its button based on the root being browsed — with two writable roots, uploading from one wrote into the other. Files now names the root it is showing; Chat names one chosen in the shell (an operator-configured directory arrives in Phase 2); the node refuses an unknown name rather than falling back, and refuses read-only and ejected roots by code. Shared directories were unreachable on the web. The table read its roots only from the loopback API, which resolves to "not available" in a browser, so the section rendered for nobody there — while the Uploads controls it replaced had worked — and the transport.updateRoot/ejectRoot/plugRoot methods beside it were dead. MNP is now the path, loopback the fallback for a local node with no live connection, and adding a root over MNP takes a typed path since no web page can browse a remote disk. Ejecting updated nobody's screen. transport.js resolves an admin ack against the pending request and returns, which is right for every op whose caller knows the value it chose; the root acks carry state only the node can compute, so the operator who clicked Eject was the one client that never saw it happen. And the ejected flag reached roster.db but was never read back, so a restart undid it and the next scan read an empty mount point as an erased library. Also: the member-upload endpoint answered 200 and did nothing (removed); the wizard ignored the first root's RW switch; reload compared roots on name and path, so editing writable in node.toml did nothing; the table had no path column, which is the only thing separating two libraries sharing a basename; apps_enabled normalisation differed between the two sides of a signed subject. Tests: eject/plug, per-root upload refusal and the node.toml rewrite had no coverage at all. test_member_upload_policy.py is replaced by test_root_writable_policy.py — it tested a removed feature — and every property worth keeping from it moved rather than being dropped. Docs: draft-v6 structural decision 9 is annotated as superseded (the operator can no longer have a directory only they may write to — a real capability removed, flagged rather than hidden), the man page documents the root verb and the RO/RW fields, and refactor-groups.md §7b records what the plan got wrong. Suite: 41 failures before, 13 after — all 13 pre-existing on main. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../tests/test_upload_controls_hidden.py | 224 +++++++++++++++------ 1 file changed, 163 insertions(+), 61 deletions(-) (limited to 'packages/meshbay-hub/tests/test_upload_controls_hidden.py') diff --git a/packages/meshbay-hub/tests/test_upload_controls_hidden.py b/packages/meshbay-hub/tests/test_upload_controls_hidden.py index 94917d2..ae1f444 100644 --- a/packages/meshbay-hub/tests/test_upload_controls_hidden.py +++ b/packages/meshbay-hub/tests/test_upload_controls_hidden.py @@ -1,14 +1,22 @@ """ -When the operator closes uploading, the controls go — both of them. +When a directory is read-only, the controls that write to it go — both of them. -There are two ways to put a file into a group and they are in different +There are two ways to put a file into a group and they live in different components: the Upload button in the Files toolbar, and the paperclip in the chat composer. Hiding one and forgetting the other is the obvious mistake, and -the second one is the easier to forget because it does not look like an upload. +the paperclip is the easier to forget because it does not look like an upload. Nothing here is a security property. **The node refuses the upload** — that is -`test_member_upload_policy.py` in the node package. This is about not offering -somebody a button whose only outcome is an error message. +`test_root_writable_policy.py` and `test_security_regressions.py` in the node +package. This is about not offering somebody a button whose only outcome is an +error message. + +What the RO/RW refactor changed: there is no group-wide answer any more. Files +uploads into *the root being browsed*, so its button follows that root's +`writable`. Chat has no folder on screen, so the shell picks one for it. The +two therefore read different things on purpose, and the tests below pin that +each reads the right one — a stronger claim than the old "both read one +boolean", which is why that assertion is gone rather than adapted. """ import re @@ -17,10 +25,6 @@ from pathlib import Path import pytest STATIC = Path(__file__).resolve().parents[1] / "src" / "meshbay_hub" / "static" -# The group-page refactor split what used to be one app.js into one file per -# "application" plus the group shell. mayUpload itself is still derived once, -# in the shell (group-page.js) — Files and Chat each moved to their own file -# and receive it as a prop, the same shape ChatPanel already took. APP = STATIC / "app.js" GROUP_PAGE = STATIC / "group-page.js" FILES_APP = STATIC / "files-app.js" @@ -36,45 +40,93 @@ def app() -> str: return GROUP_PAGE.read_text(encoding="utf-8") -def _component(app: str, name: str) -> str: - start = app.index(f"\nfunction {name}(") - end = app.find("\nfunction ", start + 1) - return app[start:end if end != -1 else len(app)] +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)] # ── Both controls ─────────────────────────────────────────────────────────── def test_the_files_toolbar_hides_its_upload_button(): + """ + Gated on the root being browsed, not on a group-wide answer: with one + writable root and one read-only one, a single boolean would offer the + button in both and produce a refusal in one of them. + """ page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") toolbar = page[page.index("file-toolbar"):] toolbar = toolbar[:toolbar.index("breadcrumbs")] - assert "mayUpload &&" in toolbar, "the Upload button is offered regardless" + assert "currentRootWritable" in toolbar, ( + "the Upload button is offered regardless of the directory's own flag") + + +def test_the_files_upload_button_is_not_offered_at_the_top_of_a_group(): + """ + The top level is the set of roots, which is the operator's configuration + and not a directory on anyone's disk. There is nothing to upload *into* + there, and no root name to give the node. + """ + page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") + toolbar = page[page.index("file-toolbar"):] + toolbar = toolbar[:toolbar.index("breadcrumbs")] + assert "currentPath &&" in toolbar def test_the_chat_composer_hides_its_paperclip(): chat = _component(CHAT_APP.read_text(encoding="utf-8"), "ChatPanel") composer = chat[chat.index("chat-input-row"):] - assert "mayUpload &&" in composer, ( + assert "attachRoot ?" in composer, ( "the chat attachment is the second way in and is still offered") -def test_both_read_the_same_answer(app): - """Two derivations would eventually disagree, and the disagreement would - be one of them offering an upload the node refuses.""" - assert re.search(r"const mayUpload = memberUpload \|\| isNodeAdmin;", app), ( - "mayUpload is no longer derived in one place") - # Files and Chat both receive it from the same `commonProps` object the - # shell spreads into whichever app tab is active — one derivation feeding - # one object, rather than two hand-written prop attributes that could - # drift apart. - props = app[app.index("const commonProps = {"):app.index("return html`")] - assert "mayUpload," in props or "mayUpload:" in props, ( - "mayUpload is not in the shared props object every app receives") +def test_the_paperclip_says_why_rather_than_vanishing(): + """ + A control that disappears leaves the reader no way to find out what would + bring it back. A group with no writable directory is a state an operator + can fix, so it is worth naming. + """ + chat = _component(CHAT_APP.read_text(encoding="utf-8"), "ChatPanel") + composer = chat[chat.index("chat-input-row"):] + assert "chat.attach_read_only" in composer + +# ── One derivation, in the shell ──────────────────────────────────────────── -def test_the_operator_keeps_their_own_controls(app): - assert "memberUpload || isNodeAdmin" in app, ( - "turning uploads off would hide the operator's own upload button") +def test_the_attachment_directory_is_decided_once(app): + """ + Two derivations would eventually disagree, and the disagreement would be + one of them offering an upload the node refuses. + """ + assert re.search(r"const attachRoot = ", app), ( + "attachRoot is no longer derived in one place") + props = app[app.index("const commonProps = {"):app.index("return html`")] + assert "attachRoot," in props or "attachRoot:" in props, ( + "attachRoot is not in the shared props object every app receives") + + +def test_an_unavailable_root_is_not_offered_as_a_destination(app): + """ + `writable` is configuration and stays true while a drive is unplugged or + ejected. Offering it anyway produces a refusal from the node with no + explanation on screen. + """ + block = app[app.index("const writableRoots"):] + block = block[:block.index("const attachRoot")] + assert "available" in block + + +def test_files_uploads_into_the_root_it_is_showing(): + """ + The client has to name the destination now, because the node cannot choose + between several writable roots without guessing — and a guess here means a + file landing in a directory nobody was looking at. + """ + page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") + upload = page[page.index("const uploadFile"):] + upload = upload[:upload.index("const makeDirectory")] + assert "root: uploadRoot" in upload, "the node is left to choose" + assert "currentPath.split('/')[0]" in upload # ── Learning the answer ───────────────────────────────────────────────────── @@ -82,54 +134,104 @@ def test_the_operator_keeps_their_own_controls(app): def test_the_answer_comes_from_the_node(app): """Not from the hub, which has no say in what may be written to someone else's disk, and no way to be believed about it.""" - assert "ack.member_upload !== false" in app, ( - "the handshake ack is what carries this") - assert "hubFetch" not in app[app.index("ack.member_upload") - 400: - app.index("ack.member_upload")] + assert "if (indexMsg.roots) setNodeRoots(indexMsg.roots)" in app, ( + "the roots table in the index payload is what carries this") + idx = app.index("setNodeRoots(indexMsg.roots)") + assert "hubFetch" not in app[idx - 400:idx] def test_an_older_node_is_treated_as_permissive(app): - """A node that predates the setting sends no such field. Reading a missing - field as "off" would close every group on the older half of the network.""" + """ + A node speaking MNP 1.0 sends roots with no `writable` at all, plus the old + group-wide flag. Reading a missing field as "read-only" would close every + group on the older half of the network. + """ + assert "ack.member_upload !== false" in app assert "!== false" in app[app.index("ack.member_upload"): app.index("ack.member_upload") + 60] + block = app[app.index("const legacyNode"):] + block = block[:block.index("const commonProps")] + assert "writable === undefined" in block, ( + "nothing distinguishes a 1.0 node from one with no writable roots") def test_a_change_reaches_people_already_connected(app): - """The operator may be someone else entirely, changing it while you have - the group open. A button that survives until the next reconnection is a - button somebody presses.""" - assert "transport.onUploadPolicy" in app + """ + The operator may be someone else entirely, ejecting a drive while you have + the group open. A file list that survives until the next reconnection is a + list somebody clicks. + """ + assert "transport.onRootsChanged" in app transport = TRANSPORT.read_text(encoding="utf-8") - assert "member_upload_ack" in transport, "nothing routes the node's notice" + assert "root_eject_ack" in transport, "nothing routes the node's notice" + +def test_the_notice_also_answers_the_operators_own_request(): + """ + The same message is both a broadcast and the reply to the request that + caused it. -def test_the_notice_still_answers_the_operators_own_request(app): - """The same message is both a broadcast and the reply to the request that - caused it — returning early on it would leave that request hanging until it - timed out.""" + Every other admin ack can be resolved and dropped, because its caller + already knows what it asked for and updates local state from that. The root + acks carry a whole table only the node can compute — availability, the name + it settled on, the eject a failed plug left in place — so resolving one + without handing it on left the operator who clicked Eject as the only + client that never saw it happen. + """ transport = TRANSPORT.read_text(encoding="utf-8") - # Scoped to member_upload_ack's own handler, not everything up to the next - # occurrence of "index_sync" — other handlers with their own, legitimate - # early `return` (index_progress, set_scan_settings_ack: neither is ever a - # reply anyone awaits) now sit between the two in the file. - block = transport[transport.index("member_upload_ack"):] - block = block[:block.index("apps_enabled_ack")] - assert "return" not in block + block = transport[transport.index("msg.type.endsWith('_ack')"):] + block = block[:block.index("_uploaders")] + assert "ROOT_ACK_TYPES" in block and "_onRootsChanged" in block, ( + "the initiating client resolves the ack and learns nothing from it") # ── Changing it ───────────────────────────────────────────────────────────── -def test_changing_it_is_signed(app): +def test_changing_a_root_is_signed(): transport = TRANSPORT.read_text(encoding="utf-8") - method = transport[transport.index("async setMemberUpload("):] - method = method[:method.index("\n async ", 1)] - assert "admin_challenge" in method and "_authorizeAdminOp" in method, ( - "an unsigned instruction would let any member turn uploads back on") + for method in ("updateRoot", "ejectRoot", "plugRoot"): + body = transport[transport.index(f"async {method}("):] + body = body[:body.index("\n async ", 1)] + assert "admin_challenge" in body and "_authorizeAdminOp" in body, ( + f"{method} is unsigned — any member could use it") def test_only_the_operator_is_offered_the_setting(): - panel = _component(GROUP_SETTINGS.read_text(encoding="utf-8"), "GroupSettingsPanel") - section = panel[panel.index("members.uploads_title") - 400: - panel.index("members.uploads_title")] - assert "isNodeAdmin && connected" in section + panel = _component(GROUP_SETTINGS.read_text(encoding="utf-8"), + "GroupSettingsPanel") + section = panel[panel.index("settings_node.shared_directories_title") - 600: + panel.index("settings_node.shared_directories_title")] + assert "isNodeAdmin &&" in section + + +def test_the_operator_is_offered_it_on_the_web_too(): + """ + An operator is not necessarily sitting at their node. The first version of + this section required the loopback API, which resolves to "not available" + in a browser — so it rendered for nobody on the web, while the upload + controls it replaced had worked there. + """ + source = GROUP_SETTINGS.read_text(encoding="utf-8") + panel = _component(source, "GroupSettingsPanel") + section = panel[panel.index("settings_node.shared_directories_title") - 600: + panel.index("settings_node.shared_directories_title")] + assert "connected ||" in section, ( + "the shared directories section still requires a local node") + + table = _component(source, "SharedDirectoriesTable") + for call in ("transport.updateRoot", "transport.ejectRoot", + "transport.plugRoot", "transport.removeRoot", + "transport.addRoot"): + assert call in table, f"{call} has no MNP route from the table" + + +def test_the_roots_shown_come_from_the_live_connection_when_there_is_one(): + """ + The loopback list is a second source, and the two drift: it is read once on + mount and after a change, while the MNP one is pushed. Preferring MNP also + keeps this table on the same data Files reads, so an eject shows in both at + the same instant. + """ + panel = _component(GROUP_SETTINGS.read_text(encoding="utf-8"), + "GroupSettingsPanel") + assert "const effectiveRoots = (connected && mnpRoots" in panel -- cgit v1.2.3 From f3fb449f3a943096a2569dc383f2819a612bccd5 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 22:06:45 +0200 Subject: feat(node): an upload lands in the folder it was sent to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no `uploads/` subdirectory any more, and the client names the folder rather than the root. It was the last of v5's quarantine — the per-user layer went on 2026-08-14 for the same reason — and it goes on the same grounds: a folder appearing beside the operator's library because somebody sent a file is the node deciding how their disk is arranged. Somebody dropping a file into the folder they are looking at expects it to be in that folder. **What made the quarantine worth having was never the subdirectory.** It is the filename allowlist, the size cap, the chunk ordering and the no-overwrite rule, and all four are untouched: an existing file is never replaced, the second sender of IMG_1234.jpg gets a free name, and the check still sits at the write. Letting the client choose the destination is safe for one reason and only one: it is resolved through `RootSet.resolve()`, which refuses `..`, absolute segments and anything whose resolved form escapes its root, symlinks included. A member answers "which of this group's folders", never "which path on the operator's disk" — and the test that used to assert the node chose now asserts that, with six shapes of escape. `direct` goes with it. Its only job was to say "no subdirectory for this root", which is now every root, and a config flag that does nothing is worse than none. Chat's attachment folder finally does something: the directory the operator picks in the Chat settings pane is where attachments are written, falling back to the first writable root while they have not chosen one, or if the one they chose has since been made read-only or ejected — a stale choice should not become a refusal at send time. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- docs/meshbay-draft-v6.md | 14 +++- .../meshbay-hub/src/meshbay_hub/static/chat-app.js | 10 ++- .../src/meshbay_hub/static/files-app.js | 13 ++-- .../src/meshbay_hub/static/group-page.js | 13 +++- .../src/meshbay_hub/static/transport.js | 18 +++-- .../tests/test_upload_controls_hidden.py | 7 +- packages/meshbay-node/src/meshbay_node/config.py | 3 +- packages/meshbay-node/src/meshbay_node/ops.py | 3 +- packages/meshbay-node/src/meshbay_node/roots.py | 6 +- .../src/meshbay_node/transport/webrtc_server.py | 55 +++++++++---- .../tests/test_root_writable_policy.py | 8 +- .../tests/test_security_regressions.py | 90 +++++++++++++--------- 12 files changed, 156 insertions(+), 84 deletions(-) (limited to 'packages/meshbay-hub/tests/test_upload_controls_hidden.py') diff --git a/docs/meshbay-draft-v6.md b/docs/meshbay-draft-v6.md index 4439cd1..28aea0c 100644 --- a/docs/meshbay-draft-v6.md +++ b/docs/meshbay-draft-v6.md @@ -74,12 +74,22 @@ v5 confines uploads to `shared_root/uploads/` with a filename allowlist, no overwrite, chunk ordering and a size cap. All four protections stand. Two amendments: -- There is no single `shared_root`. **Each root is read-only or read-write**, and the - quarantine lives inside whichever writable root the upload is addressed to. If that +- There is no single `shared_root`. **Each root is read-only or read-write**, and an + upload goes to the folder the sender is looking at, inside a writable root. If that root is unavailable the upload fails with a stated reason and never falls back to another; if the group has no writable root, uploads are refused rather than guessed. (Amended 2026-09-06 — the original text designated *one* root as the upload destination, and the client named none. See `docs/refactor-groups.md` §1.1.) +- **There is no `uploads/` quarantine directory any more** (2026-09-06). It was the + last of v5's, the per-user layer having gone on 2026-08-14, and it went for the same + reason: a folder appearing beside the operator's library because somebody sent a + file is the node deciding how their disk is arranged. **What made the quarantine + worth having was never the subdirectory** — it is the filename allowlist, the size + cap, the chunk ordering and the no-overwrite rule, and all four are unchanged. + The client now names the destination folder, which is safe for one reason and only + one: it is resolved through `RootSet.resolve()`, which refuses `..`, absolute + segments and anything escaping its root, symlinks included. A member answers "which + of this group's folders", never "which path on the operator's disk". - **The no-overwrite rule is unchanged and still holds on exFAT/NTFS.** An earlier draft claimed a string comparison let `README.TXT` land on `readme.txt` there. It does not: the check is `Path.exists()`, and `stat()` is itself case-insensitive on those diff --git a/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js b/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js index 4714674..0a7ef26 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js @@ -204,7 +204,8 @@ function ChatImage({ filename, entries, transportRef, gekRef }) { // root is read-only, or the one drive that was writable is unplugged — and the // paperclip says so rather than producing a refusal from the node. function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, - onPreview, attachRoot = '', onActivity, status }) { + onPreview, attachRoot = '', attachDir = '', + onActivity, status }) { const [messages, setMessages] = useState([]); const [hasMore, setHasMore] = useState(false); const [loadingOlder, setLoadingOlder] = useState(false); @@ -484,7 +485,10 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, try { // Two people sending IMG_1234.jpg both succeed; the node picks a free name // and the message has to point at the one it chose. - const ack = await transport.uploadFile(file, { root: attachRoot }); + // `attachDir` is the folder the operator chose in Settings; `attachRoot` + // is the fallback for a group where they have not chosen one yet. + const ack = await transport.uploadFile( + file, { root: attachRoot, dir: attachDir || undefined }); const storedAs = (ack && ack.stored_as) || file.name; await new Promise(r => setTimeout(r, 2500)); if (onRefreshIndex) await onRefreshIndex(); @@ -506,7 +510,7 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, } finally { setAttaching(false); } - }, [username, onRefreshIndex, jumpToBottom, attachRoot]); + }, [username, onRefreshIndex, jumpToBottom, attachRoot, attachDir]); const onKeyDown = useCallback((e) => { if (e.key === 'Enter' && !e.shiftKey) { diff --git a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js index a6a3ca4..9ca3aae 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js @@ -75,12 +75,12 @@ function FilesPanel({ e.target.value = ''; const transport = transportRef.current; if (!files.length || !transport || !transport.connected) return; - // The root being browsed is the destination. A group can have several - // writable roots, so leaving the node to pick one means a file uploaded - // from a folder the operator is looking at lands in a different one — - // which is only noticed much later, if at all. - const uploadRoot = currentPath ? currentPath.split('/')[0] : ''; - if (!uploadRoot) return; + // The folder on screen is the destination — not its root, and not a + // subdirectory of the node's invention. Somebody dropping a file into the + // folder they are looking at expects it to be in that folder. + const uploadDir = currentPath; + if (!uploadDir) return; + const uploadRoot = uploadDir.split('/')[0]; setError(''); for (const file of files) { @@ -92,6 +92,7 @@ function FilesPanel({ onProgress: (sent) => onProgress(sent, file.size), signal, root: uploadRoot, + dir: uploadDir, }); // The node re-indexes on a filesystem event, so there is nothing to // wait on but the clock. Refreshing here means the file appears in diff --git a/packages/meshbay-hub/src/meshbay_hub/static/group-page.js b/packages/meshbay-hub/src/meshbay_hub/static/group-page.js index 513792b..a1e6411 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/group-page.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/group-page.js @@ -590,7 +590,16 @@ function GroupPage({ groupId, group, token, username, userId, userPrefs, [nodeRoots]); const legacyNode = nodeRoots.length > 0 && nodeRoots.every((r) => r.writable === undefined); - const attachRoot = writableRoots.length ? writableRoots[0].name + // The operator's chosen attachment folder wins where there is one — that is + // what the Chat settings pane is for. Its root has to be writable and + // present, or the choice is stale (they made it read-only, or ejected the + // drive) and the fallback is better than a refusal at send time. + const chatDirRoot = chatDirectory ? chatDirectory.split('/')[0] : ''; + const chatDirUsable = Boolean( + chatDirRoot && writableRoots.some((r) => r.name === chatDirRoot)); + const attachDir = chatDirUsable ? chatDirectory : ''; + const attachRoot = chatDirUsable ? chatDirRoot + : writableRoots.length ? writableRoots[0].name : (legacyNode && memberUpload ? (nodeRoots.find((r) => r.upload) || nodeRoots[0]).name : ''); @@ -655,7 +664,7 @@ function GroupPage({ groupId, group, token, username, userId, userPrefs, groupId, transportRef, gekRef, status, username, entries, availableEntries, nodeDirs, nodeRoots, setEntries, setNodeDirs, setNodeRoots, applyIndex, - isNodeAdmin, operatorPaired, attachRoot, userId, setError, onPreview, + isNodeAdmin, operatorPaired, attachRoot, attachDir, userId, setError, onPreview, onRefreshIndex: refreshIndex, onActivity: touchActivity, // Plural everywhere: Videos and Music read a list now, and Photos always // did. The scalar `videoRoot`/`audioRoot` shapes survive only on the wire, diff --git a/packages/meshbay-hub/src/meshbay_hub/static/transport.js b/packages/meshbay-hub/src/meshbay_hub/static/transport.js index 32f8539..3acfd20 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/transport.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/transport.js @@ -1778,13 +1778,18 @@ class MeshBayTransport { * free one rather than replacing anything. The ack says which, and that is what * this returns. * - * `root` names which shared directory to upload into — a name, never a path; - * the node picks the destination inside it. Since a group can have several - * writable roots, leaving it out is a guess, and the node's fallback ("the - * first writable one") exists only for MNP 1.0 clients, which had exactly one - * destination. Every caller here browses a root and knows which one it is. + * `dir` names the folder to upload into, as a virtual path + * (`Media/Films/1999`) — where the sender is actually looking. The node + * resolves it against the group's own roots, which refuses `..`, absolute + * segments and anything escaping its root; it is a place among the group's + * folders, never a path on the operator's filesystem. + * + * `root` is the older, coarser form: the root's name and nothing below it. + * Kept because a node that predates `dir` reads it, and because Chat has no + * folder on screen to name. Omitting both leaves the node to pick, which it + * only does for a client old enough to have had one destination. */ - async uploadFile(file, { chunkSize, onProgress, signal, root } = {}) { + async uploadFile(file, { chunkSize, onProgress, signal, root, dir } = {}) { // The same file twice at once would confuse the node, which keys its own // upload state by name — and would race for the same destination. if (this._uploaders.has(file.name)) { @@ -1836,6 +1841,7 @@ class MeshBayTransport { total_chunks: total, data: buf, ...(root ? { root } : {}), + ...(dir ? { dir } : {}), }); } while (acked < total) { diff --git a/packages/meshbay-hub/tests/test_upload_controls_hidden.py b/packages/meshbay-hub/tests/test_upload_controls_hidden.py index ae1f444..7b8b0e3 100644 --- a/packages/meshbay-hub/tests/test_upload_controls_hidden.py +++ b/packages/meshbay-hub/tests/test_upload_controls_hidden.py @@ -125,8 +125,11 @@ def test_files_uploads_into_the_root_it_is_showing(): page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") upload = page[page.index("const uploadFile"):] upload = upload[:upload.index("const makeDirectory")] - assert "root: uploadRoot" in upload, "the node is left to choose" - assert "currentPath.split('/')[0]" in upload + assert "dir: uploadDir" in upload, "the node is left to choose the folder" + assert "const uploadDir = currentPath" in upload, ( + "the destination is not the folder on screen") + assert "root: uploadRoot" in upload, ( + "a node too old for `dir` reads `root`, and gets nothing without it") # ── Learning the answer ───────────────────────────────────────────────────── diff --git a/packages/meshbay-node/src/meshbay_node/config.py b/packages/meshbay-node/src/meshbay_node/config.py index 4712312..7673a51 100644 --- a/packages/meshbay-node/src/meshbay_node/config.py +++ b/packages/meshbay-node/src/meshbay_node/config.py @@ -184,7 +184,6 @@ class RootSpec: kind: str = "generic" # generic|video|audio|photo — a view hint, unused for now writable: bool = False # RW roots accept uploads from group members removable: bool = False # operator can eject this root before unplugging the device - direct: bool = False # uploads land at root path, not in a subdirectory @dataclass @@ -225,7 +224,7 @@ class GroupConfig: for r in self.roots: r.writable = False self.roots.append(RootSpec( - path=self.upload_dir.strip(), writable=True, direct=True)) + path=self.upload_dir.strip(), writable=True)) @dataclass diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index 5b10452..3dfdc23 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -679,8 +679,7 @@ async def add_root(state: dict, group_id: str, path: str, *, from meshbay_node.config import RootSpec cfg.roots.append(RootSpec( path=str(added.path), name=added.name, kind=added.kind, - writable=added.writable, removable=added.removable, - direct=added.direct)) + writable=added.writable, removable=added.removable)) # Deliberately *not* mutating the live RootSet in place. # diff --git a/packages/meshbay-node/src/meshbay_node/roots.py b/packages/meshbay-node/src/meshbay_node/roots.py index 9d3f7cb..ffe801c 100644 --- a/packages/meshbay-node/src/meshbay_node/roots.py +++ b/packages/meshbay-node/src/meshbay_node/roots.py @@ -122,7 +122,6 @@ class Root: kind: str = "generic" writable: bool = False removable: bool = False - direct: bool = False ejected: bool = False available: bool = True @@ -226,8 +225,7 @@ class RootSet: writable=writable, removable=bool(spec.get("removable", False)), ejected=bool(spec.get("ejected", False)), - available=not bool(spec.get("ejected", False)), - direct=bool(spec.get("direct", False))) + available=not bool(spec.get("ejected", False))) _refuse_nesting(root, roots) roots.append(root) by_folded[root.folded] = root @@ -370,8 +368,6 @@ class RootSet: "ejected": r.ejected, # Backward compat for MNP 1.0 clients "upload": r.writable} - if r.direct: - d["direct"] = True out.append(d) return out diff --git a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py index d341d8c..61458f2 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -214,7 +214,6 @@ JOIN_FAILURE_WINDOW = 600 # seconds # attachments from the chat alike. One visible directory the operator can look # into, back up or empty — rather than a hidden tree of per-user uuids that # nobody could read, or files scattered wherever someone happened to be looking. -UPLOAD_DIR_NAME = "uploads" def _extract_dtls_fingerprint(sdp: str) -> bytes: @@ -4016,7 +4015,12 @@ class WebRTCPeerSession: # later — the same reason the old single upload root was never guessed. # A client that names nothing is an MNP 1.0 one, and there was exactly # one destination in its world: the first writable root. - target_root_name = str(msg.get("root") or "").strip() + # `dir` is the folder being browsed, as a virtual path + # (`Media/Films/1999`); `root` is the older, coarser form and is what + # its first segment means on its own. + target_rel = str(msg.get("dir") or "").strip().strip("/") + target_root_name = (target_rel.split("/")[0] if target_rel + else str(msg.get("root") or "").strip()) upload_root = None if target_root_name: upload_root = roots.by_name(target_root_name) @@ -4052,18 +4056,43 @@ class WebRTCPeerSession: "filename": filename}) return - if upload_root.direct: - rel_dir = upload_root.name - target_dir = upload_root.path + # The folder the sender is looking at, and no subdirectory of the node's + # invention. + # + # Uploads used to be confined to `/uploads/`, created on demand. + # That was the last of v5's quarantine (the per-user layer went on + # 2026-08-14, for the same reason): a shared directory nobody can + # organise is not a shared directory, and a folder appearing beside the + # operator's library because somebody sent a file is the node deciding + # how their disk is arranged. + # + # What made the quarantine worth having is not the subdirectory — it is + # the filename allowlist, the size cap, the chunk ordering, and the + # no-overwrite rule below. All four are unchanged. + # + # `resolve()` and not a join: it refuses `..`, absolute segments and + # anything whose resolved form escapes its root, symlinks included. The + # client names *where among the group's own folders*, never a path on + # the operator's filesystem. + if target_rel: + target_dir = roots.resolve(target_rel) + if target_dir is None or not target_dir.is_dir(): + self._send({"type": "error", + "detail": "Not a directory in this group", + "code": "no_such_directory", + "filename": filename}) + return + rel_dir = target_rel else: - rel_dir = f"{upload_root.name}/{UPLOAD_DIR_NAME}" - target_dir = upload_root.path / UPLOAD_DIR_NAME - try: - target_dir.mkdir(parents=True, exist_ok=True) - except OSError as e: - log.warning("Cannot create upload folder in root %r: %s", - upload_root.name, e) - self._send({"type": "error", "detail": "Upload folder unavailable", + # An MNP 1.0 client names nothing; the root itself is where its one + # destination now is. + target_dir = upload_root.path + rel_dir = upload_root.name + if not target_dir.is_dir(): + self._send({"type": "error", + "detail": f"Directory '{upload_root.name}' is " + f"currently unavailable", + "code": "root_unavailable", "filename": filename}) return diff --git a/packages/meshbay-node/tests/test_root_writable_policy.py b/packages/meshbay-node/tests/test_root_writable_policy.py index da95032..d7f2666 100644 --- a/packages/meshbay-node/tests/test_root_writable_policy.py +++ b/packages/meshbay-node/tests/test_root_writable_policy.py @@ -62,14 +62,16 @@ def _session(tmp_path: Path, user_id: str, *, def _upload(session, filename="clip.mp4", body=b"bytes"): session._do_file_upload({ - "filename": filename, "root": "shared", + "filename": filename, "dir": "shared", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(body).decode(), }) def _uploads_dir(session) -> Path: - return session._ctx["roots"].roots[0].path / "uploads" + # The root itself: the `uploads/` subdirectory the node used to create is + # gone (see test_security_regressions._uploads_dir for why). + return session._ctx["roots"].roots[0].path # ── The door, not the button ───────────────────────────────────────────────── @@ -80,7 +82,7 @@ async def test_a_member_cannot_upload_to_a_read_only_root(tmp_path): refusal = [m for m in session.sent if m.get("type") == "error"] assert refusal and refusal[0].get("code") == "root_read_only" - assert not _uploads_dir(session).exists() + assert not (_uploads_dir(session) / "clip.mp4").exists() async def test_members_upload_normally_to_a_writable_root(tmp_path): diff --git a/packages/meshbay-node/tests/test_security_regressions.py b/packages/meshbay-node/tests/test_security_regressions.py index 9db8ac1..1a318f7 100644 --- a/packages/meshbay-node/tests/test_security_regressions.py +++ b/packages/meshbay-node/tests/test_security_regressions.py @@ -133,14 +133,22 @@ def test_the_node_never_generates_a_name_it_would_refuse(tmp_path): def _uploads_dir(session) -> Path: """ - Where this session's uploads land: uploads/ inside its first writable root. + Where an unaddressed upload lands: the first writable root itself. + + There is no `uploads/` subdirectory any more. It was the last of v5's + quarantine — the per-user layer went on 2026-08-14 — and it went for the + same reason: a folder appearing beside the operator's library because + somebody sent a file is the node deciding how their disk is arranged. The + protections that made the quarantine worth having are the allowlist, the + size cap, the chunk ordering and the no-overwrite rule, and every one of + them is asserted below, unchanged. Asked of the root set rather than assembled by hand, so a test cannot pass while agreeing with a wrong answer the code also produced. """ writable = session._ctx["roots"].writable_roots assert writable, "the fixture must give the group a writable root" - return writable[0].path / "uploads" + return writable[0].path def _session(tmp_path: Path, user_id: str) -> WebRTCPeerSession: @@ -176,7 +184,6 @@ def test_upload_cannot_overwrite_another_members_file(tmp_path): """ victim = _session(tmp_path, "victim-user") uploads = _uploads_dir(victim) - uploads.mkdir() original = uploads / "important.mp4" original.write_bytes(b"operator's original content") @@ -226,50 +233,57 @@ def test_dir_create_cannot_escape_the_shared_root(tmp_path, bad): assert set(tmp_path.rglob("*")) == before, f"created something via {bad!r}" -def test_upload_ignores_any_directory_the_client_asks_for(tmp_path): +def test_the_client_names_a_folder_and_never_a_filesystem_path(tmp_path): """ - The destination inside a root is the node's decision, and stays so. + The destination is now the folder the sender is looking at, which means the + client does choose it — and the whole of what keeps that safe is that the + choice is *resolved against the group's own roots* rather than joined to + one. - A client now names the *root* it is uploading into — it has to, once a group - can have several writable ones — but that is a name looked up in the root - table, never a path. Everything below the root is still chosen here, so the - traversal surface a client-chosen destination would open does not exist. + `RootSet.resolve()` refuses `..`, absolute segments and anything whose + resolved form escapes its root, symlinks included. So "which of this + group's folders" is answerable by a member and "which path on the + operator's disk" is not. """ session = _session(tmp_path, "user-1") + (session._ctx["roots"].roots[0].path / "sub").mkdir() + before = set(tmp_path.rglob("*")) - session._do_file_upload({ - "filename": "note.txt", "dir": "../../etc", "path": "/etc", - "chunk_index": 0, "total_chunks": 1, - "data": base64.b64encode(b"x").decode(), - }) + for bad in ("../../etc", "/etc", "shared/../..", "shared/../../etc", + "nope", "shared/missing"): + session.sent.clear() + session._do_file_upload({ + "filename": "note.txt", "dir": bad, + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal, f"{bad!r} was accepted" + assert refusal[0].get("code") in ("no_such_root", "no_such_directory"), bad - assert (_uploads_dir(session) / "note.txt").read_bytes() == b"x" - assert not (tmp_path / "etc").exists() + assert set(tmp_path.rglob("*")) == before, "a refused upload still wrote" -@pytest.mark.parametrize("named_root", [ - "../../etc", "/etc", "shared/../..", "Shared/uploads", "nope", -]) -def test_a_root_name_is_looked_up_never_joined(tmp_path, named_root): +def test_an_upload_lands_in_the_folder_it_names(tmp_path): """ - The name the client sends is matched against the group's root table and - refused when it matches nothing. A version that joined it to a path — or - that quietly fell back to the first writable root — would turn "which - directory" into either a traversal or a file on a disk the operator did - not intend, and the second is discovered weeks later. + And in that folder itself — the `uploads/` subdirectory the node used to + create is gone. Somebody dropping a file into the folder they are looking + at expects it to be in that folder. """ session = _session(tmp_path, "user-1") - before = set(tmp_path.rglob("*")) + root = session._ctx["roots"].roots[0] + (root.path / "Albums").mkdir() session._do_file_upload({ - "filename": "note.txt", "root": named_root, + "filename": "note.txt", "dir": f"{root.name}/Albums", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"x").decode(), }) - refusal = [m for m in session.sent if m.get("type") == "error"] - assert refusal and refusal[0].get("code") == "no_such_root", named_root - assert set(tmp_path.rglob("*")) == before, f"wrote something via {named_root!r}" + assert (root.path / "Albums" / "note.txt").read_bytes() == b"x" + assert not (root.path / "Albums" / "uploads").exists(), ( + "the node invented a subdirectory in the operator's library") + assert not (root.path / "uploads").exists() def test_an_upload_goes_to_the_root_it_names(tmp_path): @@ -291,13 +305,13 @@ def test_an_upload_goes_to_the_root_it_names(tmp_path): ]) session._do_file_upload({ - "filename": "note.txt", "root": "Incoming", + "filename": "note.txt", "dir": "Incoming", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"x").decode(), }) - assert (incoming / "uploads" / "note.txt").read_bytes() == b"x" - assert not (media / "uploads").exists(), "it went to the first root instead" + assert (incoming / "note.txt").read_bytes() == b"x" + assert not (media / "note.txt").exists(), "it went to the first root instead" def test_a_read_only_root_refuses_an_upload(tmp_path): @@ -314,14 +328,14 @@ def test_a_read_only_root_refuses_an_upload(tmp_path): session._is_node_admin = lambda: True session._do_file_upload({ - "filename": "note.txt", "root": "Published", + "filename": "note.txt", "dir": "Published", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"x").decode(), }) refusal = [m for m in session.sent if m.get("type") == "error"] assert refusal and refusal[0].get("code") == "root_read_only" - assert not (published / "uploads").exists() + assert not (published / "note.txt").exists() def test_a_fully_read_only_group_refuses_an_unaddressed_upload(tmp_path): @@ -343,7 +357,7 @@ def test_a_fully_read_only_group_refuses_an_unaddressed_upload(tmp_path): refusal = [m for m in session.sent if m.get("type") == "error"] assert refusal and refusal[0].get("code") == "no_writable_root" - assert not (published / "uploads").exists() + assert not (published / "note.txt").exists() def test_an_ejected_root_refuses_an_upload(tmp_path): @@ -362,14 +376,14 @@ def test_an_ejected_root_refuses_an_upload(tmp_path): session._ctx["roots"] = roots session._do_file_upload({ - "filename": "note.txt", "root": "USB", + "filename": "note.txt", "dir": "USB", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"x").decode(), }) refusal = [m for m in session.sent if m.get("type") == "error"] assert refusal and refusal[0].get("code") == "root_unavailable" - assert not (usb / "uploads").exists() + assert not (usb / "note.txt").exists() def test_two_members_can_send_the_same_filename(tmp_path): -- cgit v1.2.3 From 920284009d634cb568f95b3e93b93012c4b803bb Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 23:42:59 +0200 Subject: feat(client): bring back New folder, icon-only — and close the hole it opened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The control was hidden and its `canCreateDir` left computed and unused. It is back in the Files toolbar as an icon: the toolbar already carries one labelled primary action, and a second beside it competes for the width the breadcrumb trail needs. The name is in `title` *and* `aria-label` — a title is invisible to a screen reader on a button with no text, so an icon-only control without both is simply unnamed for anyone not reading with their eyes. Its gate changes. It required `isNodeAdmin`, which contradicted the node's own rule — "making a directory is not a privileged act; a member who can add a file can organise where it goes" — and hid the control from everyone who could have used it. It now follows the Upload button: a writable root, and not at the top of a group, where the level is the set of roots rather than a directory on anyone's disk. Restoring it surfaced a real gap. `_do_dir_create` never learned about RO/RW: `_do_file_upload` gained the `writable` check with the model and this one did not, so a member refused a file in a published library could still leave empty directories all through it, and could write to a drive mid-eject. Read-only has to mean read-only for every way of writing, not just for files. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../src/meshbay_hub/static/files-app.js | 19 +++++++++- .../meshbay-hub/src/meshbay_hub/static/style.css | 4 +++ .../tests/test_upload_controls_hidden.py | 30 ++++++++++++++++ .../src/meshbay_node/transport/webrtc_server.py | 22 ++++++++++++ .../tests/test_root_writable_policy.py | 41 ++++++++++++++++++++++ 5 files changed, 115 insertions(+), 1 deletion(-) (limited to 'packages/meshbay-hub/tests/test_upload_controls_hidden.py') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js index 9ca3aae..fb57a0f 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js @@ -234,7 +234,13 @@ function FilesPanel({ // set of roots, which is the operator's configuration and not a directory on // anyone's disk. The node refuses it, so offering it would only produce an // error nobody can act on. - const canCreateDir = Boolean(currentPath) && isNodeAdmin; + // + // Otherwise the rule is the same as the Upload button's, and for the same + // reason the node gives: "making a directory is not a privileged act — a + // member who can add a file can organise where it goes". It used to require + // `isNodeAdmin`, which contradicted the node and hid the control from + // everyone who could actually use it. + const canCreateDir = Boolean(currentPath) && currentRootWritable && !readOnly; const breadcrumbs = currentPath ? currentPath.split('/') : []; @@ -363,6 +369,17 @@ function FilesPanel({ onChange=${uploadFile} /> `} + ${/* Icon only: the toolbar already carries a labelled primary + action, and a second one beside it competes with it for the + width a breadcrumb trail needs. The name lives in the tooltip + and in aria-label, so it is not lost to anyone reading with + something other than their eyes. */''} + ${canCreateDir && html` + + `}
diff --git a/packages/meshbay-hub/src/meshbay_hub/static/style.css b/packages/meshbay-hub/src/meshbay_hub/static/style.css index 0441e14..4647209 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/style.css +++ b/packages/meshbay-hub/src/meshbay_hub/static/style.css @@ -833,6 +833,10 @@ button:disabled { opacity: 0.5; cursor: not-allowed; } .tb-btn:disabled { opacity: 0.45; cursor: not-allowed; } .tb-btn:disabled:hover { border-color: var(--border); color: var(--text); } .tb-btn .icon { width: 15px; height: 15px; } +/* Square, for a toolbar button whose name is in its tooltip rather than + beside it. The horizontal padding goes; the height does not, so it lines up + with the labelled buttons next to it. */ +.tb-btn-icon { padding: 0; width: 32px; justify-content: center; gap: 0; } .tb-btn.primary { background: var(--accent); border-color: var(--accent); diff --git a/packages/meshbay-hub/tests/test_upload_controls_hidden.py b/packages/meshbay-hub/tests/test_upload_controls_hidden.py index 7b8b0e3..a61de1f 100644 --- a/packages/meshbay-hub/tests/test_upload_controls_hidden.py +++ b/packages/meshbay-hub/tests/test_upload_controls_hidden.py @@ -73,6 +73,36 @@ def test_the_files_upload_button_is_not_offered_at_the_top_of_a_group(): assert "currentPath &&" in toolbar +def test_the_new_folder_button_follows_the_same_rule_as_upload(): + """ + Both write to the operator's disk, so both need a writable root — the node + refuses either otherwise. It used to require `isNodeAdmin`, which + contradicted the node ("a member who can add a file can organise where it + goes") and hid the control from everyone who could have used it. + """ + page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") + decl = page[page.index("const canCreateDir"):] + decl = decl[:decl.index(";") + 1] + assert "currentRootWritable" in decl + assert "currentPath" in decl, ( + "the top of a group is the set of roots, not a directory to create in") + assert "isNodeAdmin" not in decl + + +def test_an_icon_only_button_still_says_what_it_is(): + """ + The name moved into a tooltip to save toolbar width. A `title` is invisible + to a screen reader on a button with no text, so the label has to be there + as well — otherwise the control is simply unnamed for anyone not reading + with their eyes. + """ + page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") + block = page[page.index("canCreateDir && html`"):] + block = block[:block.index("")] + assert "title=" in block and "aria-label=" in block + assert "group.mkdir" in block + + def test_the_chat_composer_hides_its_paperclip(): chat = _component(CHAT_APP.read_text(encoding="utf-8"), "ChatPanel") composer = chat[chat.index("chat-input-row"):] diff --git a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py index 61458f2..0e2d418 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -1595,6 +1595,28 @@ class WebRTCPeerSession: "detail": "Choose a folder to create this in"}) return + # Read-only means read-only, and creating a folder writes to the + # operator's disk. `_do_file_upload` gained this check with the RO/RW + # model and this one did not — so a member could not add a file to a + # published library but could still leave empty directories in it. + owner = roots.split(parent_rel) + if owner is None: + self._send({"type": "error", "detail": "Invalid directory"}) + return + parent_root, _tail = owner + if not parent_root.writable: + self._send({"type": "error", + "detail": f"Directory '{parent_root.name}' is read-only", + "code": "root_read_only"}) + self._audit("dir_create_refused", parent_rel[:64]) + return + if not parent_root.available: + self._send({"type": "error", + "detail": f"Directory '{parent_root.name}' is " + f"currently unavailable", + "code": "root_unavailable"}) + return + parent = safe_subdir(roots, parent_rel) if parent is None or not parent.is_dir(): self._send({"type": "error", "detail": "Invalid directory"}) diff --git a/packages/meshbay-node/tests/test_root_writable_policy.py b/packages/meshbay-node/tests/test_root_writable_policy.py index d7f2666..7eb75fd 100644 --- a/packages/meshbay-node/tests/test_root_writable_policy.py +++ b/packages/meshbay-node/tests/test_root_writable_policy.py @@ -108,6 +108,47 @@ async def test_read_only_binds_the_operator_too(tmp_path): assert refusal and refusal[0].get("code") == "root_read_only" +async def test_a_member_cannot_create_a_folder_in_a_read_only_root(tmp_path): + """ + Read-only has to mean read-only for every way of writing, not just for + files. `_do_file_upload` gained this check with the RO/RW model and + `_do_dir_create` did not, so a member refused a file in a published library + could still leave empty directories all through it. + + Creating a folder stays unprivileged — the node's own words: "a member who + can add a file can organise where it goes". What changed is that it now + requires the same root to be writable that adding the file would have. + """ + session = _session(tmp_path, "member-1", writable=False) + session._do_dir_create({"dir": "shared", "name": "New folder"}) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" + assert not (tmp_path / "shared" / "New folder").exists() + + +async def test_a_member_can_create_a_folder_in_a_writable_root(tmp_path): + """The counter-property: it must stay unprivileged where it is allowed.""" + session = _session(tmp_path, "member-1", writable=True) + session._do_dir_create({"dir": "shared", "name": "New folder"}) + + assert not [m for m in session.sent if m.get("type") == "error"] + assert (tmp_path / "shared" / "New folder").is_dir() + + +async def test_an_ejected_root_refuses_a_new_folder(tmp_path): + """Writing to a drive somebody has their hand on, one level up from a file.""" + session = _session(tmp_path, "member-1", writable=True) + roots = session._ctx["roots"] + roots.roots[0].ejected = True + roots.roots[0].available = False + + session._do_dir_create({"dir": "shared", "name": "New folder"}) + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_unavailable" + assert not (tmp_path / "shared" / "New folder").exists() + + # ── Signed, or it is a suggestion ──────────────────────────────────────────── def _capture_challenges(session) -> list[tuple[str, str]]: -- cgit v1.2.3 From 80ca7dd0765a6c08fa5ea54c2e14972bba6fa564 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Mon, 7 Sep 2026 00:18:54 +0200 Subject: fix(client): New folder did nothing — prompt() throws in the desktop client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same `static/` tree is the web page and the application, and Electron does not implement `window.prompt`: Chromium leaves it to the embedder and Electron declines. It does not return null — it **throws**. The call sat above its own try, so clicking produced no folder, no error and nothing on screen to react to. A dead button, which is exactly how it was reported. Measured against this repo's own Electron 44 rather than assumed, because the first two diagnoses this session were reasoned and wrong: prompt('name?') -> Error: prompt() is not supported. confirm('sure?') -> opens a real modal alert('hi') -> opens a real modal So `confirm` and `alert` stay — a dozen call sites depend on them — and only `prompt` is banned. `test_no_prompt_in_the_spa.py` holds the whole tree to it, with the near-misses it must not flag (`mkdir_prompt`, `promptForName`). The name now comes from a field in the toolbar, which works in both clients and can show the node's refusal beside the input instead of after a dialog has closed. Navigating away drops a half-typed name: it would otherwise create the folder somewhere the person is no longer looking. The rest of the chain was verified end to end and was sound: `dir_create` {dir,name} → the node's handler → `dir_create_ack`, and `list_dirs` walks the filesystem rather than the index, so a folder with nothing in it appears on the very fetch that follows. The field itself was then driven inside a real Electron window — typing, Enter, the click, and the icon rendering. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../src/meshbay_hub/static/files-app.js | 65 ++++++++++++++-- .../src/meshbay_hub/static/locales/de.js | 3 +- .../src/meshbay_hub/static/locales/en.js | 3 +- .../src/meshbay_hub/static/locales/es.js | 3 +- .../src/meshbay_hub/static/locales/fr.js | 3 +- .../src/meshbay_hub/static/locales/it.js | 3 +- .../src/meshbay_hub/static/locales/ja.js | 3 +- .../src/meshbay_hub/static/locales/nl.js | 3 +- .../src/meshbay_hub/static/locales/pl.js | 3 +- .../src/meshbay_hub/static/locales/pt-BR.js | 3 +- .../src/meshbay_hub/static/locales/zh-CN.js | 3 +- .../meshbay-hub/src/meshbay_hub/static/style.css | 12 +++ .../meshbay-hub/tests/test_no_prompt_in_the_spa.py | 90 ++++++++++++++++++++++ .../tests/test_upload_controls_hidden.py | 17 ++-- 14 files changed, 192 insertions(+), 22 deletions(-) create mode 100644 packages/meshbay-hub/tests/test_no_prompt_in_the_spa.py (limited to 'packages/meshbay-hub/tests/test_upload_controls_hidden.py') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js index fb57a0f..4947f8c 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js @@ -104,19 +104,47 @@ function FilesPanel({ } }, [applyIndex, currentPath]); - const makeDirectory = useCallback(async () => { + // `null` while not creating one; a draft string while the field is open. + const [newDirName, setNewDirName] = useState(null); + const [creatingDir, setCreatingDir] = useState(false); + + /** + * Create a folder in the directory being browsed. + * + * The name comes from a field in the toolbar rather than `window.prompt`, + * which **throws** in Electron — "prompt() is not supported" — and threw + * outside this function's try, so clicking the button did nothing at all: + * no folder, no error, nothing in the interface to react to. `confirm()` and + * `alert()` do work there and are used elsewhere; `prompt` is the one + * Chromium leaves to the embedder and Electron declines to implement. + * + * An inline field is better anyway — it can show the refusal next to the + * input instead of after the dialog has closed. + */ + useEffect(() => { setNewDirName(null); }, [currentPath]); + + const makeDirectory = useCallback(async (rawName) => { const transport = transportRef.current; - if (!transport || !transport.connected) return; - const name = prompt(t('group.mkdir_prompt')); - if (!name || !name.trim()) return; + const name = (rawName || '').trim(); + if (!name) return; + if (!transport || !transport.connected) { + setError(t('group.mkdir_offline')); + return; + } + setCreatingDir(true); try { - await transport.createDirectory(currentPath, name.trim()); + await transport.createDirectory(currentPath, name); + // `list_dirs` walks the filesystem rather than the index, so a folder + // with nothing in it is here on this very fetch. const indexMsg = await transport.fetchIndex(); if (indexMsg.entries) setEntries(indexMsg.entries); if (indexMsg.dirs) setNodeDirs(indexMsg.dirs); if (indexMsg.roots) setNodeRoots(indexMsg.roots); + setNewDirName(null); } catch (err) { setError(err.message); + } finally { + setCreatingDir(false); } }, [currentPath]); @@ -374,12 +402,35 @@ function FilesPanel({ width a breadcrumb trail needs. The name lives in the tooltip and in aria-label, so it is not lost to anyone reading with something other than their eyes. */''} - ${canCreateDir && html` - `} + ${canCreateDir && newDirName !== null && html` + + setNewDirName(e.target.value)} + onKeyDown=${(e) => { + if (e.key === 'Enter') makeDirectory(newDirName); + if (e.key === 'Escape') setNewDirName(null); + }} /> + + + + `}
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 2e7d3c5..c6d0331 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/de.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/de.js @@ -154,7 +154,8 @@ export default { + 'diese Gruppe hostet.', 'group.upload': 'Hochladen', 'group.mkdir': 'Neuer Ordner', - 'group.mkdir_prompt': 'Name des neuen Ordners:', + 'group.mkdir_prompt': 'Name des neuen Ordners', + 'group.mkdir_offline': 'Nicht mit dem Node verbunden.', 'device.add_title': 'This browser is not linked to this node yet', 'device.add_hint': 'Your account is known here, but this browser holds a different key. Approve it from a device already linked — no operator needed.', 'device.add_btn': 'Get a linking code', 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 350a597..0831aa3 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/en.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/en.js @@ -154,7 +154,8 @@ export default { 'group.offline_hint': 'Files will appear when a node hosting this group connects.', 'group.upload': 'Upload', 'group.mkdir': 'New folder', - 'group.mkdir_prompt': 'Name of the new folder:', + 'group.mkdir_prompt': 'New folder name', + 'group.mkdir_offline': 'Not connected to the node.', 'device.add_title': 'This browser is not linked to this node yet', 'device.add_hint': 'Your account is known here, but this browser holds a different key. Approve it from a device already linked — no operator needed.', 'device.add_btn': 'Get a linking code', 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 22a9461..18570cc 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/es.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/es.js @@ -152,7 +152,8 @@ export default { + 'este grupo.', 'group.upload': 'Subir', 'group.mkdir': 'Nueva carpeta', - 'group.mkdir_prompt': 'Nombre de la nueva carpeta:', + 'group.mkdir_prompt': 'Nombre de la nueva carpeta', + 'group.mkdir_offline': 'Sin conexión con el nodo.', 'device.add_title': 'This browser is not linked to this node yet', 'device.add_hint': 'Your account is known here, but this browser holds a different key. Approve it from a device already linked — no operator needed.', 'device.add_btn': 'Get a linking code', 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 3da68b2..65021e3 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/fr.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/fr.js @@ -153,7 +153,8 @@ export default { + 'groupe se connectera.', 'group.upload': 'Envoyer', 'group.mkdir': 'Nouveau dossier', - 'group.mkdir_prompt': 'Nom du nouveau dossier :', + 'group.mkdir_prompt': 'Nom du nouveau dossier', + 'group.mkdir_offline': 'Non connecté au nœud.', 'device.add_title': 'Ce navigateur n’est pas encore lié à ce nœud', 'device.add_hint': 'Votre compte est connu ici, mais ce navigateur détient une autre clé. Approuvez-le depuis un appareil déjà lié — sans passer par l’opérateur.', 'device.add_btn': 'Obtenir un code de liaison', 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 335ed68..7f58764 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/it.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/it.js @@ -153,7 +153,8 @@ export default { + 'questo gruppo.', 'group.upload': 'Carica', 'group.mkdir': 'Nuova cartella', - 'group.mkdir_prompt': 'Nome della nuova cartella:', + 'group.mkdir_prompt': 'Nome della nuova cartella', + 'group.mkdir_offline': 'Non connesso al nodo.', 'device.add_title': 'This browser is not linked to this node yet', 'device.add_hint': 'Your account is known here, but this browser holds a different key. Approve it from a device already linked — no operator needed.', 'device.add_btn': 'Get a linking code', 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 4dd2f30..0e2dd42 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/ja.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/ja.js @@ -151,7 +151,8 @@ export default { + 'ファイルが表示されます。', 'group.upload': 'アップロード', 'group.mkdir': '新しいフォルダー', - 'group.mkdir_prompt': '新しいフォルダーの名前：', + 'group.mkdir_prompt': '新しいフォルダー名', + 'group.mkdir_offline': 'ノードに接続していません。', 'device.add_title': 'This browser is not linked to this node yet', 'device.add_hint': 'Your account is known here, but this browser holds a different key. Approve it from a device already linked — no operator needed.', 'device.add_btn': 'Get a linking code', 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 cb36d38..9f74e19 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/nl.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/nl.js @@ -154,7 +154,8 @@ export default { + 'die deze groep host.', 'group.upload': 'Uploaden', 'group.mkdir': 'Nieuwe map', - 'group.mkdir_prompt': 'Naam van de nieuwe map:', + 'group.mkdir_prompt': 'Naam van de nieuwe map', + 'group.mkdir_offline': 'Niet verbonden met de node.', 'device.add_title': 'This browser is not linked to this node yet', 'device.add_hint': 'Your account is known here, but this browser holds a different key. Approve it from a device already linked — no operator needed.', 'device.add_btn': 'Get a linking code', 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 91d0d90..083d72d 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/pl.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/pl.js @@ -157,7 +157,8 @@ export default { 'group.offline_hint': 'Pliki pojawią się, gdy połączy się node hostujący tę grupę.', 'group.upload': 'Wyślij', 'group.mkdir': 'Nowy folder', - 'group.mkdir_prompt': 'Nazwa nowego folderu:', + 'group.mkdir_prompt': 'Nazwa nowego folderu', + 'group.mkdir_offline': 'Brak połączenia z węzłem.', 'device.add_title': 'This browser is not linked to this node yet', 'device.add_hint': 'Your account is known here, but this browser holds a different key. Approve it from a device already linked — no operator needed.', 'device.add_btn': 'Get a linking code', 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 2b26a0b..bdc6c3b 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 @@ -154,7 +154,8 @@ export default { + 'se conectar.', 'group.upload': 'Enviar', 'group.mkdir': 'Nova pasta', - 'group.mkdir_prompt': 'Nome da nova pasta:', + 'group.mkdir_prompt': 'Nome da nova pasta', + 'group.mkdir_offline': 'Sem conexão com o nó.', 'device.add_title': 'This browser is not linked to this node yet', 'device.add_hint': 'Your account is known here, but this browser holds a different key. Approve it from a device already linked — no operator needed.', 'device.add_btn': 'Get a linking code', 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 f1d7f9c..0a1fe1a 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 @@ -150,7 +150,8 @@ export default { 'group.offline_hint': '当托管此群组的 node 连接后，文件就会出现。', 'group.upload': '上传', 'group.mkdir': '新建文件夹', - 'group.mkdir_prompt': '新文件夹的名称：', + 'group.mkdir_prompt': '新文件夹名称', + 'group.mkdir_offline': '未连接到节点。', 'device.add_title': 'This browser is not linked to this node yet', 'device.add_hint': 'Your account is known here, but this browser holds a different key. Approve it from a device already linked — no operator needed.', 'device.add_btn': 'Get a linking code', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/style.css b/packages/meshbay-hub/src/meshbay_hub/static/style.css index 4647209..c5d5ce3 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/style.css +++ b/packages/meshbay-hub/src/meshbay_hub/static/style.css @@ -837,6 +837,18 @@ button:disabled { opacity: 0.5; cursor: not-allowed; } beside it. The horizontal padding goes; the height does not, so it lines up with the labelled buttons next to it. */ .tb-btn-icon { padding: 0; width: 32px; justify-content: center; gap: 0; } + +/* Naming a new folder, in the toolbar. Not `window.prompt`, which throws in + Electron — and an inline field can put the node's refusal next to the input + rather than after a dialog has closed. */ +.tb-mkdir { display: inline-flex; align-items: center; gap: 6px; } +.tb-mkdir-input { + height: 32px; width: 170px; padding: 0 9px; + border: 1px solid var(--border-focus); border-radius: 6px; + background: var(--bg-surface); color: var(--text); + font-family: inherit; font-size: 0.83em; +} +.tb-mkdir-input:focus { outline: none; } .tb-btn.primary { background: var(--accent); border-color: var(--accent); diff --git a/packages/meshbay-hub/tests/test_no_prompt_in_the_spa.py b/packages/meshbay-hub/tests/test_no_prompt_in_the_spa.py new file mode 100644 index 0000000..d126a05 --- /dev/null +++ b/packages/meshbay-hub/tests/test_no_prompt_in_the_spa.py @@ -0,0 +1,90 @@ +""" +`window.prompt` does not exist in the desktop client. + +The same `static/` tree is the web page and the application (CLAUDE.md's "one +UI source"), and Electron does not implement `prompt` — Chromium leaves it to +the embedder and Electron declines. It does not return null: it **throws**, +`Error: prompt() is not supported.` + +That made the Files toolbar's New folder button do nothing whatsoever. The call +sat above its own try, so the click produced no folder, no error, and nothing +on screen to react to — the failure looks exactly like a dead button, which is +what it was reported as. + +Measured rather than assumed, against this repo's own Electron 44: + + prompt('name?') -> Error: prompt() is not supported. + confirm('sure?') -> opens a real modal + alert('hi') -> opens a real modal + +So `confirm` and `alert` stay allowed and are used in a dozen places; only +`prompt` is banned. Anything that needs typed input needs a field. +""" + +import re +from pathlib import Path + +import pytest + +STATIC = Path(__file__).resolve().parents[1] / "src" / "meshbay_hub" / "static" + +pytestmark = pytest.mark.skipif(not STATIC.exists(), + reason="SPA sources unavailable") + +# `prompt(` as a call, not `window.prompt` inside a comment or a longer +# identifier like `mkdir_prompt` / `promptForName`. +CALL = re.compile(r"(? str: + source = re.sub(r"/\*.*?\*/", "", source, flags=re.S) + return re.sub(r"^\s*//.*$", "", source, flags=re.M) + + +def test_nothing_calls_prompt(): + offenders = [] + for path in sorted(STATIC.glob("*.js")): + if path.name == "sw.js": + continue + for i, line in enumerate(_code_only( + path.read_text(encoding="utf-8")).splitlines(), 1): + if CALL.search(line): + offenders.append(f"{path.name}:{i}: {line.strip()}") + + assert not offenders, ( + "prompt() throws in the desktop client, and the click that reaches it " + "does nothing at all:\n " + "\n ".join(offenders)) + + +def test_the_pattern_would_catch_a_real_call(): + """ + A guard that matches nothing passes over an empty set, which looks exactly + like success. Both spellings, and the near-misses it must not flag. + """ + assert CALL.search("const n = prompt('x');") + assert CALL.search("const n = window.prompt('x');") + assert not CALL.search("t('group.mkdir_prompt')") + assert not CALL.search("promptForName();") + assert not CALL.search("this.prompt(1);") + + +def test_creating_a_folder_uses_a_field(): + """ + The control the ban is about. A typed name needs somewhere to type it, and + an inline field can show the node's refusal beside the input rather than + after a dialog has closed. + """ + source = (STATIC / "files-app.js").read_text(encoding="utf-8") + assert "tb-mkdir-input" in source + assert "newDirName" in source + assert "group.mkdir_prompt" in source, "the field has no label or placeholder" + + +def test_leaving_the_folder_drops_a_half_typed_name(): + """ + Otherwise the folder is created where the person is no longer looking — + they navigated away, the draft came along, and the name lands in a + directory they were not thinking about. + """ + source = (STATIC / "files-app.js").read_text(encoding="utf-8") + assert "useEffect(() => { setNewDirName(null); }, [currentPath]);" in source diff --git a/packages/meshbay-hub/tests/test_upload_controls_hidden.py b/packages/meshbay-hub/tests/test_upload_controls_hidden.py index a61de1f..bdba373 100644 --- a/packages/meshbay-hub/tests/test_upload_controls_hidden.py +++ b/packages/meshbay-hub/tests/test_upload_controls_hidden.py @@ -94,13 +94,20 @@ def test_an_icon_only_button_still_says_what_it_is(): The name moved into a tooltip to save toolbar width. A `title` is invisible to a screen reader on a button with no text, so the label has to be there as well — otherwise the control is simply unnamed for anyone not reading - with their eyes. + with their eyes. The same goes for the field it opens, which has a + placeholder and no visible label. """ page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") - block = page[page.index("canCreateDir && html`"):] - block = block[:block.index("")] - assert "title=" in block and "aria-label=" in block - assert "group.mkdir" in block + opener = page[page.index("canCreateDir && newDirName === null"):] + opener = opener[:opener.index("")] + assert "title=" in opener and "aria-label=" in opener + assert "group.mkdir" in opener + + field = page[page.index("canCreateDir && newDirName !== null"):] + field = field[:field.index("")] + assert "aria-label=" in field, ( + "the name field is labelled by a placeholder alone, which a screen " + "reader does not announce as a name") def test_the_chat_composer_hides_its_paperclip(): -- cgit v1.2.3 From 375419c041a4523614b3a1b98fa54fb5acb1b8bf Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Mon, 7 Sep 2026 01:50:14 +0200 Subject: fix(client): Save in the app panes was invisible, and Chat's never landed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects behind one report — "you cannot always click Save, you do not notice, and it does not work". **It did not work, for Chat, systematically.** `_dispatch` resolves an admin ack against the pending request and returns, which is 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. Same shape as the root-ack bug, in a different message — so the set is now `BROADCAST_ACK_TYPES`, named for the property that makes it true, and covers `chat_directory`, `chat_link_preview` and `app_directories`. **You could not notice, because Save was not visibly a button.** It carried `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; disabled it was the same thing at 40% opacity. Measured on a real page: enabled is now accent on white, disabled is grey text on a plain border, and an inert one says why ("No changes to save") rather than leaving the reader to guess what the pane counts as a change. Videos' second Save — the TMDB one — is the same control, because two Save buttons in one pane that do not look alike is worse than either looking wrong. Verified by driving the real pane in Electron through the whole cycle: inert, pick a folder, live, save, and the node's answer coming back to disable it again. Four earlier readings said the enabled button was transparent; all four were taken inside python's directory-listing page, which the app never runs in — my scaffolding, not the code. Hub suite only: the node package is untouched by this. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../src/meshbay_hub/static/chat-app-settings.js | 7 ++- .../src/meshbay_hub/static/locales/de.js | 1 + .../src/meshbay_hub/static/locales/en.js | 1 + .../src/meshbay_hub/static/locales/es.js | 1 + .../src/meshbay_hub/static/locales/fr.js | 1 + .../src/meshbay_hub/static/locales/it.js | 1 + .../src/meshbay_hub/static/locales/ja.js | 1 + .../src/meshbay_hub/static/locales/nl.js | 1 + .../src/meshbay_hub/static/locales/pl.js | 1 + .../src/meshbay_hub/static/locales/pt-BR.js | 1 + .../src/meshbay_hub/static/locales/zh-CN.js | 1 + .../src/meshbay_hub/static/music-app-settings.js | 7 ++- .../src/meshbay_hub/static/photos-app-settings.js | 7 ++- .../meshbay-hub/src/meshbay_hub/static/style.css | 30 +++++++++ .../src/meshbay_hub/static/transport.js | 38 +++++++++--- .../src/meshbay_hub/static/video-app-settings.js | 13 +++- .../meshbay-hub/tests/test_app_settings_plugin.py | 72 ++++++++++++++++++++++ .../tests/test_upload_controls_hidden.py | 13 ++-- 18 files changed, 174 insertions(+), 23 deletions(-) (limited to 'packages/meshbay-hub/tests/test_upload_controls_hidden.py') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/chat-app-settings.js b/packages/meshbay-hub/src/meshbay_hub/static/chat-app-settings.js index 68569b9..5a15074 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/chat-app-settings.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/chat-app-settings.js @@ -45,11 +45,14 @@ function ChatSettings({ roots, dirs, settings, saveDirectories, transport, signF value=${directory} disabled=${busy || noWritable} onChange=${setDirectory} /> - + ${!busy && !dirty && html` + ${t('settings_app.no_changes')}`} +
<${ToggleSwitch} checked=${linkPreview} disabled=${busy} 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 d05d3a3..d47762e 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/de.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/de.js @@ -840,6 +840,7 @@ export default { 'folder_tree.read_only_blocked': 'Nur lesen — kein Schreiben möglich', 'settings_app.save': 'Speichern', 'settings_app.saving': 'Wird gespeichert…', + 'settings_app.no_changes': 'Keine Änderungen zu speichern', 'settings_app.disabled_hint': 'Schalten Sie diese App ein, um sie zu konfigurieren.', 'settings_app.video_directories_label': 'Video-Ordner', 'settings_app.video_directories_hint': 'Wo die Filme und Serien dieser Gruppe liegen. Nichts außerhalb erscheint im Videos-Tab.', 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 6dbb70f..932aad3 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/en.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/en.js @@ -628,6 +628,7 @@ export default { 'folder_tree.read_only_blocked': 'Read-only — cannot be written to', 'settings_app.save': 'Save', 'settings_app.saving': 'Saving…', + 'settings_app.no_changes': 'No changes to save', 'settings_app.disabled_hint': 'Turn this app on to configure it.', 'settings_app.video_directories_label': 'Video folders', 'settings_app.video_directories_hint': 'Where this group\'s films and shows live. Nothing outside them appears in the Videos tab.', 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 aa950cc..ca912a4 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/es.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/es.js @@ -836,6 +836,7 @@ export default { 'folder_tree.read_only_blocked': 'Solo lectura: no se puede escribir', 'settings_app.save': 'Guardar', 'settings_app.saving': 'Guardando…', + 'settings_app.no_changes': 'Sin cambios que guardar', 'settings_app.disabled_hint': 'Activa esta aplicación para configurarla.', 'settings_app.video_directories_label': 'Carpetas de vídeo', 'settings_app.video_directories_hint': 'Dónde están las películas y series de este grupo. Nada fuera de ellas aparece en la pestaña Vídeos.', 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 daa9f94..ccd9e25 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/fr.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/fr.js @@ -854,6 +854,7 @@ export default { 'folder_tree.read_only_blocked': 'Lecture seule — écriture impossible', 'settings_app.save': 'Enregistrer', 'settings_app.saving': 'Enregistrement…', + 'settings_app.no_changes': 'Aucune modification à enregistrer', 'settings_app.disabled_hint': 'Activez cette application pour la configurer.', 'settings_app.video_directories_label': 'Dossiers vidéo', 'settings_app.video_directories_hint': 'Où vivent les films et séries de ce groupe. Rien en dehors n\'apparaît dans l\'onglet Vidéos.', 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 364c26c..c1c3d52 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/it.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/it.js @@ -850,6 +850,7 @@ export default { 'folder_tree.read_only_blocked': 'Sola lettura: non vi si può scrivere', 'settings_app.save': 'Salva', 'settings_app.saving': 'Salvataggio…', + 'settings_app.no_changes': 'Nessuna modifica da salvare', 'settings_app.disabled_hint': 'Attiva questa applicazione per configurarla.', 'settings_app.video_directories_label': 'Cartelle video', 'settings_app.video_directories_hint': 'Dove si trovano film e serie di questo gruppo. Nulla al di fuori compare nella scheda Video.', 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 f778fc4..e5392a3 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/ja.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/ja.js @@ -834,6 +834,7 @@ export default { 'folder_tree.read_only_blocked': '読み取り専用 — 書き込みできません', 'settings_app.save': '保存', 'settings_app.saving': '保存中…', + 'settings_app.no_changes': '保存する変更はありません', 'settings_app.disabled_hint': 'このアプリを有効にすると設定できます。', 'settings_app.video_directories_label': '動画フォルダー', 'settings_app.video_directories_hint': 'このグループの映画や番組がある場所です。それ以外は動画タブに表示されません。', 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 6e6842e..09e3e8d 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/nl.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/nl.js @@ -852,6 +852,7 @@ export default { 'folder_tree.read_only_blocked': 'Alleen-lezen — kan niet worden beschreven', 'settings_app.save': 'Opslaan', 'settings_app.saving': 'Opslaan…', + 'settings_app.no_changes': 'Geen wijzigingen om op te slaan', 'settings_app.disabled_hint': 'Zet deze app aan om hem in te stellen.', 'settings_app.video_directories_label': 'Videomappen', 'settings_app.video_directories_hint': 'Waar de films en series van deze groep staan. Niets daarbuiten verschijnt op het tabblad Video\'s.', 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 c8cbd16..34847e1 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/locales/pl.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/locales/pl.js @@ -878,6 +878,7 @@ export default { 'folder_tree.read_only_blocked': 'Tylko do odczytu — nie można zapisywać', 'settings_app.save': 'Zapisz', 'settings_app.saving': 'Zapisywanie…', + 'settings_app.no_changes': 'Brak zmian do zapisania', 'settings_app.disabled_hint': 'Włącz tę aplikację, aby ją skonfigurować.', 'settings_app.video_directories_label': 'Foldery wideo', 'settings_app.video_directories_hint': 'Gdzie znajdują się filmy i seriale tej grupy. Nic poza nimi nie pojawi się w zakładce Wideo.', 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 6eb2242..fc211c9 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 @@ -837,6 +837,7 @@ export default { 'folder_tree.read_only_blocked': 'Somente leitura — não é possível gravar', 'settings_app.save': 'Salvar', 'settings_app.saving': 'Salvando…', + 'settings_app.no_changes': 'Nenhuma alteração para salvar', 'settings_app.disabled_hint': 'Ative este aplicativo para configurá-lo.', 'settings_app.video_directories_label': 'Pastas de vídeo', 'settings_app.video_directories_hint': 'Onde ficam os filmes e séries deste grupo. Nada fora delas aparece na aba Vídeos.', 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 044611e..9db7cc7 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 @@ -821,6 +821,7 @@ export default { 'folder_tree.read_only_blocked': '只读 — 无法写入', 'settings_app.save': '保存', 'settings_app.saving': '正在保存…', + 'settings_app.no_changes': '没有需要保存的更改', 'settings_app.disabled_hint': '启用该应用后即可配置。', 'settings_app.video_directories_label': '视频文件夹', 'settings_app.video_directories_hint': '该群组的影片和剧集所在位置。其外的内容不会出现在「视频」标签页。', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/music-app-settings.js b/packages/meshbay-hub/src/meshbay_hub/static/music-app-settings.js index 01174b1..a3b02d9 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/music-app-settings.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/music-app-settings.js @@ -38,11 +38,14 @@ function MusicSettings({ roots, dirs, settings, saveDirectories, transport, sign value=${directories} disabled=${busy} onChange=${setDirectories} /> - + ${!busy && !dirsDirty && html` + ${t('settings_app.no_changes')}`} +

${t('settings_node.musicbrainz_title')}

${t('settings_node.musicbrainz_hint')}

diff --git a/packages/meshbay-hub/src/meshbay_hub/static/photos-app-settings.js b/packages/meshbay-hub/src/meshbay_hub/static/photos-app-settings.js index 81e6cc1..a241eea 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/photos-app-settings.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/photos-app-settings.js @@ -32,11 +32,14 @@ function PhotoSettings({ roots, dirs, settings, saveDirectories }) { value=${directories} disabled=${busy} onChange=${setDirectories} /> - + ${!busy && !dirty && html` + ${t('settings_app.no_changes')}`} + ${msg && html`

${msg}

`} `; diff --git a/packages/meshbay-hub/src/meshbay_hub/static/style.css b/packages/meshbay-hub/src/meshbay_hub/static/style.css index 9c0d869..bf4e8e1 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/style.css +++ b/packages/meshbay-hub/src/meshbay_hub/static/style.css @@ -1271,6 +1271,36 @@ button:disabled { opacity: 0.5; cursor: not-allowed; } /* One app's settings pane, inside its section. */ .app-settings > .settings-row:first-child { margin-top: 0; } +/* The Save button of an app's settings pane. + * + * It was `btn btn-small btn-secondary`, and there is no `.btn` rule at all — + * so it inherited `.btn-secondary`: no background, a transparent border, dim + * grey text. Enabled, it already looked like a disabled control; disabled, it + * was the same thing at 40% opacity. "You cannot always click Save, you do not + * notice, and it does not work" was one sentence describing all of that. + * + * It looks like a button now, and a disabled one looks disabled. */ +.app-save { + display: inline-flex; align-items: center; gap: 6px; + height: 30px; padding: 0 14px; margin-top: 4px; + border: 1px solid var(--accent); border-radius: 6px; + background: var(--accent); color: var(--accent-text); + font-family: inherit; font-size: 0.83em; cursor: pointer; +} +.app-save:hover:not(:disabled) { background: var(--accent-hover); } +/* No transition on the enabled/disabled swap. It animates a colour change that + coincides with a text-colour change, which reads as a flicker — and it made + the state genuinely hard to observe: a measurement taken during it reports a + transparent background on a button that is not transparent. */ +.app-save:disabled { + background: none; color: var(--text-dim); + border-color: var(--border); opacity: 1; cursor: default; +} +/* Why it is inert, beside it — a disabled control that explains itself is the + difference between "nothing changed" and "this is broken". */ +.app-save-row { display: flex; align-items: center; gap: 10px; margin-top: 8px; } +.app-save-why { font-size: 0.82em; color: var(--text-dim); } + .app-settings-sub { margin: 18px 0 4px; font-size: 0.9em; font-weight: 600; padding-top: 12px; border-top: 1px solid var(--border); diff --git a/packages/meshbay-hub/src/meshbay_hub/static/transport.js b/packages/meshbay-hub/src/meshbay_hub/static/transport.js index 3acfd20..179292e 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/transport.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/transport.js @@ -72,13 +72,35 @@ function _aborted() { // change anything — `op` is already on every admin_challenge, and this // list is what lets a response two steps later be tied back to the right // one. -// The acks whose payload is state no caller could have predicted: they carry -// the node's whole roots table back. See the note where they are dispatched. -const ROOT_ACK_TYPES = new Set([ +// Acks the node *broadcasts* to everyone in the group, which the requester +// therefore also has to be handed. +// +// `_dispatch` resolves an admin ack against the pending request and returns, +// which is right for an op whose caller already knows the value it chose. It is +// wrong for these: every *other* connected client learns the change from the +// broadcast, and the one that asked for it is the only one that does not, +// because its own request swallowed its copy. Found twice — first on the root +// table, then on Chat's directory, where it meant the pane went on showing an +// unsaved-looking draft after a save that had worked. +const BROADCAST_ACK_TYPES = new Set([ 'root_update_ack', 'root_eject_ack', 'root_plug_ack', 'root_add_ack', 'root_remove_ack', + 'app_directories_ack', 'chat_directory_ack', 'chat_link_preview_ack', ]); +/** Hand a broadcast ack to the callback that would have had it from a peer. */ +function _replayBroadcast(transport, msg) { + if (msg.type === 'app_directories_ack' && transport._onAppDirectories) { + transport._onAppDirectories(msg.app, msg.directories || []); + } else if (msg.type === 'chat_directory_ack' && transport._onChatDirectory) { + transport._onChatDirectory(msg.path || ''); + } else if (msg.type === 'chat_link_preview_ack' && transport._onChatLinkPreview) { + transport._onChatLinkPreview(Boolean(msg.enabled)); + } else if (transport._onRootsChanged) { + transport._onRootsChanged(msg); + } +} + const ADMIN_OP_TYPES = new Set([ 'tmdb_override', 'tmdb_rematch', 'tmdb_config', 'tmdb_enabled', 'video_root', 'audio_root', 'photo_roots', @@ -2349,9 +2371,7 @@ class MeshBayTransport { // name it settled on. Returning here left the operator who clicked // Eject as the one client that never saw it happen, while every // other peer got the broadcast. So this one type is handed on. - if (ROOT_ACK_TYPES.has(msg.type) && this._onRootsChanged) { - this._onRootsChanged(msg); - } + if (BROADCAST_ACK_TYPES.has(msg.type)) _replayBroadcast(this, msg); return; } } @@ -2486,8 +2506,10 @@ class MeshBayTransport { // A root's flags changed, or one was ejected, plugged, added or removed. // Broadcast by the node to every peer, so everyone's table updates without // waiting for the next index_sync. - if (ROOT_ACK_TYPES.has(msg.type) && this._onRootsChanged) { - this._onRootsChanged(msg); + if (msg.type === 'root_update_ack' || msg.type === 'root_eject_ack' + || msg.type === 'root_plug_ack' || msg.type === 'root_add_ack' + || msg.type === 'root_remove_ack') { + if (this._onRootsChanged) this._onRootsChanged(msg); } // The operator's node is scanning — never the entries themselves, just diff --git a/packages/meshbay-hub/src/meshbay_hub/static/video-app-settings.js b/packages/meshbay-hub/src/meshbay_hub/static/video-app-settings.js index b2d5015..86e47ec 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/video-app-settings.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/video-app-settings.js @@ -56,11 +56,14 @@ function VideoSettings({ roots, dirs, settings, saveDirectories, transport, sign value=${directories} disabled=${busy} onChange=${setDirectories} /> - + ${!busy && !dirsDirty && html` + ${t('settings_app.no_changes')}`} +

${t('settings_node.tmdb_title')}

${t('settings_node.tmdb_hint')}

@@ -108,7 +111,11 @@ function VideoSettings({ roots, dirs, settings, saveDirectories, transport, sign

${t('settings_node.tmdb_language_hint')}

-