Uh oh!
There was an error while loading. Please reload this page.
Let notifications collapse repeat events onto one row - #704
Merged
Conversation
"3 new comments on earlier chapters" is what every mature product does with a run of related events, and there was no way to say it. `create_inapp/2` only ever inserts, and the only broadcast fired on insert — so a host that wanted collapsing queried the Notification schema directly, `update_all`ed the row, and re-broadcast `:notification_created` by hand. One that tried it with a schemaless write stringified `metadata` into the jsonb column and 500'd that user's bell. `upsert_inapp/3` takes a key: refresh the row already standing for it, or post a new one. Text, icon and link are replaced, other metadata keys are merged, and `inserted_at` moves to now so a refreshed entry sorts as new. Only UNSEEN rows collapse, which is the rule worth stating. Folding a new event into something the user has already read would hide it inside a row they have finished with, and the unread count would not move — the same reasoning as unseen-first ordering. Once read, the next event is news again and gets its own row. Broadcasts either way, `:notification_updated` for a refresh, so a bell that is already open reflects it without the caller broadcasting anything. Dismissed rows are never resurrected, and a row that disappears between the read and the write posts a fresh one rather than losing the event. Two mutations checked: collapsing into read rows fails the suite, and replacing metadata instead of merging fails it twice. Writing the tests also caught two real errors — `updated_at` isn't a column on this schema, and the row-count guard was the only thing standing between a dismissed notification and resurrection.
ddon pushed a commit
that referenced
this pull request
Aug 11, 2026
Review docs for all three PRs in dev_docs/pull_requests/2026/. #704 — two bugs on the read-then-write race in `upsert_inapp/3`: * The `update_all` filtered on `uuid` alone, so the unseen/undismissed rule held only in the read. A row dismissed or read in between was refreshed anyway — the update reported success and the event ended up recorded only on a row that will never be shown again. The guard now appears in the write too, which also makes the fallback reachable for the reason it was written. * The fallback posted the replacement row without its dedupe key, so it could never be found again: every later event for that key opened a new row. One lost race turned collapsing off permanently. #705 — the level-follows-consequence rule was applied to one branch of three. Strict mode refuses every mismatch (`PhoenixKitWeb.Users.Auth` answers `{:warning, _}` exactly as it answers `{:error, _}`), so a strict-mode host logging someone out over a browser update recorded it at `:info` — below the default threshold, i.e. not at all. Extracted `log_mismatch/2`; non-strict behaviour is unchanged in all three branches, which the untouched existing tests demonstrate. #703 — the warning was emitted with `IO.warn/1`, which registers a compiler diagnostic and so failed the build on any host using `--warnings-as-errors`, on upgrade, over a mix.exs condition that is not a regression in their code. That contradicted the PR's own guarantee and the rescue guards written to back it up. Now written straight to stderr; pinned by a test asserting no diagnostic is registered. Also updates deps (phoenix 1.8.10, hackney 4.7.3) and corrects the `version/0` doc example, which still claimed "1.3.3". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From the integration report, item #3.
The gap
"3 new comments on earlier chapters" is what every mature product does with a
run of related events, and there was no way to say it.
create_inapp/2onlyever inserts, and the only broadcast fired on insert.
So a host that wanted collapsing reached past the API: queried the
Notificationschema directly,update_alled the row, then re-broadcast:notification_createdby hand. One that tried it with a schemaless writestringified
metadatainto the jsonb column and 500'd that user's bell — whichis the cost of leaving a schema as the only way to do something.
upsert_inapp/3Takes a key: refresh the row already standing for it, or post a new one.
Text, icon and link are replaced; other metadata keys are merged, so keys
the caller doesn't mention survive.
inserted_atmoves to now, so a refreshedentry sorts as new.
The rule worth stating: only unseen rows collapse
Folding a new event into something the user has already read would hide it
inside a row they've finished with, and the unread count wouldn't move. That's
the same reasoning as the unseen-first ordering in #702. Once read, the next
event is news again and gets its own row.
Dismissed rows are never resurrected either, and a row that vanishes between the
read and the write posts a fresh one rather than losing the event.
Broadcasting
{:notification_created, n}for a new row,{:notification_updated, n}for arefresh — so an already-open bell reflects it without the caller broadcasting
anything.
NotificationsBellhandles the new event;Events' docs list it.Testing
Seven integration tests, including the three refusals (seen, dismissed, other
user) and the merge-not-replace behaviour.
Two mutations checked: collapsing into read rows fails the suite; replacing
metadata instead of merging fails it twice.
Writing them caught two real errors before this went anywhere —
updated_atisn't a column on this schema, and the row-count guard on
update_allwas theonly thing standing between a dismissed notification and resurrection.
149 notification + LiveView tests pass,
credo --strictclean, compiles with--warnings-as-errors. No version bump or CHANGELOG entry.🤖 Generated with Claude Code
https://claude.ai/code/session_01NTj7hm3fpCTcFvKLRtgppW