Sitemap fix 2 of 2: Page the podcast and video sitemaps off the summary endpoint - #3759
Open
ChristopherChudzicki wants to merge 1 commit into
Open
Sitemap fix 2 of 2: Page the podcast and video sitemaps off the summary endpoint#3759ChristopherChudzicki wants to merge 1 commit into
ChristopherChudzicki wants to merge 1 commit into
Conversation
OpenAPI ChangesNo changes detected Unexpected changes? Ensure your branch is up-to-date with |
ChristopherChudzicki
marked this pull request as ready for review
August 11, 2026 23:42
ChristopherChudzicki
changed the title
ahtesham-quraish
approved these changes
Aug 13, 2026
ahtesham-quraish
left a comment
ahtesham-quraish
left a comment
Contributor
There was a problem hiding this comment.
LGTM!
Question/Suggestion
frontends/main/src/app/sitemaps/podcast/sitemap.ts line 71: Confirm the podcast episode sitemap intentionally emits one URL per parent podcast (fan-out), whereas the video sitemap emits only the first parent. If both should be canonical-single, why this asymmetry we have?; if intended, a short comment mirroring the video file's rationale would help future readers.
ChristopherChudzicki
force-pushed
the
cc/sitemaps-use-summary-endpoint
branch
from
August 13, 2026 11:27
3c79bb5 to
abd9cdc
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Switches podcast and video sitemap page generation to the lightweight learning-resource summary endpoint.
Changes:
- Uses summary records and canonical parent IDs for sitemap URLs.
- Updates sitemap tests and summary factories accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
frontends/main/src/app/sitemaps/video/sitemap.ts |
Uses summary data for video sitemap pages. |
frontends/main/src/app/sitemaps/video/sitemap.test.ts |
Tests summary-based video URLs. |
frontends/main/src/app/sitemaps/podcast/sitemap.ts |
Uses summary data for podcast sitemap pages. |
frontends/main/src/app/sitemaps/podcast/sitemap.test.ts |
Tests summary-based podcast URLs. |
frontends/api/src/test-utils/factories/learningResources.ts |
Allows summary resource types to be overridden. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Both sitemaps request limit=1000 from /api/v1/learning_resources/, which caps at 100 while their offset keeps advancing by 1000 -- so each shard returns its first 100 resources and the other 900 are never emitted. The podcast sitemap publishes 300 of 2133 URLs, the video sitemap 800 of 7109 (mitodl/mit-learn#3756). /summary/ allows limit=1000, and now carries resource_type and canonical_parent_ids, so it can serve these two the way it already serves the resources and products sitemaps. Switching endpoints is the fix -- PAGE_SIZE becomes a limit the API honors rather than one it silently clamps. canonical_parent_ids replaces the parentPodcastIds/videoPlaylistIds helpers, which read nested fields the summary serializer doesn't carry. It reports the same ids in the same order, so the emitted URLs are unchanged: one per parent podcast for an episode, and for a video the first playlist, matching the canonical tag and the bare-URL redirect on the video page. The helpers stay for the page components that still read detail responses. The summary factory is no longer a LearningResourceFactory, whose overrides omit resource_type. That's right for the fixed-type factories, but a summary carries every resource type and these tests have to pin it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChristopherChudzicki
force-pushed
the
cc/sitemaps-use-summary-endpoint
branch
from
August 13, 2026 14:07
abd9cdc to
96fb40d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Do not merge until #3758 has been released
What are the relevant tickets?
Description (What does it do?)
Switches the podcast episode and video sitemaps to use the
api/v1/learning_resoruces/summaryendpoint.How can this be tested?
Prerequisite: Podcasts and videos backpopulated locally (at least podcastes).