From a194b333169efb5e25bc05c94567600eec8bb823 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Wed, 2 Sep 2026 12:44:04 +0200 Subject: fix(hub): a dismissed notification stays dismissed across a restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking a notification navigated to the group and the entry disappeared — the intended behaviour — and it was back on the next launch. Neither half was wrong on its own, which is why it survived. `markRead` drops the entry locally *and* marks it read on the hub, deliberately: "Reading it is the point of clicking it: it goes, here and in the count, rather than sitting there greyed out." The hub honoured that and persisted it. But the startup fetch asked for `/v1/notifications?limit=20` with no filter, and the endpoint returns read and unread alike, so every dismissed notification came straight back. It asks for `unread_only=true` now — a parameter the endpoint already had and already tested. The feed still carried the fossil of the older intent: `class="notif-item ${n.read ? '' : 'notif-unread'}"`, styling for a read entry rendered greyed out, from before clicking meant dismissing. Nothing read reaches the feed any more, so that branch was dead code describing behaviour the application had abandoned — and noticing it is what made the two halves' disagreement visible. Removed. `unread_count` is computed server-side over the whole table and is unaffected by the filter, so the bell is unchanged. test_notification_dismissal.py holds both halves: the API round trip that is the reported bug (list, read, list again), its mirror showing the unfiltered endpoint still returns it — so the fix cannot read as a coincidence — and a static check that the SPA asks for the filter, which is the only one of the three that catches the defect that actually happened. Verified by dropping the parameter again: that one fails, the API tests do not. Read notifications now accumulate unread in the table rather than being deleted. Purge removes them; the volume is small. Making dismissal a delete would suit `purge_notifications`' own docstring — "these are signals, not a record" — but it would leave `/read` a misnomer and `read-all` inconsistent, so it is a separate decision. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014UtzVrzM7e2tG9fSpkR9ML --- packages/meshbay-hub/src/meshbay_hub/static/app.js | 10 +- .../tests/test_notification_dismissal.py | 144 +++++++++++++++++++++ 2 files changed, 152 insertions(+), 2 deletions(-) create mode 100644 packages/meshbay-hub/tests/test_notification_dismissal.py (limited to 'packages') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/app.js b/packages/meshbay-hub/src/meshbay_hub/static/app.js index 06b6c10..7cc7c83 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/app.js @@ -342,7 +342,7 @@ function NotificationFeed({ notifications, onMarkRead, onPurge }) { ${notifications.map(n => html` -
{ // Reading it is the point of clicking it: it goes, here and in the // count, rather than sitting there greyed out. @@ -579,7 +579,13 @@ function App() { if (!user || notifDisabled) { setNotifications([]); setUnreadCount(0); return; } - hubFetch('/v1/notifications?limit=20', { token: user.token }) + // `unread_only`: clicking one is what dismisses it (see markRead), so a + // read notification is a dismissed notification and must not come back on + // the next launch. Without this the two halves disagreed — the click + // removed it here and marked it read on the hub, and the next startup + // asked for everything and put it straight back. `unread_count` is + // computed server-side and is unaffected by the filter. + hubFetch('/v1/notifications?limit=20&unread_only=true', { token: user.token }) .then(data => { setNotifications(data.notifications || []); setUnreadCount(data.unread_count || 0); diff --git a/packages/meshbay-hub/tests/test_notification_dismissal.py b/packages/meshbay-hub/tests/test_notification_dismissal.py new file mode 100644 index 0000000..7324656 --- /dev/null +++ b/packages/meshbay-hub/tests/test_notification_dismissal.py @@ -0,0 +1,144 @@ +""" +A dismissed notification stays dismissed across a restart. + +Reported live: clicking a notification navigated to the group and the entry +disappeared — the intended behaviour — and it was back on the next launch of +the application. + +Neither half was wrong on its own, which is why it survived. `markRead` in +app.js drops the entry locally *and* marks it read on the hub, deliberately: +"Reading it is the point of clicking it: it goes, here and in the count, +rather than sitting there greyed out." The hub honoured that and persisted it. +But the startup fetch asked for `/v1/notifications?limit=20` with no filter, +and the endpoint returns read and unread alike, so every dismissed +notification came straight back. The feed still carried the fossil of the +older intent — `class="notif-item ${n.read ? '' : 'notif-unread'}"`, styling +for a read entry rendered greyed out, from before clicking meant dismissing. + +So the contract is between the two halves, and that is what this pins: the +hub's filter behaves, and the SPA asks for it. +""" + +import re +from pathlib import Path + +import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey +from cryptography.hazmat.primitives.asymmetric.x25519 import X25519PrivateKey + +from meshbay_common.crypto import pk_to_b64 +from meshbay_hub.api.deps import set_admin_usernames + +STATIC = Path(__file__).resolve().parents[1] / "src" / "meshbay_hub" / "static" +APP = STATIC / "app.js" + + +def _gen_user_keys(): + return (pk_to_b64(Ed25519PrivateKey.generate().public_key()), + pk_to_b64(X25519PrivateKey.generate().public_key())) + + +async def _register(client, username, email): + pk_ed, pk_x = _gen_user_keys() + r = await client.post("/v1/users/register", json={ + "username": username, "email": email, "password": "testpass99", + "pk_user_ed25519": pk_ed, "pk_user_x25519": pk_x, + }) + assert r.status_code == 201 + return r.json()["user_id"] + + +async def _login(client, username): + r = await client.post("/v1/users/login", + json={"username": username, "password": "testpass99"}) + assert r.status_code == 200 + return r.json()["access_token"] + + +async def _one_notification_for_alice(client): + """A role change is the cheapest thing that notifies somebody.""" + await _register(client, "admin", "admin@x.com") + set_admin_usernames(["admin"]) + admin_token = await _login(client, "admin") + uid = await _register(client, "alice", "a@x.com") + alice_token = await _login(client, "alice") + await client.patch(f"/v1/admin/users/{uid}", json={"role": "moderator"}, + headers={"Authorization": f"Bearer {admin_token}"}) + return alice_token + + +@pytest.mark.asyncio +async def test_a_read_notification_is_gone_from_what_the_spa_asks_for(client): + """The reported bug, as the two requests the application actually makes. + + The second GET is the next launch. Before the fix it answered with the + notification the user had just dismissed. + """ + token = await _login_and_notify(client) + auth = {"Authorization": f"Bearer {token}"} + + listing = await client.get("/v1/notifications?limit=20&unread_only=true", headers=auth) + entries = listing.json()["notifications"] + assert len(entries) == 1, "the fixture should have produced exactly one" + + r = await client.post(f"/v1/notifications/{entries[0]['id']}/read", headers=auth) + assert r.status_code == 200 + + relaunch = await client.get("/v1/notifications?limit=20&unread_only=true", headers=auth) + assert relaunch.json()["notifications"] == [], ( + "a dismissed notification came back on the next launch") + assert relaunch.json()["unread_count"] == 0 + + +@pytest.mark.asyncio +async def test_without_the_filter_it_does_come_back(client): + """The other half of the same fact, so the fix cannot be read as a + coincidence: unfiltered, the endpoint still returns it, and that is the + request the SPA used to make.""" + token = await _login_and_notify(client) + auth = {"Authorization": f"Bearer {token}"} + + nid = (await client.get("/v1/notifications", headers=auth)).json()["notifications"][0]["id"] + await client.post(f"/v1/notifications/{nid}/read", headers=auth) + + unfiltered = (await client.get("/v1/notifications?limit=20", headers=auth)).json() + assert len(unfiltered["notifications"]) == 1 + assert unfiltered["notifications"][0]["read"] is True + + +# `_one_notification_for_alice` is the setup; this alias keeps the tests reading +# as what they are about rather than as their fixture. +_login_and_notify = _one_notification_for_alice + + +@pytest.mark.skipif(not APP.exists(), reason="the SPA sources are not available") +def test_the_spa_asks_for_unread_only(): + """The half a browserless API test cannot reach. + + Both assertions above pass against a SPA that has dropped the parameter — + the bug was never in the endpoint. Reading the source is worth less than + exercising it, and here it is the only thing that catches the defect that + actually happened. + """ + src = APP.read_text() + fetches = re.findall(r"hubFetch\('(/v1/notifications\?[^']*)'", src) + assert fetches, "the notification list fetch is no longer where this reads it" + for url in fetches: + assert "unread_only=true" in url, ( + f"the SPA lists notifications with {url!r} — without unread_only a " + "dismissed notification comes back on the next launch") + + +@pytest.mark.skipif(not APP.exists(), reason="the SPA sources are not available") +def test_the_feed_does_not_style_a_state_it_can_no_longer_show(): + """The fossil that pointed at the bug. + + `n.read ? '' : 'notif-unread'` is from when a read notification stayed on + screen greyed out. Nothing read reaches the feed any more, so the ternary + was dead code that described behaviour the application had abandoned — + and reading it is what made the two halves' disagreement visible. + """ + src = APP.read_text() + assert "n.read ?" not in src, ( + "the feed branches on `read` again; either it is dead code or the " + "dismissal contract has changed and this file should say how") -- cgit v1.2.3