perf(reactions): key ReactionPills each by group.emoji#342
Merged
spe1020 merged 1 commit intozapcooking:mainfrom Apr 25, 2026
Merged
perf(reactions): key ReactionPills each by group.emoji#342spe1020 merged 1 commit intozapcooking:mainfrom
spe1020 merged 1 commit intozapcooking:mainfrom
Conversation
R4 from docs/perf/2026-04-review.md. `visibleGroups` at ReactionPills.svelte:93 is `$store.reactions.groups .slice(0, maxVisible)` — `slice` returns a new array reference on every reactive tick, so without an explicit each-key Svelte falls back to identity-based keying and remounts every pill button when the count updates. The buttons are visually identical between updates; the remount is pure render churn. Keying on `group.emoji` (the stable identity within the array) keeps each button instance across count changes. No behavior change. Counts, ordering, optimistic-update flow, and the dedup architecture from PRs zapcooking#321/zapcooking#323 are untouched. R1 (the feed-tab remount fix) was deferred — closer reading of FoodstrFeedOptimized showed that filterMode has no reactive watcher in the file at all (`lastFilterMode` at :487 is declared but unused), so replacing `{#key feedKey}` requires extracting setup/teardown helpers from onMount/onDestroy and wiring a `$:` watcher to them. That's a larger refactor than the report estimated; tracking separately. R2 (wire batchFetchEngagement) was withdrawn — the function IS already called from FoodstrFeedOptimized at :4717 (preload, batches of 20 with lastBatchedIds dedup) and :4753 (visibility-driven, 100ms-debounced). The report's "imported but never called" claim was based on a partial grep. Pushing batching further needs Stage 1 measurement first. svelte-check: 0 errors / 127 warnings (warnings all pre-existing). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
6 tasks
spe1020
added a commit
that referenced
this pull request
Apr 26, 2026
* perf(feed): batch engagement for anon viewers too (R2) Closes the last open recommendation from the perf review docs in PR #341 (R1 / R4 already shipped via #344 and #342). The batch helper itself is wired in at two sites — the rendered-events preload and the visibility-update reactive — but both gate on `$userPublickey`, which means signed-in users get batched fetches while anonymous viewers fall back to the full per-card fan-out: NoteTotalLikes → fetchEngagement(eventId) NoteTotalComments → fetchEngagement(eventId) NoteRepost → fetchEngagement(eventId) NoteTotalZaps → fetchEngagement(eventId) ReactionPills → fetchEngagement(eventId) That's up to 5× the active subscription churn per card, multiplied across a 30-event feed window, on every cold paint and every newly- rendered card. The gate isn't doing anything useful: `batchFetchEngagement`'s server-counts API doesn't require auth, and the NDK fallback subscription only uses `userPublickey` to flag userReacted / userReposted — which already correctly stay false for anonymous users either way. The per-card children themselves don't gate on sign-in; only the batch did. Drop the gate on both reactives so anonymous viewers get the same batched preload + visibility refresh signed-in users already benefit from. The 5-minute freshness gate inside batchFetchEngagement preempts the subsequent per-card fetches just like it does for the signed-in path. No behavior change for signed-in viewers (gate was always passing for them). Anonymous viewers see fewer concurrent NDK subs during feed scroll. Counts and groups continue to populate identically; no impact to the count-correctness work in #321/#323. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: spe1020 <sethsager@Seths-MacBook-Air.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
R4 from
docs/perf/2026-04-review.md. Adds an explicit each-block key toReactionPills. One-line change, no behavior change, intentionally tiny scope.visibleGroups(ReactionPills.svelte:93) is computed as$store.reactions.groups.slice(0, maxVisible)—slicereturns a fresh array reference every reactive tick, so without an explicit key Svelte falls back to identity-based keying and remounts every pill button when the count updates. The buttons are visually identical between updates; the remount is pure render churn.Keying on
group.emojikeeps each button instance across count changes.Why this PR is just R4
The original plan was R1 + R2 + R4 in one PR. Closer reading of the actual code changed that:
FoodstrFeedOptimizeddeclareslastFilterModeat:487but never reads it — there's no reactive watcher onfilterModeanywhere in the 5000-line component. The{#key feedKey}remount is the only change-detection mechanism. Replacing it properly means extractingsetupFeed()/teardownFeed()helpers fromonMount/onDestroyand wiring a$:watcher. That's a real refactor on a critical UX surface; deserves its own PR with focused review and shouldn't be bundled with anything else. Tracking separately.batchFetchEngagement) turned out to already be wired. The function is called atFoodstrFeedOptimized.svelte:4717(preload, 20-event batches withlastBatchedIdsdedup) and:4753(visibility-driven, 100ms-debounced). My report's "imported but never called" claim was a partial-grep miss — apologies for the noise. Pushing batching further needs Stage 1 measurement (also tracked in the report) to know what's actually fanning out.So this PR ships only the literally-one-keyword change. Risk surface against PRs #321/#323's count-correctness work is zero — counts, ordering, optimistic-update flow, and the dedup architecture in
engagementCache.tsare untouched.Test plan
pnpm svelte-checkreports 0 errors (already verified locally).🤖 Generated with Claude Code