WS-203 - Most Read pages on Next.js#13838
Conversation
amoore108
changed the title
There was a problem hiding this comment.
Pull request overview
Renders Most Read pages inside the Next.js app by treating them as Topic pages backed by a dedicated mostReadTopic identifier, including local fixture support and a temporary rewrite for variant-prefixed URLs.
Changes:
- Added a Next.js route at
[service]/popular/read/[[...variant]]that fetches Topic data (idmostReadTopic) and renders it as a Most Read page. - Added a temporary Next.js rewrite to support variant-in-the-middle URLs (e.g.
/serbian/cyr/popular/read). - Updated
constructPageFetchUrl(and tests) to support resolving a Topic fetch id formostReadTopic, plus added a pidgin fixture.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ws-nextjs-app/pages/[service]/popular/read/[[...variant]].page.tsx | New SSR route for Most Read in Next.js using Topic fetch data. |
| ws-nextjs-app/pages/[service]/popular/read/[[...variant]].page.test.tsx | Unit tests for the new Most Read route SSR props + caching header. |
| ws-nextjs-app/next.config.js | Temporary rewrite to support services where variant appears before /popular/read. |
| src/app/routes/utils/constructPageFetchUrl/index.ts | Special-cases Topic ID derivation for mostReadTopic. |
| src/app/routes/utils/constructPageFetchUrl/index.test.ts | Adds coverage for the mostReadTopic construct URL behavior in test/live envs. |
| data/pidgin/topics/mostReadTopic.json | Local fixture backing the new Most Read Topic data. |
There was a problem hiding this comment.
Sorry, just wanted to ask where this fixture data came from? Was it FABL?
There was a problem hiding this comment.
Yea we added some logic in the API to return a topic page if id='mostReadTopic' is being requested: https://github.com/bbc/fabl-modules/pull/11664
Just seemed to make sense to re-use the Topics API since it handles all the fetching and data transformation for us already.
| 'Cache-Control', | ||
| 'public, stale-if-error=2400, stale-while-revalidate=960, max-age=240', |
There was a problem hiding this comment.
Does this come from the request headers in the network tab? If so from which page did you get this from?
There was a problem hiding this comment.
Yea it comes from the cache-control repsonse header.
This was copied from the Topics page, so its actually using that value. I'll change it to the existing Most Read page values as they are a bit different.
pvaliani
left a comment
pvaliani
left a comment
There was a problem hiding this comment.
Looks good, is this just for canonical? Noticed http://localhost:7081/mundo/popular/read.lite?renderer_env=live doesn't render
Hmm good spot. Should be working on Lite. I'll take a look. |
Should |
|
This is how |
AMP will no longer be supported, but product are ok with this. |
Resolves JIRA: https://bbc.atlassian.net/browse/WS-203
Summary
Renders Most Read pages as Topic pages under the Next.js app
Note: These pages won't be using the Next.js app until routing changes are applied upstream
Code changes
[service]/popular/read/[[...variant]]route to Next.js apprewriteto allow services with variants to render under this route. This can be removed once we add a permanent redirect upstream to redirect variant paths to have the variant at the end of the pathTesting
Useful Links