Skip to content

feat(notifications): an in-app inbox for mentions, replies, tasks and shares - #23

Open
christianhuening wants to merge 24 commits into
mainfrom
feat/notifications-inbox
Open

feat(notifications): an in-app inbox for mentions, replies, tasks and shares#23
christianhuening wants to merge 24 commits into
mainfrom
feat/notifications-inbox

Conversation

@christianhuening

Copy link
Copy Markdown
Contributor

An @mention in a comment used to fire a pg_notify('comment_mentions', …) that no process in the codebase ever listened for, into an MSG_MENTION WebSocket frame the frontend had reserved and never received. Mentioning a colleague did nothing they would ever see, and KnotProvider.ts had said so in a comment since June.

Five events now write to a notifications table, read through an Inbox in the sidebar with an unread badge, a dropdown, and a /notifications page: mentions, replies in threads you're part of, task assignment, overdue tasks, and documents shared with you.

Design notes

  • Delivery is a 30-second poll, plus refetch on window focus. Deliberately no push channel: an inbox has to reach you when you have no document open, and the doc WebSocket is keyed by doc_id, so it can't. The reserved MSG_MENTION plumbing is deleted rather than completed. SSE can replace polling later behind the same API and schema.
  • Idempotency is a unique index on (user_id, dedupe_key) with ON CONFLICT DO NOTHING, not application logic. That's what lets the overdue sweep run on every replica with no leader election.
  • task_assigned's dedupe key is content-addressed (sha256(text)), because doc_tasks.id is "<doc_id>:<item_index>" and every reindex rewrites it — an id-keyed notification would re-fire for every assignee each time anyone reordered a checklist.
  • The table is an outbox. emailed_at exists and nothing writes it, so an email transport can be added without a migration.
  • Reads re-check ACLs. A notification can outlive access to its document; the list handler re-checks effective_role and filters, never 403s.

Also fixed

Members whose display name contains a space could never be mentioned in a comment. The server matched @(\w+) against display names, so @Christian Hüning captured Christian, matched nobody, and silently notified no one. The picker now sends the user ids it resolved, unioned with the regex fallback for names typed by hand. The comment edit path is not covered — it has no picker.

Accepted limitations (documented in the spec)

  • Self-assignment isn't suppressed when you assign yourself a task by typing. The guard needs an actor and the live-edit path has none: WS updates persist with by_user_id: None and the reindex channel carries only a document id. Threading identity through the collaborative hot path wouldn't clearly be correct anyway — under co-editing, several people may have edited between reindexes.
  • Two checklist items with identical text assigned to the same person notify once, a direct consequence of content-addressing. Both still appear on /tasks.

Known follow-ups, not addressed here

  1. Editing an assigned checklist item spams its assignee. The reindexer flushes every 2s, so each intermediate text hashes to a fresh key and emits a fresh unread row — roughly 15 notifications for 30s of rewording. Pre-existing, but exempting task_assigned from retention (needed to stop pruning resurrecting old assignments) removed the ceiling that bounded it. Worth fixing before this reaches users.
  2. The sweep's 7-day floor has no test — deleting the line leaves all four sweep tests green.
  3. Spec §2's key table says <due date> for task_due where the code binds the sweep date; §4's prose is right.
  4. The seed migration covers unchecked tasks only, so re-opening a task that was checked at upgrade emits one stale notification.

Upgrade safety

A second migration adds a CHECK on kind and pre-seeds already-read task_assigned dedupe rows for currently-assigned tasks, so an existing deployment doesn't get a burst covering its entire historical backlog on first reindex. The SQL and Rust dedupe keys were verified byte-identical against live Postgres 18, including 2-, 3- and 4-byte UTF-8.

Testing

cargo nextest 373/373 · clippy -D warnings · cargo fmt --check · tsc · eslint · vitest 169/169. New e2e notifications.spec.ts passes: it mentions "Mara Jade", a name the server's regex physically cannot resolve, so the test passing is the proof that the picker sent a real user id.

Design: docs/superpowers/specs/2026-09-06-notifications-inbox-design.md

🤖 Generated with Claude Code

christianhuening and others added 24 commits September 6, 2026 15:24
Closes the "Plan 19.5 — Mention push bridge" deferral by widening it: an
@mention that only reaches you while you already have the document open
is not worth the wire, so delivery is per-user rather than per-doc and
the reserved MSG_MENTION plumbing goes away instead of getting finished.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
14 tasks, TDD throughout. Two spec amendments fell out of writing it: a
trait object cannot join the caller's transaction, so request-path emits
are at-most-once after commit (task_assigned is the exception and writes
inside the reindex transaction); and comment bodies render as plain text,
so mention identity travels as an explicit id list rather than the
editor's knot://user/<uuid> sentinel.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Idempotency lives in the notifications_dedupe unique index rather than in
application logic, so concurrent emits from several replicas converge on
one row without coordination.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…unread_count cap

Remove set_ages_for_test and hard_delete_doc_for_test from the production
store type; the same SQL now runs directly against store.pool() from the
test file. Also add an assertion that unread_count's LIMIT actually clips
the result, not just bounds it from above.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the pg_notify('comment_mentions') call nothing ever listened to.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Editing a comment re-ran the full thread-participant fan-out, so a typo
fix told every participant "Alice replied" when nobody did. Mentions on
edit are still correct and unaffected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The display-name regex captures @christian and matches no member, so
anyone whose name contains a space was unmentionable. The picker knows
who it picked; it now says so. Regex kept for existing clients.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
doc_tasks ids are '<doc_id>:<item_index>' and every reindex rewrites
them, so an id-keyed notification would re-fire on every reorder.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-assignment suppression only has an effect when upsert_for_doc is
called with a known actor_id (the markdown/workspace import paths). On
the live-editing path the reindex worker only ever has a doc id - the
CRDT room persists updates with no editor identity - so it always
passes actor_id: None, and the guard never suppresses there. The doc
comment on upsert_for_doc now says so plainly, and
assignment_with_no_known_actor_still_notifies pins the behavior as
deliberate rather than leaving it undocumented.

Also strengthens assigning_a_task_notifies_once_and_survives_a_reorder
to actually discriminate content-keyed dedupe from an id-keyed one: the
two swapped items now have different assignees, so the swap pairs each
task id with an assignee it never held before. Verified against a
temporarily id-keyed dedupe key that the test fails without this fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every replica runs it; the dedupe index makes concurrent sweeps
idempotent, so there is no leader election to operate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The list handler awaited effective_role once per row in a for loop, so a
cache-cold page of up to 100 rows serialized that many round trips
through acl::resolve behind a single response. Run the checks with
futures::future::join_all instead, preserving the existing semantics
(no doc_id -> keep, Ok(None) -> drop, Err -> fail the whole request
rather than silently dropping the row) and computing next_cursor from
the raw page before filtering, unchanged.

Also documents why unread_count's lack of a per-row ACL re-check is
safe for the current single-workspace deployment, and what changes when
that assumption stops holding.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
useUnreadCount and useNotificationList threw bare Error(message) on API
failure, discarding res.error.status. queryClient's retry predicate keys
off "status" in error to skip retries on 4xx, so every 401/403/404 was
retried twice instead of failing fast. Route both hooks (and useMarkRead,
for consistency) through a shared unwrapResult() that attaches status and
code to the thrown value, and pin the behavior with a unit test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Move outside-click/Escape dismissal for the notification dropdown from
NotificationDropdown into WorkspaceHeader, which already owns inboxOpen
and wraps both the trigger and dropdown in one container ref. This
drops a data-testid based DOM lookup that only worked because that id
happens to be frozen by contract, and stops the effect from
tearing down/reattaching on every unrelated re-render (e.g. the 30s
unread-count poll) by keying it off the inboxOpen boolean instead of a
fresh onClose closure.

Also add menu accessibility semantics (aria-haspopup/aria-expanded on
the trigger, role="menu"/"menuitem" on the dropdown, focus return to
the trigger on Escape/outside-click dismissal, aria-pressed on the
inbox page's filter toggles) and correct a docstring that claimed the
dropdown sorts unread-first when it does not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Also deletes the MSG_MENTION plumbing reserved in June: delivery is
per-user now, so the doc WebSocket is not on the path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…m final review

Fixes the ten findings from the whole-branch review of the notifications
inbox before merge:

- task_assigned now skips checked items, and a new migration pre-seeds
  read dedupe rows for already-assigned tasks so an existing deployment's
  first reindex after upgrade doesn't burst-notify the entire historical
  backlog (also adds the notifications.kind CHECK constraint, now that
  PgTaskStore and the sweep write the table directly).
- The overdue sweep gets a 7-day floor for the same upgrade reason, and
  task_due switches from the reorder-fragile doc_tasks.id key to the same
  content-addressed scheme as task_assigned.
- task_assigned is excluded from retention pruning, since its row is the
  only thing suppressing a reindex re-notify.
- Comment mentions now union explicit picker ids with the display-name
  regex instead of either/or, so a hand-typed second name is no longer
  silently dropped.
- Actor-less notifications (task_due, and task_assigned on the live-edit
  path) render as a plain statement instead of attributing to "knot".
- Task notification payloads write `excerpt` to match what the UI reads.
- Adds coverage for POST /notifications/read {"all": true} and the
  unread_count capped branch; drops a picked mention id from the comment
  composer if its name is backspaced out before submit; increments the
  task_assigned metric, which the raw-SQL insert previously bypassed.
- Scopes the CHANGELOG's mention-fix entry to the create path only.

Updates the design spec's §1-§4 and Testing section wherever behaviour
changed, and records why the "sole write path" justification for skipping
a kind CHECK no longer holds.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant