[HLS] Rework HLS stream url selection logic

[sztomek]

Description

Previously we had BaseEpisode.hlsUrl that was assigned after data > domain > database conversion. This way we exposed only the first alternate enclosure, preventing us to read the entire list.
This PR gets rid of hls_url column from the database so we can work with the entire list as the server sends it.

Fixes PCDROID-608 https://linear.app/a8c/issue/PCDROID-608/update-api-parsing-logic

Testing Instructions

1. Just make sure the databse migration tests run fine

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 finished @sztomek's task in 3m 25s —— View job

---

Code Review

I reviewed the HLS stream-resolution rework. Overall this is a clean refactor — dropping the persisted hls_url column in favor of reading the full alternate-enclosure list at playback time is the right direction, the migration is correct, and the test coverage is good. I left a few inline comments; summary below.

Review checklist

• Read the changed files and understand the rework
• Review DB migration & schema (135.json)
• Review entity / domain model changes
• Review repository & playback logic
• Post review feedback

---

🟠 Main concern — possible isHlsOnly regression for HLS-only feeds

BaseEpisode.kt:138

The old isHlsOnly had a (downloadUrl == null && hlsUrl != null) branch backed by the persisted hlsUrl. Now that alternate enclosures live in a separate table, a property getter can't see them, so an episode with no progressive download_url but an HLS alternate enclosure (and a non‑HLS/null fileType) is no longer detected as HLS-only. That flips canQueueForAutoDownload, DownloadManager, and the download-button visibility in EpisodeFragment, so such an episode would be treated as downloadable with a null URL. Playback itself is unaffected (applyStreamOverride resolves the enclosure when downloadUrl.isNullOrBlank()). Worth confirming whether the server always sends a progressive url alongside HLS enclosures — if not, this needs a synchronous signal (a persisted flag, or setting fileType to the HLS MIME type during refresh).

🟡 Minor / cleanup

• Dead/speculative video-stream API — isStreamVideo + the video/* content-type branch are never exercised in production: applyStreamOverride always sets the override content type to APPLICATION_M3U8. Add a TODO if video resolution is a follow-up, or drop it.
• defaultHlsStreamUrl is only used by tests — AlternateEnclosures.kt:16. applyStreamOverride calls firstHlsStreamUrl() directly. Either route production through it or remove it.
• Prefetch dedup race — PlaybackManager.prefetchNextEpisodeIfNeeded now reads/writes lastPrefetchedEpisodeUuid inside a launch {}, so two rapid invocations for the same next episode can both pass the dedup guard and enqueue duplicate prefetch work.

✅ Looks good

• Migration 134 → 135 — the recreated podcast_episodes_new table matches 135.json's createSql exactly (columns, order, NOT NULL), all non‑hls_url columns are copied, and the three indices are recreated. AppDatabaseTest updated accordingly.
• HLS MIME matching consolidated into BaseEpisode.HLS_MIME_TYPES and broadened to the common variants — and the duplicate servers/AlternateEnclosures.kt removed. Nice de-duplication.
• applyStreamOverride resets the override fields each call (idempotent) and correctly handles the HLS-only case via the downloadUrl.isNullOrBlank() fallback.
• Test coverage (BaseEpisodeStreamOverrideTest, updated EpisodeInfoTest, AlternateEnclosuresTest) is solid.

One nit on testing: the PR description only calls out running the migration tests — given the isHlsOnly/streamUrl semantics changed, it'd be worth a manual smoke test of streaming + downloading an HLS episode with the flag on/off.
• branch feat/hls-stream-resolution

[dangermattic]

	1 Warning
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

[geekygecko]

This feels like a really dangerous migration to do as this table could be very large. How about we remove adding the hls_url column before it goes to production?

val MIGRATION_131_132 = addMigration(131, 132) { database ->
    database.execSQL("ALTER TABLE podcast_episodes ADD COLUMN hls_url TEXT") // delete this
}

[sztomek]

hmmm fair point, but we've already shipped database version 132 in our betas, so beta builds would break... as an alternative, we could keep the hls_url column untouched in the table but would not use its value in our domain layer.
what do you reckon? @geekygecko

[geekygecko]

we've already shipped database version 132 in our betas, so beta builds would break...

If we leave the column for the beta users but remove the ALTER TABLE statement before we ship to production would there be an issue for either of them? I'm happy to do whatever you think but don't reckon we should do a full table copy.

[sztomek]

i think this would leave the app crashing for our beta users who installed the previous betas. claude has confirmed that as well, so i won't take this path.
as an alternative to full table migration, we should try dropping a column, but SQLite's ALTER TABLE … DROP COLUMN only exists since 3.35 and isn't available on older Android API levels, so the table-rebuild dance is the only portable way to actually remove a column.

i think the best course of action is to leave the column dormant and just get rid of the latest migration.
what are your thoughts, @geekygecko ?

[claude]

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

---

Re-review of PR #5497 — HLS stream url selection rework

Re-reviewed the latest state (through ca32445 Address PR comment). Nice work — the four points from my first pass are all resolved. A few things remain, the migration debate being the main one.

• Fetch base & inspect latest commits
• Confirm prior review points resolved
• Review the migration discussion (full table copy vs. keep column)
• Review entity / playback logic changes
• Post review feedback

---

✅ Resolved since last review

• isHlsOnly regression — now handled at parse time: DataParser and EpisodeInfo set fileType = enclosures.firstHlsMimeType() ?: fileType when url.isNullOrBlank(), so URL-less HLS episodes are detected synchronously. 👍
• Prefetch dedup race — lastPrefetchedEpisodeUuid is now an AtomicReference and the guard uses getAndSet(...) == uuid, which closes the concurrent-launch window.
• Dead isStreamVideo / video/* branch and defaultHlsStreamUrl — both removed. Clean.

---

🟠 Migration 134 → 135 — the open debate (@geekygecko / @sztomek)

There's one technical constraint worth making explicit, because it rules out the simplest-sounding option: you can't just "leave the hls_url column for beta users" while the entity/schema no longer declares it. Room validates the live schema against the entity on open (TableInfo.equals), and an extra column that the entity doesn't expect fails validation and throws on startup. So beta users who already ran v132 (column present) must have it dropped somehow, or they crash.

That leaves two options that actually work:

Option A — Skip the rebuild for users who never had the column. Remove the ALTER TABLE … ADD COLUMN hls_url from MIGRATION_131_132, and guard the rebuild in 134→135 so it only runs when the column is actually present:

val hasHlsColumn = database.query("PRAGMA table_info(`podcast_episodes`)").use { c ->
    val nameIdx = c.getColumnIndex("name")
    generateSequence { if (c.moveToNext()) c.getString(nameIdx) else null }.any { it == "hls_url" }
}
if (hasHlsColumn) { /* existing create_new / copy / drop / rename / indices */ }

• Production users (upgrading from < 132): never get the column → guard skips → zero full-table copy.

• Beta users (already at 132 with the column): guard fires → rebuild drops it → correct schema, no crash.

  Caveat: the exported 132.json still declares hls_url, so MigrationTestHelper-based tests that hop through v132 would need attention (regenerating a shipped schema is itself awkward). This is the more surgical fix but has this test wrinkle.

Option B — Keep the column, no 134→135 migration at all. Retain hls_url as a nullable, unused column and a corresponding ignored field on PodcastEpisode, and just stop reading/writing it in the domain layer (what @sztomek suggested). No data migration runs for anyone, prod or beta — lowest risk. Cost is a permanently dead column + a retained entity field.

My lean: Option B is the safest if you're comfortable with a dormant column; Option A is the right call if you'd rather not carry dead schema, accepting the 132-schema test cleanup. Either way, avoiding the unconditional full-table rebuild for the entire production base is the right instinct — a podcast_episodes rebuild can be very expensive for heavy users.

🟡 Minor

• HLS-only video mis-detection. The parse-time fix does fileType = enclosures.firstHlsMimeType() ?: fileType, which overwrites a real file_type (e.g. video/mp4) with the HLS MIME whenever url is blank. Since isVideo = fileType?.startsWith("video/"), an HLS-only video episode now reports isVideo == false / isAudio == true. If HLS-only video is possible, prefer the real type when it's a video: e.g. only substitute when the existing fileType isn't a video/* type. Fix this →
• AlternateEnclosureManager.observeForEpisode is unused in production — either wire it up where enclosures are observed or drop it until needed.
• Danger: missing AlternateEnclosureManagerImpl test. The impl is a thin DAO delegate, so a trivial verify-delegation test, or the unit-tests-exemption label, will clear the check.

---

Note: this is review feedback only — I haven't changed any code. Happy to implement whichever migration option you land on if you want.
• branch feat/hls-stream-resolution

[claude]

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

---

Re-review — latest push (e35b56f)

• Inspect new commits since last review
• Check status of open feedback (migration debate, HLS-only video, unused API, Danger test)
• Post review feedback

The new commit adds AlternateEnclosureManagerImplTest, which clears the outstanding Danger check. Here's where the open items stand.

---

✅ Resolved

• Danger: missing AlternateEnclosureManagerImpl test — AlternateEnclosureManagerImplTest now covers findForEpisode (both populated and empty) and observeForEpisode via Turbine. Trivial delegation tests, exactly what was needed. 👍

---

🟠 Still open

1. Migration 134 → 135 — the full-table rebuild (@geekygecko / @sztomek).
The code still performs the unconditional podcast_episodes create-copy-drop-rename. Answering @geekygecko's latest question directly:

If we leave the column for the beta users but remove the ALTER TABLE statement before we ship to production would there be an issue for either of them?

Removing only the ALTER TABLE … ADD COLUMN hls_url (in MIGRATION_131_132) while keeping the 134→135 rebuild does not work cleanly, because it splits the population into two incompatible states that the single rebuild can't both satisfy:

• Prod users (upgrade from < 132): never got the column. The 134→135 INSERT … SELECT lists explicit columns and doesn't reference hls_url, so it would run fine for them — but it's a pointless full-table copy of a potentially huge table just to drop a column they never had.
• Beta users (already at 132 with the column): still need the column dropped, so they still need the rebuild.

So the real choice is between the two options from my last review — and both avoid punishing the entire prod base with a rebuild:

• Option A (drop dead schema): remove the ADD COLUMN from 131→132, and guard the 134→135 rebuild behind a PRAGMA table_info check so it only fires when hls_url is actually present. Prod → guard skips, zero copy. Beta → guard fires, column dropped. Wrinkle: the shipped 132.json still declares hls_url, so MigrationTestHelper tests hopping through v132 need attention.
• Option B (keep dormant column): retain hls_url as a nullable @Ignored/unused field, delete the 134→135 migration entirely. No copy for anyone. Cost: a permanently dead column + retained field.

Either is fine; both beat the unconditional rebuild. If you want, I'm happy to implement whichever you pick.

2. HLS-only video mis-detection — not yet addressed.
DataParser.kt:159 and PodcastResponse.kt:111 both do:

fileType = if (url.isNullOrBlank()) enclosures.firstHlsMimeType() ?: fileType else fileType

This overwrites a real file_type (e.g. video/mp4) with the HLS MIME whenever url is blank. Since isVideo = fileType?.startsWith("video/"), an HLS-only video episode now reports isVideo == false / isAudio == true. Prefer the real type when it's already a video/*:

fileType = if (url.isNullOrBlank() && !fileType.orEmpty().startsWith("video/")) {
    enclosures.firstHlsMimeType() ?: fileType
} else {
    fileType
}

Fix this →

Also note: in PodcastResponse.EpisodeInfo, url is declared non-null (val url: String), so url.isNullOrBlank() is really just an isBlank() check — worth confirming the server sends "" (not null) for HLS-only episodes here, otherwise Moshi parsing of the whole episode fails before this line is reached.

🟡 Minor

• AlternateEnclosureManager.observeForEpisode is now tested but still has no production caller (only findForEpisode is used, in PlaybackManager:370 and :2583). Fine to keep if a consumer is imminent; otherwise consider dropping until needed.

---

Review feedback only — no code changed. Ping me to implement the migration option you settle on and/or the video-detection fix.
• branch feat/hls-stream-resolution

[claude]

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

---

Re-review of PR #5497 — HLS stream url selection rework

Re-reviewed the latest push (b376020 Add tests). That commit adds AlternateEnclosureManagerImplTest, which clears the Danger unit-test check. Status of the remaining open items below.

• Inspect new commits since last review
• Check status of open feedback (migration debate, HLS-only video, unused API, Danger test)
• Post review feedback

---

✅ Resolved

• Danger: missing AlternateEnclosureManagerImpl test — AlternateEnclosureManagerImplTest now covers findForEpisode (populated + empty) and observeForEpisode. Check should clear. 👍

---

🟠 Migration 134 → 135 — answering @geekygecko's question directly

If we leave the column for the beta users but remove the ALTER TABLE statement before we ship to production would there be an issue for either of them?

Yes — that specific combination breaks beta, and it's worth being precise about why, because it's the crux of the whole debate:

Room validates the entity's expected schema against the live DB schema on open (TableInfo.equals, which compares the column maps). The current PR drops hls_url from the entity. So:

• Prod users (upgrade from < 132, never had the column): live table has no hls_url, entity doesn't declare it → ✅ match.
• Beta users (already at v132, column present): live table has an extra hls_url the entity no longer expects → ❌ TableInfo mismatch → crash on startup.

So "leave the column for beta, drop the ALTER TABLE for prod" can't work as-is: the leftover column has to be removed from beta somehow, or those users crash. That leaves the two consistent options:

Option A — drop the column only for users who have it (no full rebuild for prod).
Remove the ALTER TABLE … ADD COLUMN hls_url from MIGRATION_131_132, and guard the 134→135 rebuild so it only runs when the column is actually present:

val hasHlsColumn = database.query("PRAGMA table_info(`podcast_episodes`)").use { c ->
    val nameIdx = c.getColumnIndex("name")
    generateSequence { if (c.moveToNext()) c.getString(nameIdx) else null }.any { it == "hls_url" }
}
if (hasHlsColumn) { /* existing create_new / copy / drop / rename / indices */ }

Prod (no column) → guard skips → zero table copy. Beta (has column) → guard fires → rebuild drops it. Caveat: the shipped 132.json still declares hls_url, so MigrationTestHelper tests that hop through v132 need attention.

Option B — keep the column everywhere, no 134→135 migration at all.
Keep hls_url as a nullable, unused column and keep the corresponding field declared on PodcastEpisode (so Room's expected schema still includes it), just stop reading/writing it in the domain layer. Note the important detail that makes this cheap: ALTER TABLE … ADD COLUMN in SQLite is an O(1) metadata operation — it does not rewrite existing rows. So the 131→132 ADD COLUMN you already ship is not the expensive part; the only expensive thing in this PR is the unconditional full-table rebuild in 134→135, which Option B removes entirely. Cost is a permanently dormant column + a retained entity field.

Both avoid the unconditional podcast_episodes rebuild, which is the right instinct (@geekygecko) — that rebuild can be very expensive for heavy users. Option B is lowest-risk (no migration runs for anyone) if you can live with a dead column; Option A is cleaner schema at the cost of the 132-schema test cleanup. Happy to implement whichever you land on.

🟡 Still open

• HLS-only video mis-detection — unchanged. Both DataParser.kt:159 and PodcastResponse.kt:111 do fileType = if (url.isNullOrBlank()) enclosures.firstHlsMimeType() ?: fileType else fileType. When url is blank this overwrites a real video/* file_type with the HLS MIME (application/x-mpegURL), so isVideo (fileType?.startsWith("video/")) flips to false / isAudio to true for an HLS-only video episode. If HLS-only video is possible, only substitute when the existing type isn't already video/*. Fix this →
• AlternateEnclosureManager.observeForEpisode — now unit-tested, but still not consumed anywhere in production (PlaybackManager only uses findForEpisode). Fine to keep if it's a near-term hook; otherwise drop until needed.

---

Review feedback only — no code changed. The migration decision is yours/@geekygecko's to make; I can implement Option A or B on request.
• branch feat/hls-stream-resolution

[claude]

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

---

Re-review of PR #5497 — HLS stream url selection rework

Reviewed the latest push (1232e21 Address PR comment).

• Inspect new commit since last review
• Confirm HLS-only video fix is correct
• Confirm observeForEpisode removal is clean
• Check migration debate status
• Post review feedback

---

✅ Resolved in this commit

• HLS-only video mis-detection — now fixed correctly in both parsers. DataParser.kt:149 and PodcastResponse.kt:107 guard the substitution with fileType?.startsWith("video/") != true, so a real video/* file_type is preserved and isVideo stays true for HLS-only video episodes. The new EpisodeInfoTest case (hls-only video episode keeps its video file type) covers it. 👍
• Unused observeForEpisode — dropped end-to-end (DAO, AlternateEnclosureManager interface + impl, and the Turbine test). No dead production API left; findForEpisode is the only surface, and it's consumed in PlaybackManager. Clean. 👍

---

🟠 Still the one open item — Migration 134 → 135 (@geekygecko / @sztomek)

The code still performs the unconditional podcast_episodes create-copy-drop-rename (AppDatabase.kt:1490-1504), so @geekygecko's concern about a full-table rebuild on a potentially huge table is unaddressed. This is a product/risk decision for you two, but to restate the two options that actually work (recapping so the thread has it in one place):

• Option A — drop dead schema, no rebuild for prod. Remove the ALTER TABLE … ADD COLUMN hls_url from MIGRATION_131_132, and guard the 134→135 rebuild behind a PRAGMA table_info check so it only fires when hls_url is present. Prod (never had the column) → guard skips → zero copy. Beta (v132, column present) → guard fires → column dropped. Wrinkle: the shipped 132.json still declares hls_url, so MigrationTestHelper tests hopping through v132 need attention.
• Option B — keep a dormant column, delete 134→135 entirely. Retain hls_url as a nullable, unused column and keep the field on the PodcastEpisode entity (so Room's expected schema still matches the live DB), just stop reading/writing it in the domain layer. No migration runs for anyone. Note ALTER TABLE … ADD COLUMN is an O(1) metadata op in SQLite, so the 131→132 add is not the expensive part — the unconditional rebuild is. Cost: a permanently dead column + retained field.

Both avoid punishing the entire prod base with a rebuild. B is lowest-risk if you can live with a dead column; A is cleaner schema at the cost of the 132-test cleanup. Happy to implement whichever you land on.

🟡 Minor (non-blocking)

• In EpisodeInfo, url is declared non-null (val url: String, PodcastResponse.kt:92), so url.isNullOrBlank() is effectively just url.isBlank(). Harmless and arguably future-proof, but worth confirming the server sends "" (not null) for HLS-only episodes — if it can send null, Moshi fails parsing the whole episode before this line, and the field should be String?. (The new test uses "url": "", which matches the isBlank assumption.)

---

Review feedback only — no code changed. The migration decision is yours/@geekygecko's; ping me to implement Option A or B.
• branch feat/hls-stream-resolution

[claude]

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

---

Re-review of PR #5497 — HLS stream url selection rework

Reviewed the latest push (b9e5313 Drop db version 135 and keep episode.hls_url column as dormant). This resolves the long-running migration debate by landing Option B, and it's implemented correctly.

• Inspect the latest commit
• Verify Option B (dormant column) is implemented correctly and safely
• Confirm entity/schema consistency (Room validation)
• Check remaining open items
• Post review feedback

---

✅ Migration decision — Option B, correctly done

The full-table 134→135 rebuild is gone and the hls_url column is retained as a dormant column. I traced the whole chain and it's internally consistent — no crash risk, no expensive rebuild:

• version = 134 (AppDatabase.kt) matches the shipped 134.json, which still declares hls_url (unchanged from main — confirmed the schemas/ diff vs origin/main is empty). So Room's entity-vs-exported-schema check passes at compile time.
• PodcastEpisode re-declares @ColumnInfo("hls_url") var hlsUrl and BaseEpisode exposes val hlsUrl get() = null (default), so UserEpisode inherits cleanly without its own override. Entity ↔ live-DB TableInfo now match for both cohorts:
  • Prod (upgrade from < 132): MIGRATION_131_132's ALTER TABLE … ADD COLUMN hls_url still runs — an O(1) metadata op, not a row rewrite — so the column is present and matches. ✅
  • Beta (already at 132+, column present): column stays, matches. No crash. ✅
• MIGRATION_134_135 removed from both its definition and the addMigrations(...) list; AppDatabaseTest drops it too. No dangling references (grep for 134_135 / 135 is clean).
• PlaylistDao correctly re-adds podcast_episode.hls_url AS p_hls_url — needed because the @Embedded(prefix = "p_") PodcastEpisode now expects that column in the manual projection. I checked and PlaylistDao is the only DAO using the manual p_ projection, so nothing else needs updating.
• hlsUrl is genuinely dormant: the only hlsUrl references in production are the entity declarations and a local variable in PlaybackManager.applyStreamOverride (line 371) — nothing reads the entity property. The playable stream is resolved from episode_alternate_enclosures via findForEpisode(...).firstHlsStreamUrl(). 👍

This is the lowest-risk path and avoids punishing the entire prod base with a rebuild — the right call.

✅ Previously-open items, all resolved

• HLS-only video mis-detection — guarded in both parsers (DataParser.kt:150, PodcastResponse.kt:107): url.isNullOrBlank() && fileType?.startsWith("video/") != true, so a real video/* type is preserved.
• Unused observeForEpisode — dropped end-to-end.
• isHlsOnly regression / prefetch race / dead video API / defaultHlsStreamUrl — all addressed in earlier commits.

🟡 Minor (non-blocking)

• Document the dormancy. PodcastEpisode.hlsUrl / BaseEpisode.hlsUrl now exist solely to keep Room's schema aligned with the shipped 134.json — they're intentionally never read. A one-line comment on each (e.g. "Dormant: retained only so the entity matches the shipped v134 schema; the playable HLS stream comes from episode_alternate_enclosures") would save the next reader from assuming it's live and wiring it back in. Add note →
• EpisodeInfo.url non-null — still declared val url: String, so url.isNullOrBlank() is effectively isBlank(). Fine, but worth confirming the server sends "" (not null) for HLS-only episodes here; if it can send null, Moshi fails the whole episode before this line and the field should be String?. The test uses "url": "", which matches the assumption.

---

Nice resolution — the migration approach is sound and the diff is clean. LGTM aside from the two minor nits above.

Review feedback only — no code changed.
• branch feat/hls-stream-resolution