fix: likes follow-ups #256, #257, #260

[podclaude]

Summary

Fixes three follow-up issues from the likes PR #255:

• [Follow-up #255] Replace String.to_existing_atom with explicit whitelist in LikeNormalizer #256: Replace String.to_existing_atom with an explicit whitelist in LikeNormalizer — unknown keys are dropped silently instead of crashing on checkpoint replay if a field is ever renamed
• [Follow-up #255] Prune old unliked entries from Like aggregate state #257: Add 45-day pruning of old unliked entries in SnapshotLike, matching the subscription aggregate pattern — entries with unliked_at older than 45 days are pruned from snapshots
• [Follow-up #255] Add private user tests for PopularityProjector likes #260: Add is_public option to popularity test helpers so private users can be tested — increments likes count without appearing in likes_people

Also closes #258 (documented as inherent snapshot pattern limitation, TODO in code).

Test Plan

• New test: unknown string keys dropped in LikeCheckpoint normalization
• New test: 45-day pruning of old unliked entries in SnapshotLike
• New test: private user podcast like increments count but not in likes_people
• New test: private user episode like increments count but not in likes_people
• All 589 tests pass, 0 failures

Closes #256, closes #257, closes #260

[claude]

Code Review — PR #261 (likes follow-ups #256, #257, #260)

Overall: APPROVED — the three issues are well addressed, code follows existing patterns, and tests are solid. A few minor observations below.

---

#256 — LikeNormalizer whitelist

LGTM. Replacing String.to_existing_atom with an explicit whitelist is the right approach. The flat_map + Map.new() pattern is clean and readable.

One small note: the _ catch-all arm silently drops key-value pairs with non-binary, non-atom keys. This is the correct defensive behaviour for JSON-sourced data, and the comment in the module doc covers it. No action needed.

---

#257 — 45-day pruning in filter_old_unlikes

LGTM. Faithful copy of the subscription aggregate pattern with the same three-condition guard:

1. unliked_at != nil (was unliked)
2. unliked_at > liked_at (guards against corrupted timestamps)
3. unliked_at < 45 days ago (old enough to prune)

Minor observation — nil liked_at fallback: the || DateTime.from_unix!(0) fallback means an entry with a nil liked_at would compare against 1970-01-01, so any unliked_at older than 45 days would trigger pruning. This is identical to the subscription aggregate and is arguably the right behaviour (prune if liked_at is corrupted), but it's not commented here the way it is in subscription.ex:

# The unsubscribed_at > subscribed_at guard prevents filtering entries with
# corrupted timestamps (e.g. unsubscribed before subscribed).

Consider adding the same clarifying comment to filter_old_unlikes for future maintainers.

Test gap (NICE-TO-HAVE): The test covers active, old-unliked, and recent-unliked entries. Missing: an entry where unliked_at == liked_at (same timestamp). The :gt condition means this entry would NOT be pruned. This is intentionally defensive, but there's no test asserting that invariant.

---

#260 — is_public option in popularity test helpers

LGTM. The opts \\ [] default makes this fully backward-compatible. The behaviour correctly mirrors the real PopularityProjector:

• Private user → likes incremented, user NOT added to likes_people
• Public user → both incremented (unchanged)

Consistent: PodcastUnliked/EpisodeUnliked helpers don't receive opts since privacy doesn't affect removal from likes_people, which is correct.

Episode-level podcast score: apply_popularity_episode_liked propagates is_public to the episode's likes_people but not to the podcast-level update — which only updates score, never likes_people. That matches the projector logic (podcast likes_people is managed via PodcastLiked, not EpisodeLiked). ✓

---

Issue #258

The PR body says it's documented as an "inherent snapshot pattern limitation, TODO in code" but there's no new TODO comment visible in the diff. If this was meant to be an existing comment, it might be worth referencing the issue number in it (e.g. # Known limitation (#258): ...) so it's traceable.

---

Summary

Item	Status
LikeNormalizer whitelist (#256)	✅ LGTM
45-day pruning logic (#257)	✅ LGTM
Missing comment on nil liked_at fallback	NICE-TO-HAVE
Missing edge-case test (unliked_at == liked_at)	NICE-TO-HAVE
is_public opt for popularity helpers (#260)	✅ LGTM
Issue #258 TODO traceability	NICE-TO-HAVE

No MUST-FIX or SHOULD-FIX items — safe to merge.

🤖 Claude Code review

[claude]

Code Review — PR #261 (likes follow-ups #256, #257, #260)

Overall: APPROVED — all three follow-up issues are properly resolved with solid tests and no MUST-FIX or SHOULD-FIX items.

---

#256 — LikeNormalizer whitelist

LGTM. The flat_map + Map.new() pattern is clean and the explicit whitelist is clearly the right approach over String.to_existing_atom. The _ catch-all arm that drops unknown-typed keys (non-binary, non-atom) is correct defensive behaviour for JSON-sourced checkpoint data.

One thing to be aware of for future developers: when a new field is added to like data maps (e.g. a new metadata field), this whitelist in LikeNormalizer must be updated explicitly — otherwise the field will be silently dropped on checkpoint replay. Worth mentioning in the module @moduledoc to prevent subtle bugs. This is NICE-TO-HAVE.

---

#257 — 45-day pruning in filter_old_unlikes

LGTM. Faithful copy of the subscription aggregate pattern with the correct three-condition guard.

Minor (NICE-TO-HAVE): The nil liked_at fallback || DateTime.from_unix\!(0) is not explained by a comment here, unlike in the subscription aggregate where this is annotated. Consider adding:

# When liked_at is nil, falls back to epoch so any valid unliked_at passes the guard.

Note to correct the existing review: The test for the unliked_at == liked_at edge case is present in this PR ("preserves entry where unliked_at equals liked_at (same timestamp)"). The existing review comment incorrectly listed this as a missing NICE-TO-HAVE. The test coverage is complete.

Minor style: Enum.into(%{}) is used at the end of filter_old_unlikes while |> Map.new() is used in atomize_keys in the same codebase. Both are equivalent; consider using Map.new() for consistency. NICE-TO-HAVE.

---

#260 — is_public option in popularity test helpers

LGTM. The opts \ [] default is fully backward-compatible. The two new tests correctly cover the private user paths for both podcast and episode likes.

Correct by design: PodcastUnliked/EpisodeUnliked helpers don't receive opts because privacy doesn't affect removal from likes_people — consistent with the real projector logic. ✓

The acceptance criteria for #260 mentioned potentially refactoring helpers to call real projector logic instead of reimplementing. The PR chose the simpler opts approach instead. Given the follow-up rule (no follow-up of a follow-up), this is the right call.

---

Issue #258 (documented limitation)

Issue is correctly closed. The PR body notes it's documented as an inherent snapshot pattern limitation with an existing TODO in PopularityProjector. No new code needed here.

---

Summary

Item	Status
LikeNormalizer whitelist (#256)	✅ LGTM
Whitelist update guidance in moduledoc	NICE-TO-HAVE
45-day pruning logic (#257)	✅ LGTM
nil liked_at fallback comment	NICE-TO-HAVE
Enum.into(%{}) vs Map.new() style	NICE-TO-HAVE
unliked_at == liked_at edge-case test	✅ Present (contrary to previous review)
is_public opt for popularity helpers (#260)	✅ LGTM
Issue #258	✅ Correctly closed as documented

No MUST-FIX or SHOULD-FIX items — safe to merge.

🤖 Claude Code review