[TV] Load auth-specific discover feeds on TV home - #5722
Conversation
|
Claude finished @sztomek's task in 3m 31s —— View job Code review
The change is small and well-scoped, and the PR body does a good job documenting the trade-offs. Endpoint selection is sound: Findings1. The new 2.
You explicitly rejected nullable, which is reasonable. A cheaper middle ground within this PR's scope: filter blank sources once in Two smaller notes on the same change: Moshi applies defaults only for an absent key — an explicit 3. Neither behaviour change is tested — The test diff is a mock rename. Endpoint selection is implicitly covered (an unstubbed non-null suspend returns Nits
No security or performance concerns — both endpoints are unauthenticated static JSON on the same client, and the feed is fetched once per login-state change. --- · Branch: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| private suspend fun loadRow(row: DiscoverRow): TvDiscoverRow? { | ||
| if (row.source.isBlank()) return null |
There was a problem hiding this comment.
This guard covers loadRow, but loadCategories (line 40) reads source off the layout without the same check:
val source = discover.layout.firstOrNull { it.type is ListType.Categories }?.source ?: return emptyList()The elvis only handles "no categories row". Now that source defaults to "", a categories row missing the key falls through as a blank string into listRepository.getCategoriesList(""), and Retrofit's @Url resolves "" against the base URL — i.e. a GET https://static.pocketcasts.com/. The surrounding try/catch means it degrades to emptyList() rather than crashing, but it's a wasted request and a misleading "Failed to load TV search categories" log instead of a clean no-op.
| if (row.source.isBlank()) return null | |
| if (row.source.isBlank()) { | |
| Timber.d("Skipping discover row without a source: ${row.id ?: row.title}") | |
| return null | |
| } |
...and separately at line 40:
val source = discover.layout.firstOrNull { it.type is ListType.Categories }
?.source?.takeIf(String::isNotBlank) ?: return emptyList()The Timber.d is optional but worth it — right now placeholder rows vanish with no trace, which will be annoying the next time the feed shape changes.
There was a problem hiding this comment.
Both guards are in place. loadRow returns null on row.source.isBlank(), and loadCategories now resolves the source with ?.source?.takeIf(String::isNotBlank) ?: return emptyList(), so a categories row present but missing its source no longer falls through as "" into getCategoriesList("") (which would resolve @Url against the base URL). Both degrade to a clean no-op instead of a wasted request + misleading log.
| @Json(name = "expanded_top_item_label") override val expandedTopItemLabel: String?, | ||
| @Json(name = "title") override val title: String, | ||
| @Json(name = "source") override val source: String, | ||
| @Json(name = "source") override val source: String = "", |
There was a problem hiding this comment.
Two things worth recording here.
1. The default only applies when the key is absent. Moshi's generated adapter uses the default for a missing key, but an explicit "source": null still throws Non-null value 'source' was null. That matches what you describe in the PR body (the keys are omitted), just noting the invariant is narrower than "tolerates missing source".
2. This relaxes a shared invariant that ~18 phone call sites rely on. DiscoverAdapter.kt:806/824/841/877/933/978/1051, CuratedPodcastsCrawler.kt:29, and PodcastGridListFragment.kt:87 all feed row.source straight into a request or a fragment arg. Before this change, a feed row without source failed loudly at parse time; now it silently becomes "" and turns into a request against the base URL that fails with a confusing error further downstream. The TV loader has an explicit blank guard — nothing on phone does.
You explicitly considered nullable and rejected it, which is fair. A cheaper middle ground that keeps this PR's scope: filter blank sources once in transformWithRegion (DiscoverModel.kt:54), so every consumer including phone gets the guarantee, and the TV-side isBlank() check becomes belt-and-braces:
fun List<DiscoverRow>.transformWithRegion(...) = this
.filter { it.regions.contains(region.code) && it.source.isNotBlank() }
.map { it.transformWithReplacements(replacements, resources) }If you'd rather not, a short KDoc on the property explaining that blank means "placeholder row the client fills locally" would at least make the contract discoverable.
FWIW the expandedStyle default on line 63 is a non-issue — ExpandedStyle.fromString already returns PlainList() for unrecognised values (ExpandedStyle.kt:22), so the new default is exactly the existing fallback.
There was a problem hiding this comment.
Went with the KDoc rather than filtering in transformWithRegion. Documented the contract on the source property directly ("Blank when the feed omits the key: a placeholder row the client fills locally. Consumers must skip blank sources."), which makes the invariant discoverable at all ~18 phone call sites.
I deliberately did not add it.source.isNotBlank() to transformWithRegion: that's the shared phone Discover path, and silently dropping a blank-source row there would change phone behaviour in a TV-scoped PR (a blank source on phone currently fails loudly downstream, which for the phone feed is arguably the signal you want, not a silent disappearance). The TV loader owns its own blank-source skip, so the guarantee is enforced where the placeholder rows actually occur. And noted your point 1 — the default only kicks in for an absent key; an explicit "source": null still throws — which the KDoc wording ("omits the key") reflects.
| fun `keep listening row shows first up next episode even when signed out`() = runTest { | ||
| whenever(syncManager.isLoggedIn()).thenReturn(false) | ||
| whenever(listRepository.getDiscoverFeed()).thenReturn(discover()) | ||
| whenever(listRepository.getLoggedOutDiscoverFeed()).thenReturn(discover()) |
There was a problem hiding this comment.
The test diff is a mock rename only, so neither of the two actual behaviour changes in this PR is covered.
One of them is implicitly covered, to be fair: because getLoggedInDiscoverFeed/getLoggedOutDiscoverFeed return a non-null Discover, an unstubbed call returns null and the ViewModel lands in Error, so these tests would fail if the wrong endpoint were picked. Good enough for endpoint selection.
The two gaps that aren't covered:
Blank-source rows are dropped — the new loadRow guard. Trivial with the existing row() helper:
@Test
fun `rows without a source are dropped`() = runTest {
whenever(syncManager.isLoggedIn()).thenReturn(false)
whenever(listRepository.getLoggedOutDiscoverFeed()).thenReturn(
discover(
row(id = "up-next-placeholder",, source = ""),
row(id = "trending",, source = "https://lists/trending.json"),
),
)
whenever(listRepository.getListFeed(eq("https://lists/trending.json"), any()))
.thenReturn(listFeed(podcast("podcast-1")))
createViewModel().uiState.test {
val state = awaitItem() as TvHomeUiState.Ready
assertEquals(listOf("trending"), state.rows.map { it.id })
verify(listRepository, never()).getListFeed(eq(""), any())
}
}The Moshi defaults actually parse — this is the whole reason DiscoverModel changed, and nothing guards it. A regression here (someone re-tightening the field, or the adapter changing) breaks the TV Home feed entirely at runtime with no test failure. There's no existing parse test in modules/services/servers to extend, but a small one asserting that a DiscoverRow JSON with source/expanded_style omitted parses to "" / PlainList would pin the contract that this PR depends on.
There was a problem hiding this comment.
Both gaps are covered now.
- Blank-source rows dropped —
rows without a source are droppedinTvHomeViewModelTestasserts the placeholder row is gone andgetListFeed("")is never called. - Moshi defaults parse — added
DiscoverModelTestinmodules/services/servers(new.../servers/model/test package): it parses aDiscoverRowJSON withsourceandexpanded_styleomitted through the realListType/DisplayStyle/ExpandedStyleadapters and assertssource == ""andexpandedStyle is ExpandedStyle.PlainList. That pins the contract this PR depends on, so re-tightening the field breaks a test instead of the TV Home feed at runtime.
sztomek
changed the title
|
Claude finished @sztomek's task in 1m 37s —— View job Code review
Re-reviewed on What's solid: endpoint selection is race-free in the way that matters. Findings1. The new guard is in val source = discover.layout.firstOrNull { it.type is ListType.Categories }
?.source?.takeIf(String::isNotBlank) ?: return emptyList()2.
You explicitly rejected nullable, which is reasonable. A cheaper middle ground inside this PR's scope — filter once in return this.filter { it.regions.contains(region.code) && it.source.isNotBlank() }
.map { it.transformWithReplacements(replacements, resources) }Failing that, a KDoc noting blank means "placeholder row the client fills locally" would at least make the contract discoverable. Two sub-notes on the same change:
3. Neither behaviour change is tested — The test diff is a mock rename only. Endpoint selection is implicitly covered (an unstubbed non-null suspend returns
Nits
No security or performance concerns: both endpoints are unauthenticated static JSON on the same client, fetched once per login-state change. · Branch: |
cabb1c1 to
78bccff
Compare
937037a to
a9ef5f4
Compare
|
Claude finished @sztomek's task in 1m 43s —— View job Code review
Re-reviewed after
The endpoint wiring is unchanged and still looks right: Remaining1. The Moshi defaults themselves are still unpinned — This is the one gap I'd still close. The new TV test constructs A small test in 2. Unchanged from my last pass, and I recognise you may have consciously decided to leave it. Recording it once more rather than pressing: a source-less row used to fail loudly at parse time and now silently becomes Note the tolerance is narrower than it looks: the default applies only to an absent key. An explicit Nits (all still open, all optional)
No security or performance concerns. I wasn't able to run --- · Branch: |
78bccff to
618a0d8
Compare
a9ef5f4 to
eba2af7
Compare
|
Claude finished @sztomek's task in 2m 52s —— View job Code review
Re-reviewed after Verified
Also confirmed unchanged and still correct: Remaining (all optional)1. The blank- The new comment ("Consumers must skip blank sources") is a real improvement over nothing. Two small ways to get more out of it:
Fine to leave as-is too — phone feeds all send 2. suspend fun load(isLoggedIn: Boolean) = buildRows(listRepository.getHomeDiscoverFeed(isLoggedIn), isLoggedIn)3. 4. Dropped rows still vanish silently ( No security or performance concerns: both endpoints are unauthenticated static JSON on the same client, fetched once per login-state change. Caveat: gradle isn't in my allowed tools in this environment, so I couldn't run --- · Branch: |
618a0d8 to
add79fb
Compare
eba2af7 to
14ede72
Compare
|
Claude finished @sztomek's task in 1m 53s —— View job Code review
Re-reviewed after the force-push to Verified this pass
Remaining (all optional, all pre-existing nits)
No security or performance concerns: both endpoints are unauthenticated static JSON on the same client, fetched once per login-state change. Caveat: gradle isn't in my allowed tools in this environment, so I could not execute --- · Branch: |
add79fb to
a8a6f63
Compare
14ede72 to
425f5e8
Compare
|
Claude finished @sztomek's task in 1m 54s —— View job Code review
Re-reviewed after the force-push to Verified on the new base
Remaining (all optional, all carried over)
No security or performance concerns: both endpoints are unauthenticated static JSON on the same client, fetched once per login-state change. Caveat: I attempted |
2 participants
Description
Stacked on #5720. Switches the TV Home screen to Apple TV's auth-specific discover feeds.
Apple TV (
DiscoverServerHandler.DiscoverType) serves a dedicated feed per sign-in state;HomeViewswitches onuserState.isLoggedIn. Android Home used a single genericcontent_v3.json. This aligns it:https://static.pocketcasts.com/discover/tv/content_v3_logged_in.jsonhttps://static.pocketcasts.com/discover/tv/content_v3_logged_out.json(Staging/debug:
static.pocketcasts.net.)TvDiscoverFeedLoader.load(isLoggedIn)now picks the feed fromsyncManager.isLoggedIn(), matching iOS.Model change (why it's needed)
The logged-in/out feeds contain rows that omit the
sourceandexpanded_stylekeys — the empty "Up Next"/"New Releases" placeholders (which iOS fills from local data) andbannerrows. Android'sDiscoverRow.source/expandedStylewere non-null with no default, so Moshi threw on those rows and the whole feed failed to parse. They now default (source = "",expandedStyle = ExpandedStyle.PlainList()) when the key is absent. Existing feeds always send both keys, so behavior there is unchanged; this avoided makingsourcenullable across the shared model (which the phone Discover feature relies on).ListRepository.getDiscoverFeed()(→content_v3.json) stays for the phone Discover feature.Rendering behavior (known divergence from iOS)
Android keeps building its own local rows (Keep Listening / Up Next / New Releases) and prepending them; the feed's blank-
sourceplaceholder rows andbannerrows are dropped (if (row.source.isBlank()) return null+ unknown type → null). So the row set/order differs from iOS's inline-fill, even though the feed file now matches. Full inline-fill + banner support would be a further follow-up.Fixes PCDROID-717 https://linear.app/a8c/issue/PCDROID-717/use-appropriate-feed-depending-on-signed-in-state
Testing Instructions
./gradlew :tv:installDebug, open the TV app on the Home tab.adb logcat | grep content_v3:content_v3_logged_in.jsoncontent_v3_logged_out.jsonChecklist
./gradlew spotlessApply)