From 16b5e5c463ec998c6434bbf2ef39113ed01da801 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Wed, 2 Sep 2026 12:55:05 +0200 Subject: fix(hub): dismissing a notification deletes it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit filtered the list to unread, which corrected what the reader saw and left every dismissed row in the table, invisible for ever. That is a place to hide the disagreement rather than a resolution, and the operator said so: "elles s'accumulent pour rien." So dismissing drops the row. It is the reasoning `purge_notifications` has carried all along — "these are signals, not a record: the group is still there, the message is still in the chat, the invitation is still an invitation" — applied one at a time instead of only in bulk. - `DELETE /v1/notifications/{id}` is the honest name and what the SPA calls. - `POST /{id}/read` reaches the same handler and now deletes too. It has to keep working: the interface ships inside the desktop package, so a hub is always answering some client older than itself, and giving the old path the new behaviour means those clients stop hoarding as well rather than only the updated ones. - `read-all` deletes rather than marking, which makes it `DELETE ""` under an older name. Marking would have made it the one route still filling the table. Nothing in this repo calls it, but a reachable endpoint is one that can be called. `Notification.read` is now vestigial — nothing stored can be read, because reading it deletes it. It stays because dropping a column is a migration for no gain, and `unread_only` stays because a SPA newer than its hub still needs it to be right. Both are said in the module docstring rather than left to be worked out. Two existing tests encoded the old semantics and now assert the opposite; test_notification_dismissal.py gains one for the old `/read` path, because version skew is the normal case here and not the exception. 617 hub tests pass. docs/USERGUIDE.md's endpoint table updated in both places it lists them. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014UtzVrzM7e2tG9fSpkR9ML --- .../src/meshbay_hub/api/notifications.py | 61 +++++++++++++++++----- packages/meshbay-hub/src/meshbay_hub/static/app.js | 8 +-- .../tests/test_notification_dismissal.py | 48 +++++++++++++---- packages/meshbay-hub/tests/test_notifications.py | 13 +++-- 4 files changed, 103 insertions(+), 27 deletions(-) (limited to 'packages/meshbay-hub') diff --git a/packages/meshbay-hub/src/meshbay_hub/api/notifications.py b/packages/meshbay-hub/src/meshbay_hub/api/notifications.py index ca56620..9d5c125 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/notifications.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/notifications.py @@ -1,9 +1,28 @@ -"""Notification endpoints — /v1/notifications/*""" +""" +Notification endpoints — /v1/notifications/* + +**Dismissing one deletes it.** These are signals, not a record: the group is +still there, the message is still in the chat, the invitation is still an +invitation, so nothing is lost by dropping the row — which is what +`purge_notifications` below has always said, now applied to one at a time. + +That resolves a disagreement between two halves that were each defensible +alone. The interface treats a click as "this is gone" and removes the entry; +the hub marked it read and kept it; and the next launch listed read entries +too, so everything dismissed came back. Filtering the list to unread fixed +what the user saw and left the rows accumulating for nothing, invisible for +ever — which is the state this replaces. + +`Notification.read` is therefore **vestigial**: nothing stored can be read, +because reading it deletes it. It stays because dropping a column is a +migration for no gain, and `unread_only` stays because it is what an older +interface asks for and it still answers correctly — every row is unread. +""" from fastapi import APIRouter, Depends, HTTPException from datetime import datetime, timezone -from sqlalchemy import delete, func, select, update +from sqlalchemy import delete, func, select from sqlalchemy.ext.asyncio import AsyncSession from meshbay_hub.api.deps import get_current_user @@ -52,16 +71,27 @@ async def list_notifications( } +@router.delete("/{notification_id}") @router.post("/{notification_id}/read") -async def mark_read( +async def dismiss( notification_id: int, current_user: User = Depends(get_current_user), db: AsyncSession = Depends(get_db), ): + """ + Dismiss one. The row goes. + + Two paths to the same handler. `DELETE /{id}` says what happens and is what + the interface calls; `POST /{id}/read` is what every already-installed + client calls, and it has to keep working — the SPA ships inside the desktop + package, so a hub is always talking to some interface older than itself. + Giving the old path the new behaviour means those clients stop accumulating + rows too, rather than only the ones that have been updated. + """ notif = await db.get(Notification, notification_id) if not notif or notif.user_id != current_user.id: raise HTTPException(status_code=404, detail="Notification not found") - notif.read = True + await db.delete(notif) await db.commit() return {"status": "ok"} @@ -76,7 +106,8 @@ async def purge_notifications( These are signals, not a record: the group is still there, the message is still in the chat, the invitation is still an invitation. Nothing is lost by - clearing the list, so it clears rather than marking a hundred rows read. + clearing the list, so it clears rather than marking a hundred rows read — + the reasoning the whole module now follows. """ result = await db.execute( delete(Notification).where(Notification.user_id == current_user.id)) @@ -85,17 +116,23 @@ async def purge_notifications( @router.post("/read-all") -async def mark_all_read( +async def dismiss_all( current_user: User = Depends(get_current_user), db: AsyncSession = Depends(get_db), ): - await db.execute( - update(Notification) - .where(Notification.user_id == current_user.id, Notification.read == False) # noqa: E712 - .values(read=True) - ) + """ + Dismiss every one — the same thing as `DELETE ""`, under the name an older + client knows it by. + + Marking them read instead would put back exactly what this change removes: + rows the list can never show again. Nothing in this repository calls it, + but an endpoint that is reachable is an endpoint that can be called, and it + should not be the one route that still hoards. + """ + result = await db.execute( + delete(Notification).where(Notification.user_id == current_user.id)) await db.commit() - return {"status": "ok"} + return {"status": "ok", "removed": result.rowcount} async def create_notification( diff --git a/packages/meshbay-hub/src/meshbay_hub/static/app.js b/packages/meshbay-hub/src/meshbay_hub/static/app.js index 7cc7c83..5311403 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/app.js @@ -650,7 +650,9 @@ function App() { // the page navigates, which reads as "the click did nothing". setNotifications(prev => prev.filter(n => n.id !== id)); setUnreadCount(c => Math.max(0, c - 1)); - hubFetch(`/v1/notifications/${id}/read`, { method: 'POST', token: user.token }) + // DELETE, not `/read`: dismissing one drops the row. The old path still + // works and still deletes, for interfaces older than the hub. + hubFetch(`/v1/notifications/${id}`, { method: 'DELETE', token: user.token }) .catch(() => fetchNotifications()); }, [user, fetchNotifications]); @@ -667,8 +669,8 @@ function App() { if (!user) return; setNotifications(prev => { const gone = prev.filter(n => n.group_id === groupId && n.kind === 'group_invite'); - gone.forEach(n => hubFetch(`/v1/notifications/${n.id}/read`, - { method: 'POST', token: user.token }).catch(() => {})); + gone.forEach(n => hubFetch(`/v1/notifications/${n.id}`, + { method: 'DELETE', token: user.token }).catch(() => {})); if (gone.length) setUnreadCount(c => Math.max(0, c - gone.length)); return prev.filter(n => !gone.includes(n)); }); diff --git a/packages/meshbay-hub/tests/test_notification_dismissal.py b/packages/meshbay-hub/tests/test_notification_dismissal.py index 7324656..07cf715 100644 --- a/packages/meshbay-hub/tests/test_notification_dismissal.py +++ b/packages/meshbay-hub/tests/test_notification_dismissal.py @@ -15,8 +15,15 @@ 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. +Filtering the list to unread was the first fix. It corrected what the user saw +and left the rows accumulating for nothing, invisible for ever, which is not a +resolution so much as a place to hide the disagreement. **Dismissing one now +deletes it** — the reasoning `purge_notifications` always carried, applied one +at a time: these are signals, not a record. + +So what this pins is the contract between the two halves: the hub drops the +row by either path, old client or new, and the SPA still sends the filter for +the case where it is newer than the hub it is talking to. """ import re @@ -91,19 +98,42 @@ async def test_a_read_notification_is_gone_from_what_the_spa_asks_for(client): @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.""" +async def test_it_is_gone_from_the_unfiltered_list_too(client): + """The filter is no longer what makes it disappear — the row is. + + This assertion is the inverse of what it was. Filtering to unread fixed + what the user saw and left the rows behind, invisible for ever; dismissing + now deletes, so even a client that asks for everything sees nothing. The + filter stays because an interface older than the hub still sends it. + """ 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) + await client.delete(f"/v1/notifications/{nid}", 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 + assert unfiltered["notifications"] == [] + + +@pytest.mark.asyncio +async def test_the_old_read_path_dismisses_too(client): + """Version skew is the normal case here, not the exception. + + The SPA ships inside the desktop package, so a hub is always talking to + some interface older than itself — CLAUDE.md records that as a standing + consequence of shipping the UI in a package. An old client calling + `/read` must stop accumulating rows as well, or the fix only reaches + whoever updated. + """ + token = await _login_and_notify(client) + auth = {"Authorization": f"Bearer {token}"} + + nid = (await client.get("/v1/notifications", headers=auth)).json()["notifications"][0]["id"] + r = await client.post(f"/v1/notifications/{nid}/read", headers=auth) + assert r.status_code == 200 + + assert (await client.get("/v1/notifications", headers=auth)).json()["notifications"] == [] # `_one_notification_for_alice` is the setup; this alias keeps the tests reading diff --git a/packages/meshbay-hub/tests/test_notifications.py b/packages/meshbay-hub/tests/test_notifications.py index 8da589a..35ed288 100644 --- a/packages/meshbay-hub/tests/test_notifications.py +++ b/packages/meshbay-hub/tests/test_notifications.py @@ -92,7 +92,7 @@ async def test_notification_on_suspend(client): @pytest.mark.asyncio -async def test_mark_notification_read(client): +async def test_dismissing_one_deletes_it(client): _, admin_token = await _setup_admin(client) uid = await _register(client, "alice", email="a@x.com") alice_token = await _login(client, "alice") @@ -105,6 +105,8 @@ async def test_mark_notification_read(client): headers={"Authorization": f"Bearer {alice_token}"}) nid = r.json()["notifications"][0]["id"] + # `/read` is the old path and still the one older clients call. It + # dismisses, like DELETE — see api/notifications.py. r = await client.post(f"/v1/notifications/{nid}/read", headers={"Authorization": f"Bearer {alice_token}"}) assert r.status_code == 200 @@ -112,11 +114,13 @@ async def test_mark_notification_read(client): r = await client.get("/v1/notifications", headers={"Authorization": f"Bearer {alice_token}"}) assert r.json()["unread_count"] == 0 - assert r.json()["notifications"][0]["read"] is True + assert r.json()["notifications"] == [], ( + "a dismissed notification is deleted, not kept as a row nothing can " + "ever show again") @pytest.mark.asyncio -async def test_mark_all_read(client): +async def test_dismissing_all_deletes_them(client): _, admin_token = await _setup_admin(client) uid = await _register(client, "alice", email="a@x.com") alice_token = await _login(client, "alice") @@ -135,10 +139,13 @@ async def test_mark_all_read(client): r = await client.post("/v1/notifications/read-all", headers={"Authorization": f"Bearer {alice_token}"}) assert r.status_code == 200 + assert r.json()["removed"] == 2 r = await client.get("/v1/notifications", headers={"Authorization": f"Bearer {alice_token}"}) assert r.json()["unread_count"] == 0 + assert r.json()["notifications"] == [], ( + "read-all must not be the one route that still hoards rows") @pytest.mark.asyncio -- cgit v1.2.3