fix(feed-comment-count): dedupe across concurrent engagementCache subscriptions (#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>

Author: 3 people

package.json

@@ -1,7 +1,7 @@
 {
   "name": "zap.cooking",
   "license": "MIT",
-  "version": "4.2.266",
+  "version": "4.2.267",
   "private": true,
   "scripts": {
     "dev": "vite dev",

src/lib/engagementCache.ts

@@ -311,34 +311,61 @@ export async function fetchEngagement(
     return;
   }
 
-  // If we have a subscription but no amount data, close it and create a fresh one
+  // If we have a subscription but no amount data, close it and create a fresh one.
+  // Note: we intentionally do NOT wipe processedEventIds / processedReactionPairs
+  // here — those dedup Sets must persist across sub close+reopen so any
+  // events the prior sub already counted aren't re-counted by the new sub
+  // when the relay redelivers them (F5-B feed-comment double-count bug).
+  // The Sets are only evicted by `cleanupEngagement(eventId)` when the
+  // event goes off-screen.
   if (existingPersistent && !hasAmountData) {
     console.debug('[Engagement] Subscription exists but no amount data, refreshing for', eventId);
     existingPersistent.sub.stop();
     persistentSubscriptions.delete(eventId);
-    processedEventIds.delete(eventId); // Allow re-processing events
-    processedReactionPairs.delete(eventId);
   }
-  
-  // Initialize processed event tracking - MUST be fresh to avoid double counting
-  // Clear any stale processed IDs and start fresh
-  processedEventIds.set(eventId, new Set());
-  processedReactionPairs.set(eventId, new Set());
+
+  // Initialize processed event tracking — init-if-absent, never wipe.
+  // Multiple components (NoteTotalComments, NoteTotalLikes/ReactionTrigger,
+  // NoteTotalZaps, NoteRepost, ReactionPills) all call fetchEngagement for
+  // the same eventId on mount. Wiping the Set on every call caused each
+  // component's resulting subscription to treat already-counted events as
+  // new — producing the F5-B fingerprint of one event yielding 5+ counts.
+  // Sharing a single Set across all callers is the correct pattern for
+  // cross-sub dedup.
+  //
+  // The Set lifetime is bound to the count state that was accumulated
+  // against it: if we created the Set this call, counts are still at
+  // zero (or cached / API-fast-path values) and the subscription should
+  // count up from there. If the Set already existed from a prior call,
+  // the count state it produced is already in `store` — wiping counts
+  // while preserving the Set would cause the sub to dedup historical
+  // events (they're in the Set) without ever re-counting them, leaving
+  // counts stuck at 0.
+  const isFirstInit = !processedEventIds.has(eventId);
+  if (isFirstInit) {
+    processedEventIds.set(eventId, new Set());
+  }
+  if (!processedReactionPairs.has(eventId)) {
+    processedReactionPairs.set(eventId, new Set());
+  }
   const processed = processedEventIds.get(eventId)!;
-  
+
   // Stop any old-style subscriptions
   const existingSubs = activeSubscriptions.get(eventId);
   if (existingSubs) {
     existingSubs.forEach(sub => sub.stop());
     activeSubscriptions.delete(eventId);
   }
-  
-  // IMPORTANT: Reset counts to 0 before subscription to prevent double counting
-  // The cache shows instant data, but subscription will provide accurate final counts
-  // Preserve any pending optimistic zap updates so they aren't wiped during re-fetch
+
+  // Count-reset is only safe when starting fresh. On re-entry (Set
+  // already populated), leave the accumulated counts in place — the
+  // subscription will dedup historical events and only increment for
+  // genuinely new arrivals, which is the correct behavior.
+  // Pending optimistic zap updates are re-applied on both paths so
+  // they aren't wiped during re-fetch.
   let pendingOptimisticAmount = 0;
   let pendingOptimisticCount = 0;
-  let pendingOptimisticZappers: Array<{ pubkey: string; amount: number; timestamp: number }> = [];
+  const pendingOptimisticZappers: Array<{ pubkey: string; amount: number; timestamp: number }> = [];
   for (const [key, zap] of optimisticZaps.entries()) {
     if (key.startsWith(`${eventId}:`)) {
       // totalAmount is stored in millisats; topZappers.amount is sats
@@ -357,20 +384,37 @@ export async function fetchEngagement(
       }
     }
   }
-  store.update(s => ({
-    ...s,
-    reactions: { count: 0, userReacted: false, groups: [], userReactions: new Set() },
-    comments: { count: 0 },
-    reposts: { count: 0, userReposted: false },
-    zaps: {
-      ...s.zaps,
-      count: pendingOptimisticCount,
-      totalAmount: pendingOptimisticAmount,
-      topZappers: pendingOptimisticZappers,
-      userZapped: s.zaps.userZapped || pendingOptimisticCount > 0
-    },
-    loading: true
-  }));
+  if (isFirstInit) {
+    store.update(s => ({
+      ...s,
+      reactions: { count: 0, userReacted: false, groups: [], userReactions: new Set() },
+      comments: { count: 0 },
+      reposts: { count: 0, userReposted: false },
+      zaps: {
+        ...s.zaps,
+        count: pendingOptimisticCount,
+        totalAmount: pendingOptimisticAmount,
+        topZappers: pendingOptimisticZappers,
+        userZapped: s.zaps.userZapped || pendingOptimisticCount > 0
+      },
+      loading: true
+    }));
+  } else {
+    // Re-entry: preserve counts already produced against this Set;
+    // just mark loading so UI knows a refresh is in flight, and top
+    // up the optimistic-zap state.
+    store.update(s => ({
+      ...s,
+      zaps: {
+        ...s.zaps,
+        count: Math.max(s.zaps.count, pendingOptimisticCount),
+        totalAmount: Math.max(s.zaps.totalAmount, pendingOptimisticAmount),
+        topZappers: pendingOptimisticZappers.length > 0 ? pendingOptimisticZappers : s.zaps.topZappers,
+        userZapped: s.zaps.userZapped || pendingOptimisticCount > 0
+      },
+      loading: true
+    }));
+  }
 
   // Mark that subscription counting is in progress (prevents NIP-45 race condition)
   subscriptionCountingInProgress.add(eventId);
@@ -881,25 +925,48 @@ export async function batchFetchEngagement(
   }
 
   // FULL PATH: NDK subscription for accurate counts + user state
-  // Reset processed IDs and counts to prevent double counting
+  // Init-if-absent on the dedup Sets — never wipe them here. The batch
+  // subscription shares `processedEventIds` with any per-event subscription
+  // that fetchEngagement may have already opened for the same eventId.
+  // Wiping the Set at batch-entry caused the in-flight single subs and
+  // the new batch sub to each count the same event against a fresh Set
+  // (F5-B fingerprint). Eviction happens only in cleanupEngagement when
+  // the event leaves the viewport.
   toFetch.forEach(id => {
-    // Clear processed IDs to start fresh
-    processedEventIds.set(id, new Set());
-    processedReactionPairs.set(id, new Set());
-    
+    // Only reset counts to 0 when we're creating the dedup Set for the
+    // first time for this id. If the Set is already populated (e.g. a
+    // per-event `fetchEngagement` ran for this id moments ago), the
+    // accumulated counts in `store` were produced against that Set —
+    // wiping counts while preserving the Set would cause this batch sub
+    // to dedup every historical event without ever re-counting them,
+    // leaving counts stuck at 0.
+    const isFirstInit = !processedEventIds.has(id);
+    if (isFirstInit) {
+      processedEventIds.set(id, new Set());
+    }
+    if (!processedReactionPairs.has(id)) {
+      processedReactionPairs.set(id, new Set());
+    }
+
     // Mark that subscription counting is in progress
     subscriptionCountingInProgress.add(id);
-    
-    // Reset counts to 0 before fetching to prevent accumulation
-    const store = getEngagementStore(id);
-    store.update(s => ({
-      ...s,
-      reactions: { count: 0, userReacted: false, groups: [], userReactions: new Set() },
-      comments: { count: 0 },
-      reposts: { count: 0, userReposted: false },
-      zaps: { ...s.zaps, count: 0, totalAmount: 0, topZappers: [] },
-      loading: true
-    }));
+
+    if (isFirstInit) {
+      const store = getEngagementStore(id);
+      store.update(s => ({
+        ...s,
+        reactions: { count: 0, userReacted: false, groups: [], userReactions: new Set() },
+        comments: { count: 0 },
+        reposts: { count: 0, userReposted: false },
+        zaps: { ...s.zaps, count: 0, totalAmount: 0, topZappers: [] },
+        loading: true
+      }));
+    } else {
+      // Re-entry: preserve counts; just flag loading so UI reflects
+      // the refresh.
+      const store = getEngagementStore(id);
+      store.update(s => ({ ...s, loading: true }));
+    }
   });
 
   const filter = {