fix(desktop): clear unread projections from message read markers#1242
Conversation
Align sidebar, Home badge, and Home inbox unread projections with thread badges by considering per-message read markers when evaluating thread replies. Document the per-message read-state context scheme in NIP-RS. Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
Ran a review pass on this. Three things worth a look (one's just a question), plus two doc/clarity nits — none blocking. The core change reads correct and monotone to me: it only ever clears more unread state, never relights read state, which is the safe direction for a badge-clearing fix.
| } | ||
| continue; | ||
| } | ||
| if (localDoneSet.has(item.id)) { |
There was a problem hiding this comment.
Want to confirm this fall-through is intended. On main, a thread item with a null thread marker hit continue and never got checked against localDoneSet — it stayed unread. After the refactor, resolveInboxItemReadAt returns null for that item and we now fall through to this localDoneSet.has(item.id) check, so a thread item I'd locally marked done (with no NIP-RS marker yet) now reads as done where it used to read unread. I think that's actually the more consistent behavior, just want to make sure it's deliberate — and if so maybe a line in the PR body, since it's a behavior change outside the stated scope.
| const markers: Array<number | null> = []; | ||
|
|
||
| if (item.channelId) { | ||
| markers.push(options.getChannelReadAt(item.channelId)); |
There was a problem hiding this comment.
For a thread reply this pushes the raw channel marker alongside the thread and msg markers, but both of those already fold the channel frontier — getThreadReadAt maxes in the channel marker, and getMessageReadAt is getChannelReadAt(msgContextKey(id)). So under the max the raw channel push is always dominated when we're on the thread branch. Could drop it from the thread-reply path? It's still doing real work for the non-thread channel item, just not here — and leaving it in reads like channel matters independently for thread replies, which it doesn't.
| markers.push(options.getMessageReadAt?.(item.id) ?? null); | ||
| } | ||
|
|
||
| return markers.reduce<number | null>((latest, marker) => { |
There was a problem hiding this comment.
This max-reducer is now written three times — here, maxReadAt in useHomeInboxReadState.ts:84, and the reduce in unreadChannelCounts.ts:133 — all byte-identical. Since one's already a named maxReadAt helper, maybe lift it into the readState module next to msgContextKey and import it in all three? Keeps the three from drifting.
| // A well-formed per-message context key: the msg: prefix with a non-empty id | ||
| // that does NOT itself start with thread: (guards against a thread key being | ||
| // mistaken for, or double-prefixed into, a message key). | ||
| export function isThreadContextKey(value: string): value is `thread:${string}` { |
There was a problem hiding this comment.
Minor — isThreadContextKey and isMsgContextKey don't have any production callers right now (only readStateFormat.test.mjs exercises them); the write path doesn't validate and eviction matches on raw startsWith. So the strict 64-hex tightening is safe (no persisted key gets re-classified at runtime), but these are effectively spec-conformance assertions for the tests rather than live guards. Might be worth a one-line comment saying so, so nobody assumes they're gating the read path.
| If the thread root event (and therefore its parent channel) cannot be resolved | ||
| from the event graph, `effective(thread:<root>)` degrades to | ||
| `merged[thread:<root>]` alone. | ||
| A thread is unread iff at least one reply is unread. A reply is unread iff |
There was a problem hiding this comment.
Small doc/code mismatch: the prose says a reply is unread iff reply.created_at > effective(msg:<reply-id>) with effective(msg) = max(msg, channel), but the implementation also folds the thread marker — max(channel, thread, msg). It's monotone-safe so not wrong, but the stated per-message predicate omits the thread term the code blends in. Either tighten the prose to max(channel, thread, msg) or note that a per-message client MAY additionally fold the thread marker.
Preserve the existing Home local-done fallback boundary for channel/thread rows, share the read-marker max helper across projections, and align NIP-RS prose with the implemented reply effective frontier. Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
2 participants
Summary
msg:<event-id>read markers for observed thread reply events.msg:<event-id>read-state context scheme indocs/nips/NIP-RS.mdwhile preserving the existing flat encryptedkind:30078wire format.Why
Opening a thread could clear the thread button's
(N new)count because that surface reads per-replymsg:<reply-id>markers. The sidebar and Home projections still evaluated the same reply activity without that marker, so delayed replay/catch-up could relight already-read thread replies.Testing
PATH="$PWD/bin:$PATH" just desktop-check desktop-build test-unitPATH="$PWD/bin:$PATH" just desktop-testcd desktop && pnpm build && pnpm exec playwright test --project=smoke tests/e2e/thread-unread-screenshots.spec.ts