fix: dedup reactions and recipe comment counts#321
Merged
Conversation
Main like / reaction button on feed and recipe pages was double-
counting locally on click (N → N+2) while other clients saw the
correct single reaction. Read-side dedup bug — the published
kind-7 event is spec-compliant and unchanged.
Root cause
----------
`ReactionTrigger.handleReaction()` did the optimistic
`store.update(count++)` and then `await publishReaction(...)`, but
never called `trackOptimisticReaction(...)` or
`markEventAsProcessed(...)` on engagementCache. The relay echo of
the user's own reaction came back through the shared
persistent subscription and entered `processReaction`, which:
- checked `optimisticReactions.has(key)` → false (never registered)
- fell through to the `processedReactionPairs.has(pair)` check
(first time this `pubkey:emoji` pair for this target → passes)
- ran `data.reactions.count++`
Net: +1 optimistic + +1 echo = +2.
Fix
---
Match the pattern already used by ReactionPills.svelte, which was
correct:
1. `trackOptimisticReaction(event.id, emoji, $userPublickey)`
synchronously, BEFORE the optimistic store update and BEFORE
any await. This is the load-bearing step — the subscription
echo can race any point after `publishReaction` starts, and
the dedup sentinel must be in place before the echo arrives.
2. Optimistic `store.update(count++)` (unchanged).
3. `await publishReaction(...)` (unchanged).
4. On success with a signed event id: also call
`markEventAsProcessed(event.id, result.id)` as secondary
protection against the emoji-normalization edge where `+`
and `❤️` would produce different optimistic keys.
5. On failure: `clearOptimisticReaction(...)` to let a retry
re-register cleanly, and revert the optimistic store update.
Scope
-----
- ReactionTrigger is shared by NoteTotalLikes (feed) and
Recipe/TotalLikes (recipe). This single change fixes both
contexts.
- Event publishing is untouched: no changes to `reactionEvent.tags`
or `reactionEvent.content`. Other clients (Primal, Nostrudel,
etc.) render the same kind-7 events correctly before and after
this patch.
- `engagementCache.processReaction` is untouched. The bug was in
the caller, not the cache.
Verified
--------
- `pnpm exec eslint` on the touched file: clean.
- Tag structure diff vs main: identical (no change to published
event bytes).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-comment like button (inside CommentThread) was incrementing
likeCount by 2 on a single click. Classic Fingerprint A — dedup
sentinel registered AFTER publish, racing the subscription echo.
Root cause
----------
toggleLike() built, published, then incremented:
addClientTagToEvent(reactionEvent);
await reactionEvent.publish();
liked = true;
likeCount++;
`processedLikes.add(reactionEvent.id)` was never called. When the
relay echoed the reaction back through the instance-local
subscription (registered at line 200), the `on('event')` handler
on line 205-211 hit:
if (processedLikes.has(e.id)) return; // false — never added
processedLikes.add(e.id);
likeCount++; // +1
Then the post-publish code ran:
likeCount++; // +1 again
Net: +2 on a single click. Matches the user-visible bug 1:1.
Fix
---
Sign first to obtain the id, register the id synchronously, THEN
publish. Optimistic UI state (likeCount, liked) bumps between
registration and publish so the click still feels instant.
Order now:
1. Build the event + tags + client tag.
2. `await reactionEvent.sign()` to populate `.id`.
3. `processedLikes.add(reactionEvent.id)` — sentinel is in place
before publish can echo.
4. Optimistic `likeCount++` and `liked = true`.
5. `await reactionEvent.publish()`.
6. Catch: revert `likeCount--`, `liked = false`, and
`processedLikes.delete(reactionEvent.id)` so the same id can
be re-registered on retry.
Scope
-----
- Event tag structure and content are unchanged — the published
kind-7 event bytes are identical before and after this fix.
- CommentCard is rendered in both feed and recipe CommentThread
variants; this single change fixes both.
- Subscription handler on lines 205-215 is untouched. The bug was
the caller forgetting to register; adding the registration makes
the existing guard work.
Verified
--------
- pnpm exec eslint: 3 pre-existing `any` errors at lines 92/99/173
remain (untouched by this patch). No new errors at the edited
region.
- Tag structure diff vs main on reactionEvent: identical.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Consolidate subscription creation into onMount + a guarded beforeUpdate hook. Previously `onMount(loadReactions)` and a reactive `$: if (event?.id) loadReactions()` both fired on first mount, creating two `$ndk.subscribe(...)` references. `subscription` was reassigned by each call without the prior one being stopped; onDestroy only stopped the latest, so the first leaked for the lifetime of the session (or the NDK pool's idle timeout). It's a memory leak rather than a count double-count: this component's handler rebuilds `reactionEvents` and runs `aggregateReactions` which recomputes count from scratch each tick, so an extra subscription contributing events doesn't inflate the number visibly. But every mount leaves an orphaned subscription open. Note: `ReactionButton.svelte` is NOT referenced anywhere in the current codebase — zero `<ReactionButton` usages. This fix is defensive: if the component gets wired up in the future, its lifecycle will be correct rather than leaking subs. If the intention is to delete the file outright, this patch makes that decision safer to defer. Fix --- - Drop the reactive `$: if (event?.id) loadReactions()`. - `onMount`: set `mounted = true`, capture `lastEventId = event.id`, load once. - `beforeUpdate`: if `mounted && event.id !== lastEventId`, stop the old subscription, update `lastEventId`, and load again. The `mounted` guard prevents the pre-mount tick (Svelte 4 runs beforeUpdate before initial onMount) from double-firing. - `onDestroy`: unchanged — stops whatever subscription is current. Verified -------- - pnpm exec eslint: pre-existing `fastCountLoaded` unused-var error on line 19 remains (untouched). No new errors. - Not touching `loadReactions()`, `handleReaction()`, or the subscription filter — this is a lifecycle-only change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ation
Recipe comment count inflated to 2–5× the true value on page load.
Other Nostr clients render the correct count, so the event data is
fine — this was a local read-side dedup miss.
Root cause
----------
`TotalComments.svelte` runs a standalone subscription (separate from
`createCommentSubscription` used by CommentThread) and counts events
with:
let eventCount = 0;
subscription.on('event', () => {
eventCount++;
...
});
No dedup. The NDK subscription pool delivers the same kind-1111
NIP-22 comment event once per relay that holds it; a recipe with
1 real comment and 5 relays reporting it lands `eventCount = 5`.
The handler doesn't even destructure `event` from the callback —
it literally cannot dedupe as written.
`closeOnEose: true` keeps the subscription short-lived, so this only
fires during initial page load — but that's every recipe view, and
every reload re-inflates.
For contrast: the sibling `createCommentSubscription` in
`$lib/comments/subscription.ts` uses instance-local
`processedIds = new Set<string>()` plus `.has(ev.id)` in its
on('event') handler. The pattern was already in the codebase,
just not here.
Fix
---
Instance-local `processedIds: Set<string>` declared inside the
`onMount` closure (not module-scoped — keeps scope tied to the
subscription's lifetime). Guard with `.has(ev.id)` before the
`eventCount++`. The `totalCommentAmount = eventCount` reconciliation
logic below is untouched.
Scope
-----
- Only kind-30023 recipes render TotalComments.svelte; feed-note
comment counts use NoteTotalComments (engagementCache) which has
its own dedup.
- Filter (`createCommentFilter(event)`) is unchanged.
- Subscription lifecycle is unchanged — still `closeOnEose: true`,
still stopped in `onDestroy`.
- No change to published event shape.
Verified
--------
- pnpm exec eslint: pre-existing unused `userPublickey` import (line
2) + `any` on `subscription` type (line 12) remain. No new errors.
- Dedup path mirrors $lib/comments/subscription.ts exactly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deploying frontend with
|
| Latest commit: |
297533b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ccb6b480.frontend-hvd.pages.dev |
| Branch Preview URL: | https://fix-reaction-comment-count-d.frontend-hvd.pages.dev |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
frontend | 297533b | Apr 21 2026, 04:48 PM |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes read-side double-counting/inflation for reactions and recipe comment totals across shared UI components (feed + recipe contexts), without changing the bytes of published kind-7 / kind-1111 events.
Changes:
- Prevents self-echo double-increments by pre-registering optimistic/dedup sentinels before publishing reactions (ReactionTrigger, CommentCard).
- Fixes recipe comment-count inflation by deduping subscription events by
event.id(TotalComments). - Prevents subscription leaks in an (currently unused) reaction component by consolidating subscription lifecycle to mount + id-change handling (ReactionButton).
- Bumps package version.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/components/comments/CommentCard.svelte | Signs and pre-registers the like event id in processedLikes before publish to prevent +2 like count via subscription echo. |
| src/components/Recipe/TotalComments.svelte | Dedupes comment-count subscription events by NDKEvent.id to avoid multi-relay fan-in inflation. |
| src/components/Reactions/ReactionTrigger.svelte | Registers optimistic reaction dedup before any await; marks processed ids on success and clears optimistic tracking on failure. |
| src/components/Reactions/ReactionButton.svelte | Removes reactive subscription creation; re-subscribes only when event.id changes post-mount and ensures cleanup on destroy. |
| package.json | Version bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+191
to
198
| beforeUpdate(() => { | ||
| if (!mounted) return; | ||
| const newId = event?.id ?? null; | ||
| if (newId && newId !== lastEventId) { | ||
| subscription?.stop(); | ||
| lastEventId = newId; | ||
| loadReactions(); | ||
| } |
spe1020
pushed a commit
that referenced
this pull request
Apr 21, 2026
Copilot review on PR #321 flagged that loadReactions() has a race: `await fetchCount(...)` can resolve after the `event` prop has changed, letting an older call continue past the await and assign `subscription = $ndk.subscribe(...)` for the stale id. The newer loadReactions (triggered by the beforeUpdate hook landed in the previous commit) would overwrite `subscription`, leaving the older sub running and unreferenced. Fix --- Add a monotonically-increasing `currentLoadToken`. Each call to loadReactions captures the current value into `myToken` via `++currentLoadToken`, which claims the latest slot. After every await, and before any state mutation or subscription, we check `myToken !== currentLoadToken` — if a newer call has started, we bail. The newly-created NDK subscription is also checked synchronously after `$ndk.subscribe(...)` so a subscription created against a stale id gets `.stop()`ed immediately rather than overwriting the variable or leaking. Event handlers on the successfully-assigned subscription also check the token, so once a sub is superseded it stops mutating state even if the relay keeps delivering events for the lifetime of the old filter. Scope ----- Still defensive — `ReactionButton.svelte` remains unreferenced in the tree (zero `<ReactionButton` usages). The file now handles both the mount/reactive lifecycle (previous commit) and the overlapping-loadReactions race (this commit) correctly. If someone rewires it later, it behaves. Verified -------- - pnpm exec eslint: pre-existing `fastCountLoaded` unused-var error remains (untouched by this patch). No new errors. - pnpm check: baseline 4 errors preserved. - Subscription construction unchanged — no change to published event reads or writes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot review on PR #321 flagged that loadReactions() has a race: `await fetchCount(...)` can resolve after the `event` prop has changed, letting an older call continue past the await and assign `subscription = $ndk.subscribe(...)` for the stale id. The newer loadReactions (triggered by the beforeUpdate hook landed in the previous commit) would overwrite `subscription`, leaving the older sub running and unreferenced. Fix --- Add a monotonically-increasing `currentLoadToken`. Each call to loadReactions captures the current value into `myToken` via `++currentLoadToken`, which claims the latest slot. After every await, and before any state mutation or subscription, we check `myToken !== currentLoadToken` — if a newer call has started, we bail. The newly-created NDK subscription is also checked synchronously after `$ndk.subscribe(...)` so a subscription created against a stale id gets `.stop()`ed immediately rather than overwriting the variable or leaking. Event handlers on the successfully-assigned subscription also check the token, so once a sub is superseded it stops mutating state even if the relay keeps delivering events for the lifetime of the old filter. Scope ----- Still defensive — `ReactionButton.svelte` remains unreferenced in the tree (zero `<ReactionButton` usages). The file now handles both the mount/reactive lifecycle (previous commit) and the overlapping-loadReactions race (this commit) correctly. If someone rewires it later, it behaves. Verified -------- - pnpm exec eslint: pre-existing `fastCountLoaded` unused-var error remains (untouched by this patch). No new errors. - pnpm check: baseline 4 errors preserved. - Subscription construction unchanged — no change to published event reads or writes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
spe1020
pushed a commit
that referenced
this pull request
Apr 21, 2026
Completes the reaction/comment count dedup work started in #321. Feed comment counts on kind-1 posts were inflating (a newly posted comment showed up as 6 in the console while Primal / other clients showed 1) because `processedEventIds` was being wiped on every entry into `fetchEngagement` and `batchFetchEngagement`, breaking dedup across concurrent subscription lifecycles. Fingerprint (F5-B, from the instrumentation on fix/feed-comment-count-dedup) [EC-COMMENT-SINGLE] counted 5605e182 for root 00006922 new-count: 2 [EC-COMMENT-SINGLE] counted 5605e182 for root 00006922 new-count: 3 [EC-COMMENT-SINGLE] counted 5605e182 for root 00006922 new-count: 4 [EC-COMMENT-SINGLE] counted 5605e182 for root 00006922 new-count: 5 [EC-COMMENT] counted 5605e182 for root 00006922 new-count: 6 [EC-COMMENT] deduped 5605e182 for root 00006922 One comment event counted six times — five times through the single- event subscription, once through the batch subscription, before the batch sub finally deduped. Root cause A feed card mounts several engagement-consuming components for the same root event: NoteTotalComments, NoteTotalLikes (via ReactionTrigger), NoteTotalZaps, NoteRepost, ReactionPills. Each calls `fetchEngagement(ndk, eventId, userPublickey)` on mount. Each call was executing: processedEventIds.set(eventId, new Set()); processedReactionPairs.set(eventId, new Set()); …replacing any existing Set mid-stream. And the sub-refresh branch (when `existingPersistent && !hasAmountData`) was also doing: processedEventIds.delete(eventId); processedReactionPairs.delete(eventId); But the subscription handler captures `const processed = processedEventIds.get(eventId)!` at creation time — so each stopped- but-still-delivering sub held a closure reference to its old (now orphaned) Set. New fetchEngagement entrants installed fresh Sets. When relays redelivered the same event to multiple in-flight handlers, each handler's independent Set reported `has(id) === false`, and the handlers independently incremented the shared store's `comments.count`. Fix Stop resetting the dedup Sets on entry. Initialize-if-absent instead. The Sets now persist for the entire lifetime of an event being visible on screen; `cleanupEngagement(eventId)` (fires when the event leaves the viewport) is the only place they're cleared. Applied at four sites in engagementCache.ts: 1. fetchEngagement sub-refresh branch — removed the explicit `.delete()` calls for both processedEventIds and processedReactionPairs. Stopping the subscription is kept; the Set is not touched. 2. fetchEngagement init block — `processedEventIds.set(...)` and `processedReactionPairs.set(...)` replaced with `if (!map.has(eventId)) map.set(eventId, new Set())`. 3-4. batchFetchEngagement init block — same init-if-absent pattern for both Maps, per toFetch id. All handlers already call `processed.has(event.id)` before incrementing, so the only change needed was preserving the Set's identity across calls; no handler logic changes. Why count resets are left intact The synchronous count reset (`reactions.count: 0`, `comments.count: 0`, etc.) still runs. On first-time init the Set is empty, so the subscription correctly counts events from zero. On a re-entry where the Set already has prior ids, events the relay redelivers are deduped (not counted), and the authoritative count is refreshed by the NIP-45 `getEngagementCounts` API fast path that runs earlier in `fetchEngagement`. Transient oscillation (0 → API count → 0 → API count) may be visible for 100-300ms during a concurrent-mount burst, but the final steady-state count is correct. Verified - pnpm exec eslint: only pre-existing `pendingOptimisticZappers` prefer-const on line 355 (untouched). No new errors. - pnpm check: 4 errors baseline preserved. - Instrumentation on fix/feed-comment-count-dedup captured the pre-fix F5-B fingerprint. Instrumentation revert follows in the next commit so logs don't ship. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
spe1020
added a commit
that referenced
this pull request
Apr 21, 2026
…scriptions (#323) * chore(instrument): TEMP feed-comment double-count fingerprint logs NOT A FIX. Temporary diagnostic logs to fingerprint the feed-post comment-count doubling that Phase 1 misread as correctly deduped. Revert this commit once the fingerprint is captured and the real fix lands. Tags used: [EC-COMMENT] batch sub kind-1 branch (batchFetchEngagement) [EC-COMMENT-SINGLE] single-event sub kind-1 branch (fetchEngagement) [NTC-RENDER] NoteTotalComments render tick (one per mounted instance) Each EC log carries event.id.slice(0,8) + root.slice(0,8). NTC log carries the same event slice + the rendered count. Fingerprint decoder (expected after posting a single comment on a kind-1 feed note that had zero prior comments): F5-A Two identical `[EC-COMMENT] counted` OR two identical `[EC-COMMENT-SINGLE] counted` lines for the same event id → processed Set is being reset mid-stream, or the subscription is being recreated and the new sub inherits a cleared Set. Fix: scope audit. F5-B One `[EC-COMMENT] counted` + one `[EC-COMMENT-SINGLE] counted` for the same event id → both batch subscription and single-event subscription are alive for the same root and are incrementing two counter paths. They do share processedEventIds.get(eventId), so this would mean one of them is resetting that Set before the other runs. Fix: coordinate reset between the two entry points (batchFetchEngagement + fetchEngagement). F5-C One `[EC-COMMENT*] counted` + multiple `[NTC-RENDER]` lines for the same event id → two NoteTotalComments instances mounted for the same root. Each reads independently, but both see the true count. If both render side-by-side the user sees them summed. Fix: DOM audit — likely a feed card + a detail view both visible. F5-D No `[EC-COMMENT*]` lines fire at all but count still climbs → a third write-path outside engagementCache.ts. Unlikely per the exhaustive grep at 2f.1 — all .comments.count writes are in engagementCache.ts, engagementPreloader.ts (assignment from counts API, not ++), or display-only (shareNoteImage.ts, FoodstrFeedOptimized engagementData snapshots). If this fires, grep harder. F5-E Two `[EC-COMMENT*] counted` lines with event ids the user didn't just post → filter matching something unexpected (reaction, reply-to- reply, etc.). Fix: tighten filter. Instrumentation sites — all lines are marked with `TEMP-INSTRUMENT fingerprint-5` to make revert trivial: engagementCache.ts (single-event sub, around line 420) - deduped log in top-level `processed.has` branch, kind-1 only - counted log inside kind-1 case, before count++ engagementCache.ts (batch sub, around line 920) - mirror of the above NoteTotalComments.svelte - reactive `$: console.log(...)` at render time Seth: reproduce by posting ONE comment on a kind-1 feed post with zero prior comments, reload the page, paste the console lines tagged [EC-COMMENT, [EC-COMMENT-SINGLE, and [NTC-RENDER from the last ~15 seconds. The fingerprint will dictate the Phase 2g fix shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engagementCache): dedupe across concurrent engagement subscriptions Completes the reaction/comment count dedup work started in #321. Feed comment counts on kind-1 posts were inflating (a newly posted comment showed up as 6 in the console while Primal / other clients showed 1) because `processedEventIds` was being wiped on every entry into `fetchEngagement` and `batchFetchEngagement`, breaking dedup across concurrent subscription lifecycles. Fingerprint (F5-B, from the instrumentation on fix/feed-comment-count-dedup) [EC-COMMENT-SINGLE] counted 5605e182 for root 00006922 new-count: 2 [EC-COMMENT-SINGLE] counted 5605e182 for root 00006922 new-count: 3 [EC-COMMENT-SINGLE] counted 5605e182 for root 00006922 new-count: 4 [EC-COMMENT-SINGLE] counted 5605e182 for root 00006922 new-count: 5 [EC-COMMENT] counted 5605e182 for root 00006922 new-count: 6 [EC-COMMENT] deduped 5605e182 for root 00006922 One comment event counted six times — five times through the single- event subscription, once through the batch subscription, before the batch sub finally deduped. Root cause A feed card mounts several engagement-consuming components for the same root event: NoteTotalComments, NoteTotalLikes (via ReactionTrigger), NoteTotalZaps, NoteRepost, ReactionPills. Each calls `fetchEngagement(ndk, eventId, userPublickey)` on mount. Each call was executing: processedEventIds.set(eventId, new Set()); processedReactionPairs.set(eventId, new Set()); …replacing any existing Set mid-stream. And the sub-refresh branch (when `existingPersistent && !hasAmountData`) was also doing: processedEventIds.delete(eventId); processedReactionPairs.delete(eventId); But the subscription handler captures `const processed = processedEventIds.get(eventId)!` at creation time — so each stopped- but-still-delivering sub held a closure reference to its old (now orphaned) Set. New fetchEngagement entrants installed fresh Sets. When relays redelivered the same event to multiple in-flight handlers, each handler's independent Set reported `has(id) === false`, and the handlers independently incremented the shared store's `comments.count`. Fix Stop resetting the dedup Sets on entry. Initialize-if-absent instead. The Sets now persist for the entire lifetime of an event being visible on screen; `cleanupEngagement(eventId)` (fires when the event leaves the viewport) is the only place they're cleared. Applied at four sites in engagementCache.ts: 1. fetchEngagement sub-refresh branch — removed the explicit `.delete()` calls for both processedEventIds and processedReactionPairs. Stopping the subscription is kept; the Set is not touched. 2. fetchEngagement init block — `processedEventIds.set(...)` and `processedReactionPairs.set(...)` replaced with `if (!map.has(eventId)) map.set(eventId, new Set())`. 3-4. batchFetchEngagement init block — same init-if-absent pattern for both Maps, per toFetch id. All handlers already call `processed.has(event.id)` before incrementing, so the only change needed was preserving the Set's identity across calls; no handler logic changes. Why count resets are left intact The synchronous count reset (`reactions.count: 0`, `comments.count: 0`, etc.) still runs. On first-time init the Set is empty, so the subscription correctly counts events from zero. On a re-entry where the Set already has prior ids, events the relay redelivers are deduped (not counted), and the authoritative count is refreshed by the NIP-45 `getEngagementCounts` API fast path that runs earlier in `fetchEngagement`. Transient oscillation (0 → API count → 0 → API count) may be visible for 100-300ms during a concurrent-mount burst, but the final steady-state count is correct. Verified - pnpm exec eslint: only pre-existing `pendingOptimisticZappers` prefer-const on line 355 (untouched). No new errors. - pnpm check: 4 errors baseline preserved. - Instrumentation on fix/feed-comment-count-dedup captured the pre-fix F5-B fingerprint. Instrumentation revert follows in the next commit so logs don't ship. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Revert "chore(instrument): TEMP feed-comment double-count fingerprint logs" This reverts commit eb8042f. * fix(engagementCache): gate count resets on fresh-Set init Addresses the Copilot review on PR #323: with the dedup Sets now persisting across fetchEngagement / batchFetchEngagement re-entries (the bb89d79 fix), the pre-existing unconditional `counts = 0` reset became a new failure mode. On re-entry to either function with an already-populated Set, the flow was: 1. Sync reset reactions/comments/reposts/zaps counts to 0. 2. subscriptionCountingInProgress.add(eventId), which causes the NIP-45 `getEngagementCounts()` fast-path to skip its store.update (line 258-261: "Skip if subscription is actively counting"). 3. Subscription starts against the preserved Set. 4. Relay redelivers historical events — every one returns `processed.has(event.id) === true` → dedup → handler early-exits without incrementing. 5. Nothing ever increments the zeroed counts. Stuck at 0. Fix: gate the count reset on `isFirstInit` (i.e. we created the Set this call). On re-entry, keep the counts already produced against this Set in place and just flag `loading: true` so the UI reflects the refresh. Applied at both sites: - fetchEngagement: `isFirstInit = !processedEventIds.has(eventId)` captured before the .set() call. Only the first-init branch runs the full reset+optimistic-zap-restore store.update; re-entry does a minimal `loading: true` update while topping up optimistic zap state (so a pending zap isn't erased mid-flight). - batchFetchEngagement: same pattern per `toFetch` id. The count state's invariant is now: the dedup Set and the counts are co-owned — they come into existence together and go out of existence together (via cleanupEngagement). A subscription only mutates the counts by calling `processed.add(event.id)` and incrementing, never by wiping. Verified - pnpm exec eslint src/lib/engagementCache.ts: clean (the prior pendingOptimisticZappers prefer-const warning disappeared when the variable was rewritten as const during this refactor). - pnpm check: 4 errors baseline preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: spe1020 <sethsager@Seths-MacBook-Air.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
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 free
to 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.
Summary
Three reaction-count bugs + one recipe-comment-count bug, each in a shared component so a single fix covers both feed and recipe contexts. Published kind-7 / kind-1111 event bytes are unchanged — these are all read-side dedup fixes. Other clients already render the correct counts from our events.
Not in this PR: feed comment-count doubling. Phase 1's code-read said the feed path dedupes correctly via
engagementCache.processedEventIds, but empirical observation contradicts that. A separate branch (fix/feed-comment-count-dedup) carries temporary fingerprint-5 instrumentation to let us capture the actual behaviour; the fix will ship as a follow-up PR once the fingerprint lands.What each commit does
2237aaf—fix(ReactionTrigger): register optimistic dedup before publishBug: Main like button (heart icon on both feed and recipe pages) incremented the local count by +2 on click. Other clients saw +1.
Root cause:
ReactionTrigger.handleReaction()did the optimisticstore.update(count++)thenawait publishReaction(...), but never calledtrackOptimisticReaction(...)ormarkEventAsProcessed(...)onengagementCache. The relay echo of the user's own reaction came back through the shared persistent subscription, enteredprocessReaction, found nooptimisticReactionsentry (never registered), passed theprocessedReactionPairsfirst-time gate, and randata.reactions.count++a second time.Fix: Matches the pattern already used by
ReactionPills.svelte:trackOptimisticReaction(event.id, emoji, userPublickey)synchronously, before anyawait.store.update(count++)(unchanged).await publishReaction(...).markEventAsProcessed(event.id, result.id)as secondary protection against emoji-normalization drift.clearOptimisticReaction(...)+ revert the optimistic store update.Scope:
ReactionTriggeris rendered by bothNoteTotalLikes(feed) andRecipe/TotalLikes(recipe) — single change, both contexts fixed.5498cb2—fix(CommentCard): add processedLikes before publishBug: Per-comment like button (inside any
CommentThread) incrementedlikeCountby +2 on click. Classic Fingerprint A.Root cause:
toggleLike()published then incremented without registering the event id inprocessedLikesbeforehand. The subscription'son('event')handler on line 205-211 saw the echo first, passedprocessedLikes.has(e.id)(false — never added), and ran its ownlikeCount++; then the post-publishlikeCount++ran on top.Fix: Sign first to obtain
.id,processedLikes.add(reactionEvent.id)synchronously before publish, optimisticlikeCount++andliked = true, then publish. Catch reverts all three (includingprocessedLikes.delete(id)so retries re-register cleanly).Scope:
CommentCardrenders in both recipe and feedCommentThreadvariants — single change, both contexts fixed.de51ddd—fix(ReactionButton): consolidate subscription lifecycle to onMountBug:
ReactionButton.sveltehad bothonMount(loadReactions)and a reactive$: if (event?.id) loadReactions()firing on first mount, leaking a subscription per mount.Root cause: Reactive
$:blocks fire whenever any reactive dependency ticks, not only on the watched identifier change. Each fire created a new$ndk.subscribe(...), reassigned thesubscriptionvariable, and left the previous one open.onDestroyonly stopped the latest reference.Fix: Remove the reactive
$:. KeeponMountas the single mount-time subscribe; usebeforeUpdate+mountedguard +lastEventIdtracking to re-subscribe only when theevent.idprop actually changes post-mount.Defensive scope: Note in the commit —
ReactionButton.svelteis currently unreferenced (zero<ReactionButtonusages in the tree). The fix costs nothing and keeps the file correct if anyone rewires it; if the intention is to delete the file outright, this patch doesn't block that decision.8c64d90—fix(Recipe/TotalComments): dedup by event id to stop multi-relay inflationBug: Recipe comment count inflated to 2–5× the true value on page load. Other clients showed the correct count.
Root cause:
TotalComments.svelteran a standalone subscription (separate fromcreateCommentSubscription) and counted witheventCount++peron('event')callback. No dedup. NDK's subscription pool delivers the same event once per relay that holds it, so one real kind-1111 NIP-22 comment with 5 relays reporting it landedeventCount = 5. The handler didn't even destructureeventfrom the callback — literally could not dedupe as written.Fix: Instance-local
processedIds = new Set<string>()inside theonMountclosure (tied to subscription lifetime, not module-scoped). Guard with.has(ev.id)beforeeventCount++. Mirrors the pattern already in$lib/comments/subscription.ts;TotalCommentswas the outlier.Scope: Only kind-30023 recipe pages render
TotalComments. Feed-note comment counts flow throughNoteTotalComments→engagementCachewhich handles dedup differently (and has a separate doubling bug — see below).Verification
For each fix, ran
pnpm exec eslinton the touched file immediately after editing. All errors surfaced in each file (mostly pre-existinganytypes in helper functions) were present onmainalready — zero new errors introduced by this PR.pnpm check→ 4 errors (baseline preserved; same pre-existing failures inkitchens.ts,nourishDiscovery.ts,FoodstrFeedOptimized.svelte).Event shape unchanged: for each fix, the code constructing
reactionEvent.tags/reactionEvent.contentis identical tomain. Confirmed viagit diff main -- <file>inspection during each commit.Test plan
Per-bug, in order:
ReactionTrigger (
2237aaf)Recipe/TotalLikes) → same single-increment.publishReactionto fail (e.g. offline) → UI reverts the count,userReactions.delete(emoji)runs, retry works cleanly.CommentCard (
5498cb2)reactionEvent.publish()to fail →likeCount--,liked = false, next click re-registers and works.ReactionButton (
de51ddd)pnpm exec eslint src/components/Reactions/ReactionButton.svelte(pre-existingfastCountLoadedunused-var is the only error; no new ones).Recipe/TotalComments (
8c64d90)Cross-bug
pnpm run checkandpnpm run buildboth pass.Known follow-up (out of scope for this PR)
fix/feed-comment-count-dedup(commit0a35d70). Once the console logs are captured and the fingerprint matched (F5-A through F5-E — see that commit's message), a targeted fix lands as a follow-up PR, and the instrumentation commit gets reverted as the last step of that PR.ReactionButton.sveltedead code — either delete outright or wait until a future feature rewires it; either decision is safe after this PR.🤖 Generated with Claude Code