Improve feedback when adding episodes to playlists

[joashrajin]

Description

Extend the feedback shown after changing an episode's manual playlist membership.

The existing flow already confirms playlist additions. This change adds the remaining feedback states for removals and mixed changes, while preserving Added to <playlist title> with a View action when adding to exactly one existing playlist. Multiple additions, removals, and mixed changes use the count-based copy from the Figma designs.

As an incidental fix, the summary now counts directly from the cached playlist changes instead of intersecting them with the search-filtered playlist list. Toggling a playlist and then changing the search term therefore no longer suppresses its snackbar feedback. The cached change also retains the title and UUID needed by the single-playlist confirmation and its View destination.

The create-new-playlist confirmation is unchanged: OpenCreatedPlaylistEffect continues to show Added to <playlist title> with a View action.

Fixes PCDROID-470

Testing Instructions

1. Open an English-language podcast and long-press an episode.
2. Select More options → Add to playlist.
3. Add the episode to one existing playlist and verify Added to <playlist title> appears with a View action. Tap View and verify the named playlist opens.
4. Add the episode to two playlists and verify Added to 2 playlists appears.
5. Remove the episode from one playlist and verify Removed from playlist appears.
6. Remove the episode from two playlists and verify Removed from 2 playlists appears.
7. Swap one selected playlist for one unselected playlist and verify Changed 2 playlists appears.
8. Confirm the count-based and removal messages do not include a View action.

The five feedback states and refreshed screenshots were tested on a Samsung SM-G990E running Android 16 with the debugProd build, using the Darknet Diaries episode “National Public Data.” This included the revised single-playlist title, View action, and destination. The single-playlist flow was also verified on a Pixel 8 emulator running Android 14.

Screenshots or Screencast

Design

The screenshots are ordered as: remove from one playlist, remove from two playlists, add to one playlist, add to two playlists, and change two playlists. The single-playlist addition preserves its named confirmation and View action; the remaining states follow the designs above.

Before

After

Checklist

• If this is a user-facing change, I have added an entry in CHANGELOG.md
• Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
• I have considered whether it makes sense to add tests for my changes
• All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
• Any jetpack compose components I added or changed are covered by compose previews
• I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics.

I have tested any UI changes...

• with different themes
• with a landscape orientation
• with the device set to have a large display and font size
• for accessibility with TalkBack

[claude]

Claude finished @joashrajin's task in 1m 45s —— View job

---

Code Review: Improve feedback when adding episodes to playlists

• Read PR diff and changed files
• Analyze AddToPlaylistViewModel / AddToPlaylistFragment logic
• Check for dead code / unused strings
• Review localization and tests

---

Overall this is a clean, well-scoped change. Tracking additions and removals separately is the right model, the new test is a good addition, and the when branch logic in showDoneSnackbar is correct for all six count combinations (single/plural add, single/plural remove, mixed, no-op). Nice work.

👍 Nice catch (latent bug fixed)

The old getPlaylistsAddedTo() filtered against uiState.value?.playlistPreviews, which is the search-filtered list. So if a user had a search term active when dismissing the sheet, playlists they'd added that no longer matched the filter would have been dropped from the count. The new getPlaylistChangeSummary() counts directly from playlistsChanges, which is both simpler and more correct — and it now matches exactly what commitPlaylistChanges() writes. Good improvement, even if incidental.

Suggestions (all minor / non-blocking)

1. String naming inconsistency — strings.xml
The single-add string is named added_to_playlist_feedback while its counterpart is removed_from_playlist. Consider a consistent pair (e.g. added_to_playlist / removed_from_playlist) so they read as siblings. added_to_playlist_single is already taken by the older "Added to %1$s" string (still used by OpenCreatedPlaylistEffect), so a plain added_to_playlist would avoid confusion with that.

2. Unreachable one plural forms — strings.xml
Because addedCount == 1 and removedCount == 1 are handled by dedicated non-numbered strings, the <item quantity="one"> forms of added_to_playlist_single_multiple ("Added to 1 playlist") and removed_from_playlists ("Removed from 1 playlist") are now unreachable from this code path. Not harmful (translators still get a full plural set, and some locales need the form), just worth being aware of.

3. Mixed-change wording — AddToPlaylistFragment.kt:171-177
"Changed N playlists" uses totalCount, so swapping one playlist for another reports "Changed 2 playlists". That matches the ticket's testing instructions, so this is fine — flagging only so it's a conscious product decision that a swap counts as touching 2 playlists rather than 1 "change".

4. Loss of the "View" action — intended per the description. Just confirming the trade-off is deliberate: single adds no longer offer a one-tap jump to the playlist. (The new-playlist creation flow in OpenCreatedPlaylistEffect still keeps its View action, so behavior there is unchanged.)

Test coverage

The ViewModel counting logic is now tested — good. The message-selection when in showDoneSnackbar (which string/plural is chosen per count combo) isn't covered, but since it lives in the Fragment that's understandable. No action needed.

Housekeeping

• The checklist notes CHANGELOG.md hasn't been updated. This is a user-facing change, so it likely warrants an entry.
• PR description mentions before/after screenshots should be attached before marking ready for review.

Nothing here is blocking — the core logic is correct and the change is a genuine improvement over the previous behavior.

[claude]

Claude finished @joashrajin's task in 1m 37s —— View job

---

Code Review: Improve feedback when adding episodes to playlists

• Read PR diff and changed files
• Analyze ViewModel / Fragment logic
• Check strings & localization
• Review tests

---

This is a clean, well-scoped change and it looks ready. The refactor to a sealed PlaylistChangeFeedback type is a nice improvement over the previous Set<PlaylistPreviewForEpisode>-based approach — the message-selection logic now lives in the ViewModel where it's unit-testable, and the Fragment just renders. I also confirmed the suggestions from the earlier review pass have been addressed: string naming is now a consistent pair (added_to_playlist_feedback / removed_from_playlist_feedback), the CHANGELOG entry is present, and there's good test coverage.

✅ Correctness

• PlaylistChangeFeedback.from branch order is correct. The mixed case (added > 0 && removed > 0) is checked first, so it can't be shadowed by the later single-direction branches. All six count combinations (single/plural add, single/plural remove, mixed, no-op) resolve to the right message. Verified against the parameterized test in AddToPlaylistViewModelTest.
• The counting is genuinely "net" changes. cachePlaylistChange (AddToPlaylistViewModel.kt:101-112) removes the map entry when a playlist is toggled back to its original state, so getPlaylistChangeSummary counts only real add/remove deltas — and it reads from the same playlistsChanges map that commitPlaylistChanges writes, so the snackbar can never disagree with what was persisted. This also fixes the latent bug where a search term could suppress feedback (the old code intersected with the search-filtered playlistPreviews). Good incidental fix.
• No threading concern. getPlaylistChangeSummary() is called from onDismiss on the main thread, and cachePlaylistChange only mutates the map on the main thread, so there's no concurrent read/modify of the plain mutableMapOf.

Minor / non-blocking

1. CHANGELOG categorization — CHANGELOG.md: the entry is filed under New Features, but this reads more like an enhancement to existing feedback (the add-snackbar already existed). Consider whether Bug Fixes / an "Improvements" bucket fits better. Purely cosmetic.
2. Unreachable one plural forms — strings.xml: since addedCount == 1 / removedCount == 1 route to the dedicated non-numbered strings, and the mixed branch always has a quantity ≥ 2, the <item quantity="one"> forms of added_to_playlist_single_multiple, removed_from_playlists, and changed_playlists are unreachable from this code path. Harmless — translators still need the full plural set for correctness in other locales — just noting it.
3. String grouping — strings.xml: removed_from_playlist_feedback / removed_from_playlists were inserted near playlist_sort_by rather than beside the added_* / changed_* siblings. Not enforced, just a readability nit if you happen to be in there.

Test coverage

The ViewModel counting and message-selection logic are both well covered. The getString / getQuantityString rendering in AddToPlaylistFragment.showDoneSnackbar isn't unit-tested, but that's a thin Fragment-level mapping and reasonable to leave.

Nothing blocking here — the core logic is correct and this is a solid improvement. 👍

[geekygecko]

Thanks for making this change. I might just check with the product team about losing the view button, as that seems useful to me. p1785119056617199-slack-C05RR9P9RAT

[geekygecko]

@joashrajin we have feedback from the design team. Can we keep the playlist title and the view button so users can access the playlist page after adding it to a single playlist?

[joashrajin]

@joashrajin we have feedback from the design team. Can we keep the playlist title and the view button so users can access the playlist page after adding it to a single playlist?

Sure thing, thanks Phil!

[claude]

Claude finished @joashrajin's task in 1m 51s —— View job

---

Code Review: Preserve single-playlist navigation

• Read PR diff and changed files
• Analyze the "Preserve single playlist navigation" commit
• Verify correctness of the title + View action restoration
• Post review

---

Reviewed the latest commit (4537c71), which addresses the design-team feedback from @geekygecko. This does exactly what was asked: a single add now shows Added to <title> with a View action, while the plural/removal/mixed states keep the generic action-less copy. Nicely scoped.

✅ Correctness

• The requireNotNull calls are safe. In PlaylistChangeFeedback.from, SinglePlaylistAddition is only chosen when addedCount == 1 && removedCount == 0 (the mixed case is checked first), so the map holds exactly one entry with shouldAdd == true. That means singleOrNull { it.value.shouldAdd } in getPlaylistChangeFeedback() (AddToPlaylistViewModel.kt:130-138) always resolves to that entry, and requireNotNull(singleAddedPlaylist) (:377) can't throw. Likewise requireNotNull(change.playlistTitle) (:136) holds because every shouldAdd == true entry comes from addToPlaylist(...), which now always passes a non-null playlistTitle — removeFromPlaylist is the only caller that leaves the title null, and it sets shouldAdd = false. The invariant is preserved by construction; the guards are a reasonable belt-and-suspenders.
• Title is captured at tap time from playlist.title and cached in PlaylistChange, independent of the search-filtered list — consistent with the earlier fix that decoupled the summary from playlistPreviews. No regression there.
• Snackbar refactor is clean. showDoneSnackbar now builds the Snackbar per-branch so only SinglePlaylistAddition gets .setAction(view), and the memory-leak-safe openManualPlaylist extension is reused. PlaylistChangeFeedback.None still early-returns. Good.
• No dangling strings. added_to_playlist_feedback is fully removed and has no remaining references; added_to_playlist_single ("Added to %1$s") is now shared by both this path and OpenCreatedPlaylistEffect, which is fine.
• Tests updated correctly. The parameterized FeedbackCase covers the new SinglePlaylistAddition outcome, and single playlist addition feedback keeps the cached playlist title and uuid verifies the title/uuid survive an add + add + remove sequence. Good coverage of the new behavior.

Minor / non-blocking

1. CHANGELOG categorization — CHANGELOG.md:5-6: the entry sits under New Features, but this is an enhancement to existing feedback. A Bug Fixes/improvements bucket may read better. Purely cosmetic (flagged in an earlier pass too).
2. String grouping — strings.xml:1018: removed_from_playlist_feedback / removed_from_playlists sit next to playlist_sort_by rather than beside the added_* / changed_* siblings around line 956. Readability nit only.
3. Unreachable one plural forms — the <item quantity="one"> forms of added_to_playlist_single_multiple, removed_from_playlists, and changed_playlists remain unreachable from this code path (the == 1 cases route to dedicated strings, and the mixed branch is always ≥ 2). Harmless — translators still need the full plural set — just noting it persists.

Nothing blocking. The change is correct and matches the design-team request. 👍

[claude]

Claude finished @joashrajin's task in 1m 28s —— View job

---

Code Review: Improve feedback when adding episodes to playlists

• Read latest changes
• Review ViewModel / Fragment feedback logic
• Verify strings & localization
• Review tests
• Post review

---

Reviewed the current state of the branch, including the latest commit c8a4df1 ("Group playlist feedback strings"). This addresses the string-grouping readability nit from earlier passes — removed_from_playlist_feedback / removed_from_playlists / changed_playlists now sit right beside the added_* siblings (strings.xml:956–972). The change is correct and, in my view, ready.

✅ Correctness (re-verified)

• PlaylistChangeFeedback.from branch order holds. The mixed case (addedCount > 0 && removedCount > 0) is checked first, so it can't be shadowed by the single-direction branches below. All seven combinations exercised in the parameterized test (AddToPlaylistViewModelTest.kt) resolve to the expected message, and the no-op case falls through to None.
• The requireNotNull guards are safe by construction (AddToPlaylistViewModel.kt:130–138, :361). singleAddedPlaylist is only consumed by the addedCount == 1 && removedCount == 0 branch, where playlistsChanges contains exactly one shouldAdd == true entry, so singleOrNull { it.value.shouldAdd } resolves it. requireNotNull(change.playlistTitle) holds because every shouldAdd == true entry originates from addToPlaylist(...), which now always passes a non-null title; only removeFromPlaylist leaves the title null, and that path sets shouldAdd = false. Even in a mixed change (e.g. added=1, removed=5) where singleAddedPlaylist is still computed, the title is non-null and the value is simply unused — no crash path.
• Counting is net and consistent. cachePlaylistChange drops the map entry when a playlist is toggled back to its original state, and getPlaylistChangeSummary reads from the same playlistsChanges map that commitPlaylistChanges persists — so the snackbar can never disagree with what's written. This retains the earlier incidental fix that decoupled the count from the search-filtered playlistPreviews.
• Snackbar rendering is exhaustive. The when (feedback) in showDoneSnackbar covers SinglePlaylistAddition (the only branch that gets .setAction(view)), StringResource, PluralResource, and early-returns on None. Matches the design-team request to keep the title + View action for a single add.

Minor / non-blocking (carried over)

1. CHANGELOG categorization — CHANGELOG.md:6: still filed under New Features; reads more like an enhancement to existing feedback. Purely cosmetic.
2. Unreachable one plural forms — the <item quantity="one"> forms of the three plurals stay unreachable from this code path (the == 1 cases route to dedicated strings, mixed is always ≥ 2). This is now nicely documented by the `` comments, so translators still get the full set. Good call adding those.

Nothing blocking — the logic is correct, well-tested at the ViewModel level, and the change now matches both the design-team feedback and the Figma copy. 👍

[joashrajin]

@geekygecko please could you give it another check 🙇

[claude]

Claude finished @joashrajin's task in 1m 50s —— View job

---

Code Review: Improve feedback when adding episodes to playlists

• Read PR diff and changed files
• Review ViewModel / Fragment feedback logic
• Verify strings & localization
• Review tests
• Post review

---

Re-reviewed the branch after the sync with main (88e1f1d merges origin/main). The merge is clean — main didn't touch modules/features/filters/.../playlists/manual/, so there's no semantic conflict with the feedback work, and the diff against origin/main is still just the five intended files. My conclusion is unchanged: the logic is correct and this looks ready.

✅ Verified in this pass

• PlaylistChangeFeedback.from branch order is sound (AddToPlaylistViewModel.kt:363–397). The mixed case (addedCount > 0 && removedCount > 0) is evaluated first, so it can't be shadowed by the single-direction branches. All seven combinations in the parameterized test resolve as expected, and the no-op falls through to None.
• requireNotNull guards hold by construction. singleOrNull { it.value.shouldAdd } (:130–138) can only resolve to an entry created by addToPlaylist(...), which always passes a non-null playlistTitle; removeFromPlaylist is the only caller that leaves the title null and it sets shouldAdd = false. requireNotNull(singleAddedPlaylist) at :377 is likewise only reached when addedCount == 1 && removedCount == 0.
• Counting is net and consistent with what's persisted. cachePlaylistChange's merge returns null when a playlist is toggled back to its original state, dropping the entry — so getPlaylistChangeSummary reads the same playlistsChanges map that commitPlaylistChanges (:217–223) iterates. The snackbar can't disagree with the write. This retains the incidental fix that decoupled the count from the search-filtered playlistPreviews.
• Single-add navigation is preserved per the design-team request (AddToPlaylistFragment.kt:174–179): Added to <title> + View, routed through the existing leak-safe openManualPlaylist extension (:267). The plural/removal/mixed branches are correctly action-less, and None early-returns.
• No dangling resources. added_to_playlist_feedback is gone with no remaining references; added_to_playlist_single is now shared by this path and OpenCreatedPlaylistEffect (:202), which is fine. removed_from_playlist_feedback / removed_from_playlists / changed_playlists are each referenced exactly once, and the strings now sit beside their added_* siblings (strings.xml:955–974) with translator comments on the plural sets.
• No threading concern. getPlaylistChangeFeedback() runs on the main thread from onDismiss, and cachePlaylistChange only mutates the plain mutableMapOf from main-thread entry points — the Dispatchers.Default launches only touch previewsFlow.

Minor / non-blocking

1. playlistTitle default invites a future null crash — AddToPlaylistViewModel.kt:101–105. playlistTitle: String? = null means a new shouldAdd = true call site can omit the title and the failure surfaces later as a requireNotNull crash at dismiss time rather than at the call site. Encoding the invariant in the type would make that unrepresentable — e.g. drop the default and have removeFromPlaylist pass playlistTitle = null explicitly, or split PlaylistChange into Add(title) / Remove variants. Purely defensive; nothing is broken today. Fix this →
2. getPlaylistChangeSummary() visibility — :122. It's now public but only consumed by getPlaylistChangeFeedback() and the tests; internal/private with the summary asserted via feedback would shrink the ViewModel's surface. Judgement call — it does make the counting directly testable, which has value.
3. CHANGELOG categorization — CHANGELOG.md:3–5: still filed under New Features, though it reads as an enhancement to feedback that already existed. Cosmetic; carried over from earlier passes.
4. Unreachable one plural forms — the <item quantity="one"> forms of the three plurals stay unreachable from this code path (== 1 routes to dedicated strings, mixed is always ≥ 2). Harmless, and the new comments make the intent explicit for translators. 👍

Not verified

I wasn't able to run ./gradlew :modules:features:filters:testDebugUnitTest or spotlessCheck in this environment — Gradle invocations aren't in my allowed tools here, so the test and formatting verdicts come from reading the code, not from execution. Worth confirming CI is green before merge (the only completed checks on this push so far are the two Gradle-wrapper validations).

Nothing blocking. 👍
• Branch: codex/pcdroid-470-playlist-feedback

[wpmobilebot]

Version 8.18 has now entered code-freeze, so the milestone of this PR has been updated to 8.19.

[wpmobilebot]

Version 8.19 has now entered code-freeze, so the milestone of this PR has been updated to 8.20.