Skip to content

[TV] Stop seeding the default playlists on TV - #5665

Open
sztomek wants to merge 5 commits into
feat/tv-playlists-tabfrom
feat/tv-playlist-seeding
Open

[TV] Stop seeding the default playlists on TV#5665
sztomek wants to merge 5 commits into
feat/tv-playlists-tabfrom
feat/tv-playlist-seeding

Conversation

@sztomek

@sztomek sztomek commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Description

Apple TV never seeds the default "New Releases" / "In Progress" playlists — its database only contains what sync brings down, so a signed-out session shows the Playlists empty state. On Android the shared repositories module seeded them on every app's first launch via an androidx-startup initializer, and UserManager.signOutAndClearData re-seeded them with force = true, which made the TV empty state unreachable.

This PR makes default-playlist seeding a per-platform decision:

  • DefaultPlaylistsInitializer becomes an interface with the existing logic in DefaultPlaylistsInitializerImpl and a NoOpDefaultPlaylistsInitializer. Each app binds it in its own DI module: phone/automotive/wear bind the real implementation, TV binds the no-op. UserManager keeps injecting the same type, so its force = true re-seed resolves to the no-op on TV with no diff in UserManager.
  • The androidx-startup initializer, its Hilt entry point, and the manifest provider entry are deleted. Each seeding app launches initialize() explicitly from Application.onCreate, before its blocking startup work so the timing matches the old content-provider trigger. TV launches nothing.

Phone, automotive, and wear behavior is unchanged: same seeding logic, same guard key, same scope/dispatcher, same sign-out re-seed.

Testing Instructions

  1. Fresh-install the mobile app and open the Playlists tab — "New Releases" and "In Progress" exist.
  2. Sign out with "Sign out and clear data" — both defaults are re-created.
  3. Fresh-install the TV app, choose "Browse without an account", and open the Playlists tab — no playlists exist (empty state, wired up in the follow-up PRs).

Screenshots or Screencast

Not applicable — no UI changes.

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.

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @sztomek's task in 1m 57s —— View job


PR Review: Stop seeding the default playlists on TV

I reviewed the full diff against origin/feat/tv-playlists-tab. This is a clean, well-scoped refactor. Splitting DefaultPlaylistsInitializer into an interface + Impl + NoOp and binding it per-app is the right pattern for a per-platform decision, and it keeps UserManager's force = true re-seed working unchanged (it already injected the interface type at UserManager.kt:67, so the no-op resolves automatically on TV). Behavior for phone/automotive/wear is preserved: same guard key, same @Singleton + Mutex, same applicationScope launch. No stale references to the deleted DefaultPlaylistsStartupInitializer / InitializerEntryPoint remain, and the manifest provider was removed cleanly.

Verified

  • ✅ All four apps that merged the old startup content-provider are handled: phone/automotive/wear bind Impl, TV binds NoOp.
  • applicationScope (@Inject) is populated by Hilt before setupApp() runs, and each seeding launch is on the same SupervisorJob() + Dispatchers.Default scope the old initializer used — so dispatcher/timing is equivalent.
  • ✅ Mutex + guard key make the startup initialize() and the sign-out initialize(force = true) safe to interleave.
  • WearAppModule object → abstract class + companion object conversion is the correct Dagger idiom for mixing @Provides and @Binds.
  • ✅ Good unit coverage for the three branches (first seed / already-seeded / forced).

Minor observations (non-blocking)

  1. Test hardcodes the guard-key string. DefaultPlaylistsInitializerImplTest redefines const val CREATED_KEY = "createdDefaultPlaylists" locally, mirroring the private constant in the impl. If the source key ever changes, these tests would keep passing against the old string and silently give false confidence. Consider exposing the constant (e.g. internal) and referencing it from the test. Fix this →

  2. Inconsistent error handling across apps. Wear wraps seeding in runStartupStep("default playlists seeding") { … } (which logs/contains failures), while phone and automotive use a bare applicationScope.launch { defaultPlaylistsInitializer.initialize() }. Because the scope has a SupervisorJob but no CoroutineExceptionHandler, a throw from initialize() (it does DB writes) would propagate to the default uncaught handler. This is not a regression — the old DefaultPlaylistsStartupInitializer did the exact same bare launch — but since you're already touching these call sites it might be worth aligning phone/automotive with wear's safer wrapping.

  3. Timing note (informational). The old androidx-startup provider ran during ContentProvider.onCreate, i.e. before Application.onCreate; the new call runs inside setupApp() during onCreate. Since seeding is async (launch) and idempotent via the guard key, this is fine — just confirming it's a deliberate, harmless shift rather than an exact match.

Overall this looks correct and ready. The points above are optional polish. Nice work isolating the change so UserManager needed no diff.

  • Read the changed files
  • Analyze DI wiring and seeding logic per platform
  • Check tests
  • Post review

@wpmobilebot

wpmobilebot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Project manifest changes for app

The following changes in the app's merged AndroidManifest.xml file were detected (build variant: release):

--- ./build/reports/diff_manifest/app/release/base_manifest.txt	2026-07-28 15:38:36.048039377 +0000
+++ ./build/reports/diff_manifest/app/release/head_manifest.txt	2026-07-28 15:38:37.413163484 +0000
@@ -798,9 +798,6 @@
             android:authorities="au.com.shiftyjelly.pocketcasts.androidx-startup"
             android:exported="false" >
             <meta-data
-                android:name="au.com.shiftyjelly.pocketcasts.repositories.playlist.DefaultPlaylistsStartupInitializer"
-                android:value="androidx.startup" />
-            <meta-data
                 android:name="androidx.emoji2.text.EmojiCompatInitializer"
                 android:value="androidx.startup" />
             <meta-data

Go to https://buildkite.com/automattic/pocket-casts-android/builds/17894/canvas?sid=019fa95f-96fb-4f30-91d3-04bd2facc536, click on the Artifacts tab and audit the files.

@wpmobilebot

wpmobilebot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Project manifest changes for wear

The following changes in the wear's merged AndroidManifest.xml file were detected (build variant: release):

--- ./build/reports/diff_manifest/wear/release/base_manifest.txt	2026-07-28 15:38:44.303201474 +0000
+++ ./build/reports/diff_manifest/wear/release/head_manifest.txt	2026-07-28 15:38:45.508319731 +0000
@@ -177,9 +177,6 @@
             android:authorities="au.com.shiftyjelly.pocketcasts.androidx-startup"
             android:exported="false" >
             <meta-data
-                android:name="au.com.shiftyjelly.pocketcasts.repositories.playlist.DefaultPlaylistsStartupInitializer"
-                android:value="androidx.startup" />
-            <meta-data
                 android:name="androidx.emoji2.text.EmojiCompatInitializer"
                 android:value="androidx.startup" />
             <meta-data

Go to https://buildkite.com/automattic/pocket-casts-android/builds/17894/canvas?sid=019fa95f-96fb-4f30-91d3-04bd2facc536, click on the Artifacts tab and audit the files.

@wpmobilebot

wpmobilebot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Project manifest changes for automotive

The following changes in the automotive's merged AndroidManifest.xml file were detected (build variant: release):

--- ./build/reports/diff_manifest/automotive/release/base_manifest.txt	2026-07-28 15:38:51.209941039 +0000
+++ ./build/reports/diff_manifest/automotive/release/head_manifest.txt	2026-07-28 15:38:52.453246395 +0000
@@ -124,9 +124,6 @@
             android:authorities="au.com.shiftyjelly.pocketcasts.androidx-startup"
             android:exported="false" >
             <meta-data
-                android:name="au.com.shiftyjelly.pocketcasts.repositories.playlist.DefaultPlaylistsStartupInitializer"
-                android:value="androidx.startup" />
-            <meta-data
                 android:name="androidx.emoji2.text.EmojiCompatInitializer"
                 android:value="androidx.startup" />
             <meta-data

Go to https://buildkite.com/automattic/pocket-casts-android/builds/17894/canvas?sid=019fa95f-96fb-4f30-91d3-04bd2facc536, click on the Artifacts tab and audit the files.

@sztomek
sztomek marked this pull request as ready for review July 28, 2026 15:20
@sztomek
sztomek requested a review from a team as a code owner July 28, 2026 15:20
@sztomek
sztomek requested review from geekygecko and removed request for a team July 28, 2026 15:20
@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @sztomek's task in 1m 58s —— View job


PR Review: Stop seeding the default playlists on TV

I reviewed the full diff against origin/feat/tv-playlists-tab and verified the wiring across all four apps. This is a clean, well-scoped refactor — turning DefaultPlaylistsInitializer into an interface and binding Impl (phone/automotive/wear) vs NoOp (TV) per-app is exactly the right way to make seeding a per-platform decision, and it keeps UserManager's force = true re-seed working with zero diff in UserManager (it injects the interface type at UserManager.kt:67, so the no-op resolves automatically on TV).

Verified ✅

  • No stale referencesgit grep finds no remaining uses of DefaultPlaylistsStartupInitializer, InitializerEntryPoint, or initializerEntryPoint; the manifest <provider> entry was removed cleanly (confirmed by the manifest-diff bot for app/wear/automotive).
  • All seeding apps launch correctlyapplicationScope is @Injected and populated by Hilt before setupApp() runs in all three (phone :209, automotive :103, wear :120), on the same SupervisorJob() + Dispatchers.Default scope the old initializer used, so dispatcher/timing is equivalent.
  • Concurrency is safe@Singleton impl + a single Mutex + the createdDefaultPlaylists guard key make the startup initialize() and the sign-out initialize(force = true) safe to interleave; the guard makes re-launch on every cold start idempotent.
  • WearAppModule conversion is correctobject → abstract class + companion object for @Provides alongside the new @Binds is the proper Dagger idiom.
  • Good branch coverage — the new test covers first-seed / already-seeded / forced.

Minor observations (non-blocking) — posted as inline comments

  1. Test hardcodes the guard key (DefaultPlaylistsInitializerImplTest.kt) — the local CREATED_KEY = "createdDefaultPlaylists" mirrors the private constant in the impl. If the source key ever changes, the tests keep passing against the stale string. Consider exposing it internal and referencing it. Fix this →
  2. Inconsistent error handling — wear wraps seeding in runStartupStep("default playlists seeding") { … }, but phone/automotive use a bare applicationScope.launch { … }. The scope has a SupervisorJob but no CoroutineExceptionHandler, so a throw from initialize() (it does DB writes) reaches the default uncaught handler. Not a regression — the old DefaultPlaylistsStartupInitializer did the same bare launch — but since you're touching these sites, aligning them with wear's wrapping would contain failures.

Nits (optional)

  • NoOpDefaultPlaylistsInitializer isn't @Singleton, so each injection point (Application + UserManager) gets its own instance. It's stateless, so this is harmless — annotating it just mirrors the impl for consistency.
  • Timing shift (informational): the old provider ran in ContentProvider.onCreate (before Application.onCreate); the new call runs inside setupApp(). Because seeding is async and idempotent, no consumer could have relied on it being done synchronously, so this is a safe, deliberate shift.

Overall this is correct and ready — the points above are optional polish. Nice isolation work keeping UserManager untouched.

  • Read the changed files
  • Analyze DI wiring & seeding logic per platform
  • Check tests
  • Post review

@sztomek sztomek added this to the 8.18 milestone Jul 28, 2026
}

private companion object {
const val CREATED_KEY = "createdDefaultPlaylists"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test re-declares CREATED_KEY = "createdDefaultPlaylists" locally, duplicating the private const val CREATED_DEFAULT_PLAYLISTS_KEY in DefaultPlaylistsInitializerImpl. If the production key ever changes, settings.getBooleanForKey(...)/setBooleanForKey(...) in the impl would use the new key while these tests keep stubbing/verifying the old string — the tests would still pass and silently give false confidence. Consider making the source constant internal and referencing it here so the two can't drift.

private fun setupApp() {
LogBuffer.i("Application", "App started. ${settings.getVersion()} (${settings.getVersionCode()})")

applicationScope.launch { defaultPlaylistsInitializer.initialize() }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, and not a regression (the old DefaultPlaylistsStartupInitializer used the same bare launch): initialize() does DB writes, and this bare applicationScope.launch { } has no try/catch. The scope uses SupervisorJob() (so siblings survive) but has no CoroutineExceptionHandler, so a throw here reaches the default uncaught-exception handler and can crash startup. Wear already wraps its call in runStartupStep("default playlists seeding") { … }. Since you're touching these call sites anyway, aligning phone + automotive with wear's wrapping would make failures contained/logged rather than fatal.

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

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


PR Review: Stop seeding the default playlists on TV

Re-reviewed the latest state (including f88faf1 Address PR changes) against origin/feat/tv-playlists-tab. Both points from the earlier review have been addressed, and the change is clean and ready.

Prior feedback — resolved ✅

  1. Test no longer hardcodes the guard key. CREATED_DEFAULT_PLAYLISTS_KEY is now internal in DefaultPlaylistsInitializerImpl, and DefaultPlaylistsInitializerImplTest imports it (...Companion.CREATED_DEFAULT_PLAYLISTS_KEY) instead of redeclaring the string — the source and test can no longer drift.
  2. Error handling aligned. Phone (PocketCastsApplication.kt:210) and automotive (AutomotiveApplication.kt:104) now wrap initialize() in try/catch, rethrow CancellationException, and log other failures via LogBuffer.e(TAG_BACKGROUND_TASKS, …). A DB-write failure during seeding is now contained/logged rather than reaching the default uncaught handler. Verified LogBuffer is imported in both files and TAG_BACKGROUND_TASKS exists.

Verified ✅

  • Per-platform binding is correct — phone/automotive/wear bind DefaultPlaylistsInitializerImpl, TV binds NoOpDefaultPlaylistsInitializer. UserManager's force = true re-seed resolves to the no-op on TV with zero diff in UserManager.
  • No stale references — old DefaultPlaylistsStartupInitializer, InitializerEntryPoint, and the manifest <provider> entry are removed cleanly (confirmed by the manifest-diff bot for app/wear/automotive).
  • Concurrency safe@Singleton impl + Mutex + createdDefaultPlaylists guard key keep startup initialize() and sign-out initialize(force = true) idempotent and safe to interleave.
  • WearAppModule conversion is correctobject → abstract class + companion object for @Provides alongside @Binds is the proper Dagger idiom.
  • Tests cover first-seed / already-seeded / forced branches.

Nits (optional, non-blocking)

  • Two error-handling styles. Phone/automotive use an inline try/catch, wear uses runStartupStep("default playlists seeding") { … }. Both contain failures, so this is purely cosmetic — but a shared helper (or having phone/automotive adopt runStartupStep if it were shared infrastructure) would keep the three call sites identical. Fine to leave as-is.
  • NoOpDefaultPlaylistsInitializer isn't @Singleton. It's stateless, so each injection point getting its own instance is harmless; annotating it would just mirror the impl for consistency.

Overall: correct, well-isolated, and ready to merge. Nice work keeping UserManager untouched.

  • Read the changed files
  • Verify prior review feedback was addressed
  • Analyze DI wiring & seeding logic per platform
  • Check tests
  • Post review

@geekygecko geekygecko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants