Uh oh!
There was an error while loading. Please reload this page.
Sort a user's notifications unseen first - #702
Merged
Merged
Conversation
Newest-first alone interleaves read and unread: opening a notification leaves it exactly where it was, so anything still outstanding ends up scattered among things already dealt with. Read a new arrival and the older unread one is now sitting underneath it. On an inbox of any size that means scrolling the whole list hunting for what was missed — the one question a notification centre exists to answer at a glance. Unseen now sort above seen, newest first within each group. Reading something drops it below whatever is still outstanding, so the unread block stays contiguous at the top. It matters more in the bell than in the full list. The dropdown shows ten, so with a plain newest-first order a notification already read could push an unread one off the bottom entirely — the badge counts it and opening the bell does not show it. Nothing moves while it is being looked at: "seen" is only ever set by an explicit action, and the bell either navigates away or refreshes on the click, so the reorder lands on the next look rather than under the cursor. `admin_list/1` is deliberately left plain newest-first. "Seen" belongs to the recipient, and an admin scanning everybody's notifications is reading a chronological record, not working through their own inbox. `IS NOT NULL` rather than an inverted `is_nil`, because false sorts before true — so unseen comes first without a negation to read past.
ddon pushed a commit
that referenced
this pull request
Aug 11, 2026
…total, and bump to 2.1.0 Post-merge review of #702. Unseen-first restarts the day sequence at the seen block, so the inbox's day chunking emitted the same header twice; sections are now keyed on {unread?, day}. inserted_at is second-granularity, so ties under LIMIT/OFFSET were unstable — uuid is now the final sort key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 11, 2026
ddon pushed a commit
that referenced
this pull request
Aug 14, 2026
An adversarial review of the notifications-upsert, fingerprint-logging and js-compiler-warning work (PRs #702-#705) confirmed ten findings. All ten are fixed here; the two that needed schema support ride a new migration. Notifications (V170 + code): The dedupe lookup and the unseen-first ordering had no index support — recent_for_user runs on every bell mount and had gone from index-served to fetch-everything-and-sort. V170 adds two indexes: a partial UNIQUE on (recipient_uuid, metadata->>'dedupe_key') over undismissed unseen keyed rows, which serves find_collapsible's exact predicate AND closes the find-then-insert race — two workers inserting the same absent key now trip the constraint, and insert_collapsible retries the find and folds. Pre-existing duplicate unseen rows are folded (all but the newest per key marked dismissed) under the same table lock that creates the index, so nothing slips into the gap. The second index matches order_unseen_first's expression term-for-term. The anticipated constraint trip is not logged as a failure; every other insert error still is. upsert_inapp now honors the notifications_enabled kill switch like create/1 — it is a host-facing entry point, and "off" that quietly did not apply to the newest creation path was not off. Caller metadata is merged FIRST and the reserved keys stamped on top, so a passed-through metadata map can no longer clobber the dedupe_key (silently disabling collapsing) or the display keys; caller keys are normalized to strings so %{notification_text: ...} cannot coexist with the string key and win adapter-order-dependently. find_collapsible gains the catch :exit the project's soft-failure rule requires (a dead pool EXITS, bypassing rescue and crashing the caller the comment promised it would not), and both rescue and catch now log, so a permanent query bug degrading upsert into insert-always is diagnosable. find_collapsible also tie-breaks on uuid below inserted_at's second granularity. The inbox's handle_info whitelist gains :notification_updated — the bell had it, the inbox did not, so an open inbox showed stale rows exactly when upsert refreshed one; a scrape test now holds the whitelist to every event the library broadcasts. Fingerprint logging: The dedup had only landed in fetch_phoenix_kit_current_user; fetch_phoenix_kit_current_scope still carried the old "(scope)" warning and the ":error possible hijacking" line for requests that were then served — and the shipped :phoenix_kit_admin_only pipeline runs BOTH plugs, so a mismatch logged three lines. Verification now runs at most once per request (verdict cached in conn.private, both plugs share it) with no extra logging on any branch. session_label is now THE SAME derivation the sessions UI shows as its token preview (hex of the raw token's first 4 bytes) — the truncated-sha256 label could never match it, so the correlation the comment promised failed every time an operator tried it. JS-compiler warning: warn_missing_js_compiler read Mix.Project.config() of whatever project was compiling, so a module package running its own test suite (its router expands phoenix_kit_routes(), discovery finds its own beam) got the fix-your-mix.exs warning on every compile for a configuration that was correct. The warning is now suppressed when any warned module was compiled from the current project's own source tree — excluding deps/, which sits UNDER cwd; the first cut missed that and the test caught every hex dep reading as locally compiled. And modules_declaring_js_sources gains a catch clause: a throw or exit from a discovered module's js_sources/0 escaped the rescue and failed the host's compile, precisely what the adjacent guarantee promises cannot happen. The expected-schema manifest carries the two V170 indexes hand-declared with definitions and opclasses captured from pg_get_indexdef on a live database (not hand-derived), and chain_hash was restamped over the 36 shipped files — the same treatment the V165..V167 objects received, with the same caveat recorded: verify.exs s7/s8 against a real database is what proves the manifest body, and still requires a pre-squash checkout. Full suite: 3536 tests, 0 failures, database reachable (integration tests ran). The V169 lock-guard and version-headings tests caught two omissions in the first cut of V170 — both fixed, both now covered.
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
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.
The notification centre sorts newest first, which interleaves read and unread.
Opening a notification leaves it exactly where it was, so anything still
outstanding ends up scattered among things already dealt with — read a new
arrival and the older unread one is now sitting underneath it.
On an inbox of any size that means scrolling up and down hunting for what was
missed, which is the one question a notification centre exists to answer at a
glance.
The change
Unseen sort above seen; newest first within each group. Reading something drops
it below whatever is still outstanding, so the unread block stays contiguous at
the top.
IS NOT NULLrather than an invertedis_nil, because false sorts before true— unseen comes first without a negation to read past.
Applies to
list_for_user/2andrecent_for_user/2.Why it matters more in the bell
The dropdown shows ten. With plain newest-first, a notification already read
could push an unread one off the bottom entirely — so the badge counts something
opening the bell doesn't show. There's a test for exactly that.
Nothing shuffles while you're looking at it
"Seen" is only ever set by an explicit action, and on that click the bell either
navigates away or refreshes — so the reorder lands on the next look rather
than under the cursor. No change was needed there; worth stating because the
opposite would be worse than the bug.
Deliberately unchanged
admin_list/1stays plain newest-first. "Seen" belongs to the recipient, and anadmin scanning everybody's notifications is reading a chronological record, not
working through their own inbox — sorting a shared audit feed by whether someone
else has read each row reorders it differently for nobody's benefit. Recorded in
the doc so the inconsistency doesn't look like an oversight.
Testing
Four integration tests, pinned against the database rather than a sort function,
since ordering is a promise the UI makes and this is what a host embedding the
bell actually gets:
Mutation-tested: reverting to plain newest-first fails three of the four. The
fourth is the all-seen fallback, which correctly shouldn't change either way.
One note on the second test — my first version marked the two oldest as read,
where both orderings agree on the result and it proved nothing. It now marks the
two newest, so the expectation (
b, a, d, c) differs from what newest-firstwould give (
d, c, b, a).45 notification tests and 101 LiveView tests pass;
credo --strictclean;compiles with
--warnings-as-errors.One thing worth a look separately
n_recipient_inbox_indexis(recipient_uuid, inserted_at DESC) WHERE dismissed_at IS NULL. It still serves the fetch, but the new sort key isn't init, so ordering happens in memory. Irrelevant at inbox sizes — retention prunes
at 90 days by default — but if these ever grow large the index wants
(recipient_uuid, (seen_at IS NOT NULL), inserted_at DESC). Not added here:that's a migration, and the chain isn't mine to extend on a whim.
No version bump or CHANGELOG entry.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NTj7hm3fpCTcFvKLRtgppW