feat: Podcasts Listing Page#3576
Conversation
OpenAPI ChangesNo changes detected Unexpected changes? Ensure your branch is up-to-date with |
ahtesham-quraish
force-pushed
the
ahtesham/podcast-listing-page-branch
branch
from
6ef8dcd to
bce6cb7
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a new /podcasts landing page in the Next.js App Router to give users a central place to browse MIT podcasts (latest episodes, featured series, and more series), with navigation into the existing search and podcast detail pages.
Changes:
- Introduces a new Podcasts Listing Page client component with episode list + series list + inline audio player.
- Adds search URL helpers for podcast/podcast-episode listings and a new
/podcastsroute constant. - Adds a new public env var (validated at startup) for selecting a featured podcasts learning-path list.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| frontends/main/validateEnv.js | Adds validation for a new public env var used by the podcasts listing page. |
| frontends/main/src/common/urls.ts | Adds podcast/podcast-episode search URLs and a /podcasts route constant. |
| frontends/main/src/app/(site)/podcasts/page.tsx | Adds the App Router route for /podcasts. |
| frontends/main/src/app-pages/PodcastPage/PodcastsListingPage.tsx | Implements the new podcasts landing page UI, data fetching, and player integration. |
| const { data: episodesData } = useLearningResourcesList({ | ||
| resource_type: [ResourceTypeEnum.PodcastEpisode], | ||
| sortby: LearningResourcesListSortbyEnum.LastModified, | ||
| limit: episodesLimit, | ||
| }) |
| const { data: seriesData } = useLearningResourcesList({ | ||
| resource_type: [ResourceTypeEnum.Podcast], | ||
| sortby: LearningResourcesListSortbyEnum.LastModified, | ||
| limit: seriesLimit, | ||
| }) |
| <NowPlayingMeta variant="subtitle1"> | ||
| Chalk Radio • OCW | ||
| </NowPlayingMeta> |
| <SectionHeader> | ||
| <SectionTitle variant="subtitle1">Latest Episodes</SectionTitle> | ||
| <SectionLink variant="subtitle1">All episodes</SectionLink> | ||
| </SectionHeader> |
| : "#" | ||
| } |
| export const PodcastsListingPage: React.FC = () => { | ||
| const isMobile = useMediaQuery((theme: Theme) => theme.breakpoints.down("sm")) | ||
| const [playingEpisode, setPlayingEpisode] = useState<LearningResource | null>( |
| const series = seriesData?.results ?? [] | ||
| const totalSeries = seriesData?.count ?? 0 |
There was a problem hiding this comment.
Bug: The featuredSeries and moreSeries lists are not deduplicated, which can cause the same podcast to be displayed twice on the podcast listing page.
Severity: LOW
Suggested Fix
Before rendering, filter the moreSeries list to exclude any podcasts that are already present in the featuredSeries list. This can be achieved by comparing the IDs of the items in both lists and creating a new list of unique items to pass to the PodcastSeriesSection component.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastsListingPage.tsx#L86-L87
Potential issue: The `PodcastsListingPage` component fetches a list of `featuredSeries`
and a separate list of `moreSeries`. These two lists are sourced independently and then
rendered without any deduplication logic. If a podcast is included in the featured
learning path and is also recent enough to be included in the `moreSeries` list (top 5
newest), it will appear twice on the page: once in the "FEATURED" section and again in
the "More Podcasts" section. This creates a confusing user experience and suggests poor
page curation.
ChristopherChudzicki
left a comment
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
Thanks for putting this together. The page is close. I've left some comments to help organize the code plus fix some issues.
My inline comments fall in a few groups:
- Unify the duplicated/triplicated player pieces: a shared player hook, a single
EpisodeItem, and a sharedPodcastPlayermock. (The hook can be extracted now with the current fallback; the podcast-name line swaps to the real title when that lands.) - Accessibility: no real headings on the page (fix here). Additionally, there's some a11y issues in
EpisodeItem, but I think we can fix those in a followup after it's de-duplicated. - Smaller comments:
- factory defaults over hardcoded overrides
- double-appended page title
- "All episodes" should use our Link
lightGray0(blocked on a smoot bump... Would be nice to do that as a separate PR).
- Podcast Title: My comment at :109. Figma shows each card with an overline naming the parent podcast (e.g., Chalk Radio) above the episode title. This PR doesn't render it. More on that below.
Address Before Merging: Groups 1, 2, 3 above.
Unsure: I'm unsure if we need to address 4 before merging. That's a product question for Ferdi/Peter. On the one hand, we could improve it later. On the other hand, the whole point of "Recent Episodes" is to aggregate episodes across podcasts, so showing the podcast title is important.
Podcast title on episode cards (the trickiest)
My comment at :109. Figma shows each card with an overline naming the parent podcast (e.g., Chalk Radio) above the episode title. We don't render it today (the overline is wired to offered_by, which is the wrong field and null anyway).
Either way I'd fix it by exposing resource.podcast_episode.podcast_title. Wrinkle: the model allows multiple podcast parents, but we've agreed we don't want that and afaik other platforms don't either, so expose the primary parent's title (matching the podcasts[0] we already use for the episode link).
Happy to discuss the title issue more in morning.
| import { PodcastsListingPage } from "@/app-pages/PodcastPage/PodcastsListingPage" | ||
|
|
||
| export const metadata: Metadata = standardizeMetadata({ | ||
| title: "MIT Learn | Podcasts", |
There was a problem hiding this comment.
Write now the title is MIT Learn | Podcasts | MIT Learn... Should just be "Podcasts" here. The site name gets appended automatically.
| gap: "48px", | ||
| padding: "40px", | ||
| alignItems: "flex-start", | ||
| backgroundColor: theme.custom.colors.lightGray1, |
There was a problem hiding this comment.
This should be lightGray0. Smoot-design actually has it, but in in v6.28; learn is on 6.27
| })() | ||
| : null | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
There's a lot of logic near here that is duplicated exactly in three files:
- frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx
- frontends/main/src/app-pages/PodcastPage/PodcastListingPage.tsx
- frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.tsx
All three files have:
useEffectsetting/unsetting--mit-player-height- an IIFE calculating
currentTrack handlePlayClick,handlePauseClick
Let's move all three to a shared hook:
const usePodcastPlayer = (playerRef, isMobile) => {
useEffect(/* CSS effect for --mit-player-height */)
const toggle = (epside, podcastName) => {
/* single callback for play/pause toggling; simplifies consumers */
}
const currentTrack = etc
return { toggle, currentTrack }
}NOTE:
- some places use play/pause/toggle, but I think they could all just use toggle
- currentTrack is a little tricky because of the parent podcast title issue... I think we should resolve that first (possibly outside this pr)
|
|
||
| const ENV_KEY = "NEXT_PUBLIC_PODCASTS_FEATURED_LIST_LEARNINGPATH_ID" | ||
|
|
||
| jest.mock("../PodcastPlayer", () => { |
There was a problem hiding this comment.
This is a pretty heavy / complex mock; we are using it (we check play / pause in this testfile). Howveer, both PodcastDetailPage.test.tsx and PodcastEpisodeDetailPage.test.tsx mock ./PodcastPlayer similarly. Let's have a shared mockPodcastPlayer to (A) reuse and (B) isolate the complexity of this mock. It could live aside PodcastPlayer as PodcastPlayer.test-utils.tsx perhaps.
There was a problem hiding this comment.
- PodcastDetailPage defines
EpisodeItem, and then it's used again byPodcastEpisodeDetailPage.tsx - Then
PodcastsListingPage.tsxdefines this file
I think we should unify them into a single component:
- They are the same component in figma, though there are some overrides that make styles different
- In particular, the podcast listing version has 24px vertical padding vs 28px on the other two. This seems like a mistake. We might want to check with @mbilalmughal ... Seems like they should be the same style
- There is one clearly intentional difference in Figma... whether the podcast title is shown. The podcast listing page is supposed to show the title, podcast detail + episode detail do not, since those are scoped to a single podcast.
- note: we don't actually show the podcast title on the new podcast listing page either; Figma shows it, but it's not implemented here.... Same underlying issue as https://github.com/mitodl/mit-learn/pull/3576/changes#r3540458476
Additionally: Both implementations have some accessibility issues:
- the EpisodeItem is an anchor tag, but expose to screenreaders as a list item via
role="listitem"override. Screenreader users won't know it's a link. - the anchor tag + play button are nested ... Not supposed to nest interactive elements
I don't think we need to fix those a11y issues in this pr (we should fix them, though). But we should not duplicate the EpisodeItem implementations—then we'd have to fix it in two places.
In summary: Unify now, address a11y issue later in a single place.
So, let's reuse the existing EpisodeItem!
| return ( | ||
| <HeroSectionWrap> | ||
| <PodcastContainer> | ||
| <HeroHeading variant="h1">Podcasts from across MIT</HeroHeading> |
There was a problem hiding this comment.
Accessibility Issue: The podcast listing page currently has no heading elements!
This should be component="h1"
Additionally, I'd recommend:
A... h1 = Podcasts from across MIT
B...h2 = Now Playing: <Big Featured Podcast's Title>
C... h2 = Latest Episodes
D... h2 = Podcasts Across MIT
E... h2 = More Podcasts
Note: ☝️ h1/h2 are about the element, not the visual variant property.
What do you think of the suggested headings above?
| episode_link: "https://example.com/link", | ||
| }, | ||
| ...overrides, | ||
| }) as unknown as LearningResource |
There was a problem hiding this comment.
Not a huge deal, but I would get rid of this, or change it simply to
const makeEpisode = factories.learningResources.podcastEpisodeif you want a shorter name.
- Most of the overrides are unnecessary
- Our factories merge overrides deeply, so if you do need to override an object, you can override just the piece you care about
- In a few places you assert on
"Episode Title"; Personally, I prefer just to assert onepisode.title. - If you need a specific override, you can declare it in the test that cares about that specific override
- This is the main reason to just use
factories.learningResources.podcastEpisodedirectly. Unless you're actually sharing / re-using a complicate test case, it's clearer to keep the overrides co-located with the test that actually cares about them.
- This is the main reason to just use
| return { | ||
| audioUrl, | ||
| title: playingEpisode.title || "Untitled Episode", | ||
| podcastName: playingEpisode.offered_by?.name || "Podcast", |
There was a problem hiding this comment.
We want this to be the podcast title, not offered_by.
- We shouldn't use
offered_by. It's not populated, anyway. (You and I both noticed it wasnullon episode responses). - We can't currently use the podcast title... It's not exposed on the episode API results.
IMO, we should open a separate PR to get the podcast title exposed on the resource.podcast_episode object.
offered_by fallback is used elsewhere (e.g., the EpisodeItem card overline value). We should not use that as the fallback value anywhere, imo.
| <Link color="red" href={SEARCH_PODCAST_EPISODES}> | ||
| <SectionLink>All episodes</SectionLink> | ||
| </Link> |
There was a problem hiding this comment.
Request: The "All Episodes" link should directly use our link from ol-components, probably with variant="red". For some reason, Figma just has this as a plain text node:
Plain text is bad, no hover state, plus drift from other links. Confirm with @mbilalmughal, but yeah, this should be a Link component.
Suggestion: Review figma first for things that should probably be a component. It's easy to miss things... helps to ask your agent to review also, and scan for places where Figma should be using design tokens but isn't.
What are the relevant tickets?
https://github.com/mitodl/hq/issues/12147
Description (What does it do?)
As a user, I want a central place to view podcasts from MIT, including new podcasts, recommended podcasts, and be able to view all podcasts.
Screenshots (if appropriate):
Desktop
Mobile
How can this be tested?
For testing you need to visit the following URL
http://open.odl.local:8062/podcastsit will load the page, for featured podcasts you need to create the learningPath list from the following urlshttp://open.odl.local:8062/learningpathsand add few podcasts from search page and then set the id of this newly created list in frontend envNEXT_PUBLIC_PODCASTS_FEATURED_LIST_LEARNINGPATH_ID=<learningPath_id>it will bring the featured Podcasts from this list.Additional Context