[TV] Home parity: banners, categories and sponsored podcasts - #5730
Conversation
Generated by 🚫 Danger |
sztomek
changed the title
0e2bd23 to
e3b2a38
Compare
sztomek
changed the title
|
@claude please review this |
|
Claude finished @sztomek's task in 4m 33s —— View job Code review
Overall this is clean, well-factored work — the row/overlay patterns match what's already in the TV module ( 🔴
|
sztomek
changed the title
4cdc38e to
eb644a0
Compare
74ab725 to
f1b6b25
Compare
|
Claude finished @sztomek's task in 3m 7s —— View job Re-review after
|
| Finding | Fix |
|---|---|
| 🔴 duplicate uuid → LazyRow duplicate-key crash | insertSponsored now filters the pre-existing copy out before inserting (TvDiscoverFeedLoader.kt:159-160), plus the sponsored podcast already present in the row is not duplicated test |
| 🟠 sponsored ad landing at slot 0 in short categories | minOf(SPONSORED_CATEGORY_POSITION, rest.size) (:83), plus the …appends the sponsored ad when the category is short test |
| 🟡 auth filter missing on category sponsored rows | && (isLoggedIn || it.authenticated != true) (:73) |
| unchecked cast | as? DiscoverCategory ?: this (:220) |
| file/type name mismatch | renamed to TvOpenedCategory.kt |
includeHomeSections = false untested |
banner rows are excluded from the search discover feed in TvSearchViewModelTest |
| banner text overlapping artwork | Row constrained to fillMaxWidth(0.82f / 0.68f) |
| sponsored label padding | bottom = 9.dp, matching the artwork's vertical = 9.dp |
🟡 New: unused string added to localization
tv_search_searching ("Searching…") is added in strings.xml:260 but has no references anywhere in the repo. Unused strings still go out to GlotPress for translation — worth removing unless a follow-up is about to use it. (inline) · Fix this →
🟡 New: banner text can be squeezed out by long translations
TvBannerRow.kt:92-112 — the CTA pill and the title/subtitle Column are both un-weighted children of the Row, so the pill is measured first and takes whatever width it needs; the Column gets only the remainder of the 68%/82%. With a long localised CTA ("Create a free account" grows ~30-40% in de/fr/ru) the pill plus the fixed 48dp spacer can consume the constrained width and the title/subtitle get measured at zero width — they vanish rather than ellipsise. Weighting both children (weight(1f, fill = false) on the pill, weight(1f) on the Column) makes the split proportional and bounded. (inline) · Fix this →
Carry-overs (not blocking, your call)
- Category open still costs an extra discover-feed request.
loadCategorySponsoredPodcasts(:62) callslistRepository.getHomeDiscoverFeed(isLoggedIn), andListRepositorydoes no caching — it goes straight tolistWebService.getLoggedIn/OutDiscoverFeed. Since it'sawaited alongside the podcast list (:57), it gates the category screen's first paint on a second full feed fetch. Caching the last-loadedDiscover(or just its sponsored rows) inTvDiscoverFeedLoaderwould remove it. - Search-tab category pills are still
onClick = {}(TvSearchScreen.kt:257) while the visually identical Home pills now open a grid.TvCategoryPodcastsScreenexists now, so wiring it is small — or a TODO / note in the description if it's a deliberate follow-up. TvDiscoverRow.Banner.titleis still populated fromrow.titleand never rendered (the UI uses string resources). Correct for localisation, but the field is dead weight; a one-line comment would save the next reader the lookup.- Minor, unchanged:
loadSponsoredInsertionspassesrow.authenticatedwhere the phone (DiscoverViewModel.loadCarouselSponsoredPodcasts) passesfalseunconditionally — worth confirming that's intentional; and the CHANGELOG checkbox is still unticked.
Focus handling for the new nested overlay (categoryRestoreTrigger vs restoreFocusTrigger in TvHomeScreen.kt:96-119) reads correctly to me: closing a podcast opened from a category restores the grid, closing one opened from Home restores the row.
I could not run Gradle in this environment (the sandbox blocks it), so the build, spotlessCheck and the unit tests are unverified here — findings are from reading the diff and surrounding TV/discover code.
· branch feat/tv-home-discover-parity
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| <string name="tv_banner_create_account_title">One account. Every screen</string> | ||
| <string name="tv_banner_create_account_subtitle">Your follows and progress exactly where you left them</string> | ||
| <string name="tv_banner_create_account_action_title">Create a free account</string> | ||
| <string name="tv_banner_discover_more_title">You haven\'t seen the half of it</string> |
There was a problem hiding this comment.
tv_search_searching has no references anywhere in the repo (grep -rn "tv_search_searching" only hits this file). Unused strings still get shipped to GlotPress and translated, so it's worth dropping it unless a follow-up PR is about to use it.
| Row( | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| modifier = Modifier | ||
| .fillMaxHeight() | ||
| .fillMaxWidth(if (banner.hasGradient) 0.82f else 0.68f) | ||
| .padding(horizontal = 48.dp), | ||
| ) { | ||
| Box( | ||
| contentAlignment = Alignment.Center, | ||
| modifier = Modifier | ||
| .clip(RoundedCornerShape(percent = 50)) | ||
| .background(if (isFocused) MaterialTheme.tvColors.backgroundActive else MaterialTheme.tvColors.backgroundActive20) | ||
| .padding(horizontal = 24.dp, vertical = 12.dp), | ||
| ) { | ||
| Text( | ||
| text = banner.actionTitle(), | ||
| style = MaterialTheme.tvTypography.caption1, | ||
| color = if (isFocused) MaterialTheme.tvColors.textPrimaryActive else MaterialTheme.tvColors.backgroundActive, | ||
| ) | ||
| } | ||
| Spacer(modifier = Modifier.width(48.dp)) |
There was a problem hiding this comment.
This fixes the overlap, but the two children of the Row are both un-weighted, so the CTA pill is measured first and takes whatever it wants; the text Column only gets what's left of the 68%/82%. All three banner strings are localised, and "Create a free account" is one of the longer-growing kinds of string (de/fr/ru commonly +30-40%) — with a long enough translation the pill plus the fixed 48dp spacer can consume the whole constrained width and the title/subtitle get measured at zero width, i.e. silently disappear rather than ellipsise.
Giving both children weights makes the split proportional and bounded regardless of translation length, e.g. Modifier.weight(1f, fill = false) on the pill Box and Modifier.weight(1f) on the Column. A short comment on where 0.82f/0.68f come from (gradient stop / artwork start) would also help the next reader.
|
Claude finished @sztomek's task in 2m 37s —— View job Re-review after
|
| Item | Fix |
|---|---|
| Banner text overlapping artwork (geekygecko) | CreateAccount now puts artwork at CenterStart with the text Row aligned CenterEnd at 0.72f, on a black card; asset re-exported with the fade baked in (TvBannerRow.kt:67-110) |
| Sponsored label vertical padding (geekygecko) | bottom = 9.dp matching the artwork's vertical = 9.dp, and now caption2 (TvPodcastTile.kt:50-52) |
| Banner layout duplication | Extracted BannerText / BannerActionPill — good call, the two orderings read much better than the previous inline block |
🟡 New: BackgroundLift() at line 69 is dead paint
TvBannerRow.kt:69 — the first BackgroundLift() is never visible in either branch:
CreateAccount:tv_banner_create_account.webpis lossy VP8 with no alpha channel (2796×1224, fully opaque). AtrequiredHeight(240.dp)+FillHeightit draws ~548dp wide on a ~860dp banner ≈ 63% of the width, alignedCenterStart. The lift spans0f → 0.55f— entirely behind opaque pixels.DiscoverMore: the mask below paints opaque black from0fto0.82f, covering the whole lift, and thenBackgroundLift()is drawn again on top at line 92 — that second one is the visible one.
So only line 92 has any effect; line 69 is a full-size gradient draw per frame for nothing. Drop it, or move it above the Image if it was meant to lift the area under the artwork. (inline) · Fix this →
Related: CreateAccount has hasArtworkMask = false, so text legibility depends entirely on the fade baked into the re-exported asset. A one-line comment on hasArtworkMask saying that would stop a future asset re-crop silently reintroducing the overlap geekygecko caught.
🟡 Long-translation squeeze is only half-fixed
TvBannerRow.kt:103-109 — BannerText now has weight(1f, fill = false), but BannerActionPill is still un-weighted. In a Row, un-weighted children are measured first against the full remaining width, so a long localised "Create a free account" (de/fr/ru commonly +30-40%) can still take the whole 72%/82% and leave the weighted text Column at ~0dp — the pill's Text has no maxLines, so it wraps to two or three lines rather than yielding. Adding Modifier.weight(1f, fill = false) to the pill's Box too makes the split proportional and bounded either way. (Arrangement.spacedBy(48.dp) replacing the fixed Spacer is a genuine improvement — that 48dp is now deducted before distribution.)
Smaller
containerColor/focusedContainerColorare both hardcodedColor.Black(:59-60), so the banner is the one TV surface that bypassestvColorsand focus no longer changes the card colour — it's conveyed by the 1.05× scale and the pill only. Presumably deliberate to match the artwork's baked-in black; worth a comment if so, since the next person theming the TV app will read it as an oversight.
Carry-overs from the previous review (unchanged, your call)
tv_search_searchingis still unused — added atstrings.xml:260by this PR, no references anywhere in the repo. It'll be sent to GlotPress and translated regardless.- Category open still costs an extra discover-feed request (
TvDiscoverFeedLoader.kt:62),awaited alongside the podcast list, so it gates first paint. - Search-tab category pills are still
onClick = {}(TvSearchScreen.kt:257) while the identical Home pills now open a grid. TvDiscoverRow.Banner.titleis populated fromrow.titleand never rendered.loadSponsoredInsertionspassesrow.authenticatedwhere the phone passesfalseunconditionally; and the CHANGELOG checkbox is still unticked.
Nothing here blocks merge — the dead BackgroundLift() is a one-line delete and the rest are judgement calls. Gradle is blocked in this environment, so the build, spotlessCheck and the unit tests are unverified here; findings are from reading the diff, the asset headers, and the surrounding TV code.
· branch feat/tv-home-discover-parity
| ) { | ||
| val artworkAlignment = if (banner.artworkAtStart) Alignment.CenterStart else Alignment.CenterEnd | ||
| Box(modifier = Modifier.fillMaxSize().clipToBounds()) { | ||
| BackgroundLift() |
There was a problem hiding this comment.
This first BackgroundLift() never ends up visible in either branch, so it's a full-size gradient draw per frame for nothing:
CreateAccount— the artwork is alignedCenterStartandtv_banner_create_account.webpis lossy VP8 with no alpha channel (fully opaque), drawn ~548dp wide (240dp × 2796/1224) on a ~860dp banner ≈ 63% of the width. The lift only spans0f → 0.55f, so it's entirely behind opaque pixels.DiscoverMore— the mask below paints opaque black from0fto0.82f, which covers the whole0f → 0.55flift, and thenBackgroundLift()is drawn again on top at line 92 (that one is the visible one).
So the effect only comes from line 92. Either drop line 69, or if it was meant to lift the area under a semi-transparent artwork, it needs to move above the Image for the CreateAccount case.
Related: CreateAccount has hasArtworkMask = false, so text legibility on the left→right transition depends entirely on the fade baked into the asset (which is what the re-export in 6c27451 did). Worth a comment on hasArtworkMask saying so — otherwise a future asset re-crop silently puts title text over the collage again.
6c27451 to
ce416b5
Compare
|
Claude finished @sztomek's task in 4m 12s —— View job Re-review after
|
|
|
||
| @DrawableRes | ||
| private fun TvDiscoverBanner.artwork(): Int = when (this) { | ||
| TvDiscoverBanner.CreateAccount -> IR.drawable.tv_banner_create_account |
There was a problem hiding this comment.
The tv_banner_create_account.webp re-exported in ce416b5 is horizontally mirrored. Everything inside the phone mockup reads backwards:
- the screen labels — "Good Times", "Play all", "Smart Rules", "Search", "24 episodes · 19h 48m", and the status bar
10:41/LTE - the play glyph in the "Play all" button points left (
◁) - the podcast covers in the grid (GOOD HANG, Bald & Beautiful, Dish, The Book Review, Waitrose) are all flipped
- the phone's volume/action buttons sit on the right-hand edge instead of the left
It isn't the renderer: tv_banner_discover_more.webp in the same folder reads correctly. Given the commit moved the artwork to the end edge and the file grew 29,858 → 84,958 bytes, my guess is the frame was flipped to get the phone onto the right rather than re-composed.
At requiredHeight(150.dp) the artwork draws ~343dp wide (2796×1224 → 2.28:1), of which the phone is roughly the right 155dp — small, but the reversed "Play all" and the backwards cover art are visible at TV viewing distance on a 1080p panel. Worth re-exporting with the mockup composed on the right rather than mirrored.
| Box(modifier = Modifier.fillMaxSize().clipToBounds()) { | ||
| BackgroundLift() | ||
| Image( | ||
| painter = painterResource(banner.artwork()), | ||
| contentDescription = null, | ||
| contentScale = ContentScale.FillHeight, | ||
| alignment = Alignment.CenterEnd, | ||
| modifier = Modifier | ||
| .align(Alignment.CenterEnd) | ||
| .requiredHeight(banner.artworkHeight), | ||
| ) | ||
| if (banner.hasArtworkMask) { | ||
| // Opaque black over the text side so the bright collage only shows on the end edge. | ||
| Box( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .background( | ||
| Brush.horizontalGradient( | ||
| 0f to Color.Black, | ||
| 0.82f to Color.Black, | ||
| 1f to Color.Transparent, | ||
| ), | ||
| ), | ||
| ) | ||
| BackgroundLift() | ||
| } |
There was a problem hiding this comment.
Moving the artwork onto black fixed the case I flagged last round — with the image at CenterEnd this first BackgroundLift() is now genuinely visible in the CreateAccount branch (the artwork covers roughly 0.60f → 1f of an ~864dp banner, and the lift is transparent past 0.55f).
It's still redundant in the DiscoverMore branch though: the mask below paints opaque black across 0f → 0.82f, which fully covers the 0f → 0.55f lift, and then line 92 repaints it on top. So that branch does two full-size gradient draws per frame where one suffices. Hoisting a single call below the if gives the same result in both branches — for CreateAccount the lift would then paint over the artwork, but it's already fully transparent by the point the artwork starts:
| Box(modifier = Modifier.fillMaxSize().clipToBounds()) { | |
| BackgroundLift() | |
| Image( | |
| painter = painterResource(banner.artwork()), | |
| contentDescription = null, | |
| contentScale = ContentScale.FillHeight, | |
| alignment = Alignment.CenterEnd, | |
| modifier = Modifier | |
| .align(Alignment.CenterEnd) | |
| .requiredHeight(banner.artworkHeight), | |
| ) | |
| if (banner.hasArtworkMask) { | |
| // Opaque black over the text side so the bright collage only shows on the end edge. | |
| Box( | |
| modifier = Modifier | |
| .fillMaxSize() | |
| .background( | |
| Brush.horizontalGradient( | |
| 0f to Color.Black, | |
| 0.82f to Color.Black, | |
| 1f to Color.Transparent, | |
| ), | |
| ), | |
| ) | |
| BackgroundLift() | |
| } | |
| Box(modifier = Modifier.fillMaxSize().clipToBounds()) { | |
| Image( | |
| painter = painterResource(banner.artwork()), | |
| contentDescription = null, | |
| contentScale = ContentScale.FillHeight, | |
| alignment = Alignment.CenterEnd, | |
| modifier = Modifier | |
| .align(Alignment.CenterEnd) | |
| .requiredHeight(banner.artworkHeight), | |
| ) | |
| if (banner.hasArtworkMask) { | |
| // Opaque black over the text side so the bright collage only shows on the end edge. | |
| Box( | |
| modifier = Modifier | |
| .fillMaxSize() | |
| .background( | |
| Brush.horizontalGradient( | |
| 0f to Color.Black, | |
| 0.82f to Color.Black, | |
| 1f to Color.Transparent, | |
| ), | |
| ), | |
| ) | |
| } | |
| BackgroundLift() |
Separately, worth a one-line comment on hasArtworkMask (line 179) noting that CreateAccount needs no mask because its artwork is composited on opaque black — otherwise a future re-export with a transparent or light background silently puts the title text back over the mockup, which is the overlap @geekygecko caught. Same for the hardcoded Color.Black container at lines 60-61: it's the one TV surface bypassing tvColors, and the reason (both assets are baked on black) isn't obvious from the code.
| Row( | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| horizontalArrangement = Arrangement.spacedBy(48.dp), | ||
| modifier = Modifier | ||
| .align(Alignment.CenterStart) | ||
| .fillMaxHeight() | ||
| .fillMaxWidth(banner.contentWidthFraction) | ||
| .padding(horizontal = 48.dp), | ||
| ) { | ||
| BannerActionPill(banner, isFocused) | ||
| BannerText(banner, modifier = Modifier.weight(1f, fill = false)) | ||
| } |
There was a problem hiding this comment.
Arrangement.spacedBy(48.dp) replacing the fixed Spacer is a real improvement — the 48dp now comes off the top before distribution. But the weighting is still one-sided: BannerText is weighted, BannerActionPill isn't, and in a Row un-weighted children are measured first against the full remaining width. So a long localised CTA can still take the whole 0.68f/0.82f and leave the text Column at ~0dp. The pill's Text (line 153) also has no maxLines, so instead of yielding it wraps to two or three lines and grows the pill vertically inside the 132dp card.
Weighting both bounds it either way, e.g. Modifier.weight(1f, fill = false) on the pill Box and Modifier.weight(2f) on the text — the asymmetric weights keep the text the bigger share while capping the pill at a third, since fill = false means the leftover isn't redistributed. maxLines = 1 + TextOverflow.Ellipsis on the pill's Text would make the failure mode graceful rather than a reflowing pill.
"Create a free account" is the risky one — de/fr/ru commonly run 30-40% longer.
ce416b5 to
f4ee971
Compare
|
Claude finished @sztomek's task in 2m 46s —— View job Re-review after
|
3 participants
Description
Closes feature-parity gaps between the Apple TV and Android TV apps on the Home feed.
What this PR adds
Banner row (iOS
DiscoverRowSection.makeBannerRow+BannerRow.swift) — abanner/inline_bannerfeed item keyed byid(create_account/discover_more). NewTvBannerRow:discover_more→ Search tab,create_account→ create-account flow.Category rows + detail screen (iOS
DiscoverCategoriesRow+DiscoverPodcastsListView) — the Home feed now renders the category pills row, and pills open a newTvCategoryPodcastsScreen("Most Popular in {category}", 6-column grid matching iOS) via the establishedTvDetailOverlayfocus-stacking pattern, reusingTvPodcastGridScaffold/TvPodcastTile.Sponsored parity (tvOS has no dedicated ad row — sponsorship is per-podcast).
TvDiscoverFeedLoadernow:single_podcastrow to "Pocket Casts recommends".sponsored_podcastsinto normal lists at their server positions.category_id-tagged rows) into the category page at slot 5.TvPodcastTile/TvSinglePodcastTile/TvFeaturedTilerender the "Sponsored" label (matching iOSDiscoverPodcastCell/DiscoverSinglePodcastCell).Data model: new
TvDiscoverRow.Banner/.Categories,TvDiscoverBannerenum,TvCategorySelectionsaver, and region-token resolution for category sources. No new components were invented where an existing one fit. No analytics (separate workstream); the callbacks are structured for later hookup.Notes on the live feed
category_id-tagged, so they surface only on their category pages (Business / Kids & Family / Music) — matching iOS, which filterscategoryID != nilout of the home feed.listShowAllTapped/ "show all" is phone-only, not a TV parity gap.Fixes PCDROID-722 https://linear.app/a8c/issue/PCDROID-722/discover-banner
Figma: Ftk3KwnfqaK4g57yCN63p0-fi-2373_1041
Testing Instructions
create_accountbanner it renders with artwork near the top; pressing it opens create-account.discover_morebanner (artwork + gradient) routes to Search; focus it to see the 1.05× scale and CTA colour change.Screenshots or Screencast
Screen_recording_20260812_204515.mp4
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xmlI have tested any UI changes...