feat(web): Consume Experience and render watch page sections#63
Merged
feat(web): Consume Experience and render watch page sections#63
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis PR introduces a watch experience rendering system for the web application. It adds new React section components (MediaCollection, PromoBanner, InfoBlocks, CTASection), state-handling components (ExperienceEmpty, ExperienceError), a GraphQL-based content fetching mechanism, locale resolution utilities, data enrichment logic, and new dynamic pages that render experiences from Strapi. Environment variables are centralized via a typed env module, and Next.js configuration is updated to permit remote image loading from localhost. Changes
Sequence DiagramsequenceDiagram
participant Browser
participant NextJS as Next.js Page
participant Apollo as Apollo Client
participant GraphQL as Strapi GraphQL API
participant Enrichment as Enrichment Layer
participant Renderer as Section Renderer
Browser->>NextJS: Request /watch or /watch/[slug]
NextJS->>NextJS: Extract locale from route/headers
NextJS->>Apollo: getWatchExperience(locale, {slug?})
Apollo->>GraphQL: Query GetWatchExperience
GraphQL-->>Apollo: Return experience with sections
Apollo-->>NextJS: data or error result
alt Error occurred
NextJS->>Renderer: Render ExperienceError
Renderer-->>Browser: Display error message
else No sections
NextJS->>Renderer: Render ExperienceEmpty
Renderer-->>Browser: Display empty state
else Success with sections
NextJS->>Enrichment: enrichMediaItem() for each item
Enrichment-->>NextJS: Return enriched items
NextJS->>Renderer: Map sections to SectionRenderer
Renderer->>Renderer: Switch on section.__typename
Renderer->>Renderer: Render MediaCollection/PromoBanner/InfoBlocks/CTASection
Renderer-->>Browser: Display experience sections
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…cale] Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (6)
apps/web/src/components/sections/InfoBlocks.tsx (1)
1-21:idinInfoBlocksPropsis accepted but never destructured or used.The
idprop is declared in the type and passed bySectionRenderer, but the function signature doesn't destructure it and it is never referenced inside the component. Same pattern applies toPromoBanner(Line 2 of PromoBanner.tsx).♻️ Proposed fix
export function InfoBlocks({ heading, intro, description, blocks, + // id is available for consumers needing it (e.g. anchor links) }: InfoBlocksProps) {Or, if it is intentionally unused today, remove it from the props type to avoid confusion:
type InfoBlocksProps = { - id: string heading?: string | null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sections/InfoBlocks.tsx` around lines 1 - 21, InfoBlocksProps declares an id prop that InfoBlocks doesn't destructure or use; either (A) include id in the component signature (export function InfoBlocks({ id, heading, intro, description, blocks }: InfoBlocksProps)) and apply it to the root element (e.g., section/div id={id}) so SectionRenderer-provided ids are preserved, or (B) remove id from the InfoBlocksProps type if it's intentionally unused; apply the same fix pattern to PromoBanner (ensure PromoBanner either destructures/uses id or remove it from its props type).apps/web/src/lib/locale.ts (1)
9-12: No validation on the extractedAccept-Languageprimary tag.
primarycould be"*"(fromAccept-Language: *) or any other non-locale string. Passing an invalid locale code directly to the StrapiI18NLocaleCodevariable will cause the query to fail or return no results silently, surfacing as a spurious "No experience found" error to the user. Consider guarding with an allowlist of supported locales or at minimum a basic/^[a-z]{2,3}$/icheck before trusting the parsed value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/locale.ts` around lines 9 - 12, The parsed primary locale from acceptLanguage (variable primary derived from acceptLanguage.split...) is not validated and may be "*" or other invalid values; update the logic in apps/web/src/lib/locale.ts (where acceptLanguage and primary are computed) to validate primary before returning—either check against a supported-locale allowlist or apply a regex like /^[a-z]{2,3}$/i to ensure it’s a valid ISO-like code, and only return primary if it passes validation (otherwise fall back to the default locale or undefined so downstream I18NLocaleCode queries don’t receive invalid values).apps/web/src/lib/enrichment.ts (1)
1-11:MediaItemis duplicated betweenenrichment.tsand the inline type insections/index.tsx.The unexported
MediaItemtype here is structurally identical to theitemsarray element type defined inline inMediaCollectionSectioninindex.tsx(lines 17–27). ExportingMediaItemand importing it inindex.tsxwould give one source of truth.♻️ Proposed refactor
In
enrichment.ts:-type MediaItem = { +export type MediaItem = {In
sections/index.tsx:+import { enrichMediaItem, type MediaItem } from "@/lib/enrichment" ... items?: Array<{ - id: string - titleOverride?: string | null - subtitleOverride?: string | null - imageOverride?: { url: string } | null - video?: { - title: string - slug: string - image?: { url: string } | null - } | null + id: string }> | nullOr more precisely, replace the inline shape with
MediaItem:items?: Array<MediaItem> | null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/enrichment.ts` around lines 1 - 11, The MediaItem type is duplicated; export the existing MediaItem from enrichment.ts (add an export on the type declaration) and in sections/index.tsx import MediaItem and replace the inline items element type used in MediaCollectionSection (the inline shape for the items array elements) with the imported MediaItem, then remove the duplicated inline type so there is a single source of truth.apps/web/src/components/sections/CTASection.tsx (1)
20-25: Usenext/link<Link>instead of a bare<a>for CTA navigation.A raw
<a>triggers a full-page reload for internal routes. IfbuttonLinkcan point to internal paths (e.g./watch/en), wrap it in Next.js<Link>so client-side navigation is preserved. The same applies toMediaCollection's "View all" anchor.♻️ Proposed refactor
+import Link from "next/link" ... - <a - href={buttonLink} - className="inline-block rounded bg-gray-800 px-6 py-3 font-medium text-white hover:bg-gray-900" - > + <Link + href={buttonLink} + className="inline-block rounded bg-gray-800 px-6 py-3 font-medium text-white hover:bg-gray-900" + > {buttonLabel} - </a> + </Link>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sections/CTASection.tsx` around lines 20 - 25, Replace the raw anchor used for the CTA with Next.js client-side navigation: wrap the existing <a> that uses buttonLink and buttonLabel in a next/link <Link> and move the href to the <Link> (keeping className on the rendered anchor) so internal routes use client-side routing; do the same change for MediaCollection's "View all" anchor to wrap it with <Link> to avoid full-page reloads. Ensure imports include Link from "next/link" and that props/buttonLink/buttonLabel usage remain unchanged.apps/web/src/components/sections/index.tsx (1)
17-27:itemselement shape inMediaCollectionSectionduplicates the unexportedMediaItemtype inenrichment.ts.Once
MediaItemis exported fromenrichment.ts(see comment there), replace the inline array element type here to eliminate the duplicated definition.♻️ Proposed refactor
In
enrichment.ts:-type MediaItem = { +export type MediaItem = {In
sections/index.tsx:+import { enrichMediaItem, type MediaItem } from "@/lib/enrichment" ... items?: Array<{ - id: string - titleOverride?: string | null - subtitleOverride?: string | null - imageOverride?: { url: string } | null - video?: { - title: string - slug: string - image?: { url: string } | null - } | null + // inherit from shared MediaItem }> | nullReplace the full inline shape with:
- items?: Array<{ - id: string - titleOverride?: string | null - subtitleOverride?: string | null - imageOverride?: { url: string } | null - video?: { - title: string - slug: string - image?: { url: string } | null - } | null - }> | null + items?: Array<MediaItem> | null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sections/index.tsx` around lines 17 - 27, The items prop in MediaCollectionSection duplicates the unexported MediaItem shape; export MediaItem from enrichment.ts and update MediaCollectionSection's items prop to use that exported type instead of the inline type—replace the inline element type (the object with id, titleOverride, subtitleOverride, imageOverride, video) with Array<MediaItem> | null (or mediaItem[] as appropriate) and import MediaItem at the top of the file so MediaCollectionSection and its items prop reference the single exported MediaItem type.apps/web/src/components/sections/MediaCollection.tsx (1)
28-55:ItemCarddefined inside the render function — extract to module scope.Although this is currently a server component (no reconciliation), defining a component inside a render function is an established anti-pattern: if
"use client"is ever added, React will create a new component type reference on every render, causing the fullItemCardsubtree to unmount and remount instead of reconciling.♻️ Proposed refactor
+import type { EnrichedMediaItem } from "@/lib/enrichment" + +function ItemCard({ + item, + index, + showItemNumbers, +}: { + item: EnrichedMediaItem + index: number + showItemNumbers?: boolean | null +}) { + return ( + <article className="rounded-lg border bg-white p-4 shadow-sm"> + {item.imageUrl && ( + <div className="relative mb-2 aspect-video w-full overflow-hidden rounded"> + <Image + src={item.imageUrl} + alt={item.title} + fill + className="object-cover" + sizes="(max-width: 768px) 100vw, 25vw" + /> + </div> + )} + <h3 className="font-semibold">{item.title}</h3> + {item.subtitle && <p className="text-sm text-gray-600">{item.subtitle}</p>} + {showItemNumbers && <span className="text-xs text-gray-400">{index + 1}</span>} + </article> + ) +} + export function MediaCollection({ ... }) { ... - const ItemCard = ({ item, index }: ...) => (...) ... {items.map((item, i) => ( - <ItemCard key={item.id} item={item} index={i} /> + <ItemCard key={item.id} item={item} index={i} showItemNumbers={showItemNumbers} /> ))} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sections/MediaCollection.tsx` around lines 28 - 55, The ItemCard component must be moved out of the render scope into module scope to avoid creating a new component type on each render; create a top-level function/component named ItemCard that accepts props { item: EnrichedMediaItem, index: number, showItemNumbers: boolean } (preserve the EnrichedMediaItem type usage and Image usage) and render the same markup there, then update the current in-render usage to pass showItemNumbers and index into the top-level ItemCard; optionally wrap ItemCard with React.memo if you want to prevent unnecessary re-renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/next.config.mjs`:
- Around line 7-12: The current images.remotePatterns only whitelists
localhost/127.0.0.1 so production/staging Strapi images will 400; update
next.config.mjs to add explicit remotePatterns entries for your deployed Strapi
host(s) and include the protocol to avoid the implicit ** wildcard. Concretely,
modify the images.remotePatterns array (in next.config.mjs) to push one or more
entries that use a secure protocol (e.g. protocol: "https") and the production
hostname driven from an env var (e.g. process.env.STRAPI_HOSTNAME or
NEXT_PUBLIC_STRAPI_URL) with the same pathname "/uploads/**", or add a separate
hardcoded staging/production hostname entry if you prefer.
In `@apps/web/src/app/watch/page.tsx`:
- Around line 22-32: The map over experience.sections contains an unreachable
sentinel check for { __typename: "Error" } and uses an unsafe cast to a
hand-rolled Section type; remove the "__typename" branch and the Array<Section |
...> cast, and instead use the generated Apollo section type (or derive your
Section union from the generated types in index.tsx) so TypeScript enforces
correct nullability/shape; keep the existing guard for falsy section, compute
key as "id" in section ? section.id : `section-${i}`, and return
<SectionRenderer key={key} section={section} /> with the section typed from the
generated GraphQL types.
In `@apps/web/src/components/ExperienceError.tsx`:
- Around line 5-12: The ExperienceError component currently renders raw error
strings; change it to show a sanitized, user-friendly message instead by mapping
known error conditions (e.g., errors thrown by getWatchExperience such as
"GraphQL URL not configured" or network errors) to friendly labels and
defaulting to a generic "An unexpected error occurred" for anything else;
implement the mapping either inside ExperienceError (e.g., add a helper like
sanitizeErrorMessage) or, better, return a sanitized message from
getWatchExperience and reserve the original error for server-side logging;
ensure ExperienceError no longer displays raw hostnames/ports and that full
error details are logged only server-side.
In `@apps/web/src/components/sections/CTASection.tsx`:
- Around line 1-14: CTASectionProps declares an id prop but CTASection doesn't
destructure or apply it; update the CTASection function signature to accept id
(from CTASectionProps) and add id={id} to the rendered <section> element so
anchors work; mirror the same fix pattern used in MediaCollection.tsx and ensure
SectionRenderer continues passing id={section.id} to CTASection.
In `@apps/web/src/components/sections/index.tsx`:
- Around line 7-60: The file defines a hand-rolled Section union that duplicates
the generated GraphQL types; instead export the generated WatchExperience type
from content.ts (the type derived from the GET_WATCH_EXPERIENCE result) and in
apps/web/src/components/sections/index.tsx remove the manual Section definition
and import WatchExperience, then derive Section from WatchExperience.sections
(use a NonNullable/array-element extract) and remove any unnecessary "as
Section" casts in consuming components so the code uses the single
source-of-truth WatchExperience type.
In `@apps/web/src/components/sections/InfoBlocks.tsx`:
- Around line 36-40: In the InfoBlocks component, remove the conflicting
role="img" on the icon span and mark the decorative icon as hidden to assistive
tech by using aria-hidden={true} (leave the existing conditional render of
block.icon and the span className intact); ensure no role attribute remains on
the span so the decorative icon is only aria-hidden and not announced by screen
readers.
In `@apps/web/src/components/sections/MediaCollection.tsx`:
- Around line 84-91: The "View all" string in MediaCollection is hardcoded;
update MediaCollectionProps to accept an optional ctaLabel?: string, use that
prop in the JSX in place of the literal "View all" with a fallback like ctaLabel
?? "View all", and ensure any call sites or CMS wiring pass the CMS-driven label
into the ctaLabel prop when ctaLink is provided; update the MediaCollection
component signature and its prop type/interface to include ctaLabel and replace
the hardcoded text with the prop + fallback.
- Around line 4-5: MediaCollectionProps declares an id prop that is passed from
SectionRenderer but not applied to the rendered <section>, so destructure id
from the MediaCollection component props (or accept ...props) and forward it to
the section element as id={id}; update the component signature that uses
MediaCollectionProps and ensure the rendered <section> includes the id attribute
so SectionRenderer-provided ids are honored.
- Around line 36-44: The sizes attribute is hard-coded to " (max-width: 768px)
100vw, 25vw" causing undersized images for the hero and player variants; update
the MediaCollection component (the Image usage where item.imageUrl is rendered)
to compute sizes based on the variant prop: use 100vw for 'hero' (e.g.,
"(max-width: 768px) 100vw, 100vw"), ~33vw for 'player' (e.g., "(max-width:
768px) 100vw, 33vw"), and keep the existing 25vw fallback for other variants,
and pass that computed sizes string into the Image component.
In `@apps/web/src/components/sections/PromoBanner.tsx`:
- Around line 25-30: The CTA label is hardcoded in the PromoBanner component;
update PromoBannerProps to add an optional ctaLabel string, update the GraphQL
fragment that feeds this component to request ctaLabel from Strapi, and change
the anchor to use props.ctaLabel || t('learn_more') (or your app's translation
helper) as the visible text so it falls back to a locale-aware default when
Strapi doesn't provide a custom label. Ensure you export/accept ctaLabel in the
PromoBanner function signature and update any callers that construct
PromoBannerProps if needed.
In `@apps/web/src/lib/content.ts`:
- Around line 97-119: The options.homepage parameter is unused in
getWatchExperience and the filters variable always falls back to { isHomepage: {
eq: true } } when slug is absent; update the logic to either honor the homepage
flag or remove it from the signature: either (A) change the filters computation
in getWatchExperience to use options?.homepage (e.g. when slug is null use {
isHomepage: { eq: options?.homepage ?? true } } or equivalent) so callers can
request non-homepage results, or (B) remove the homepage property from the
options type and any callers that pass it so the function signature only accepts
slug, and delete any related dead handling around filters/slug to avoid
confusion.
In `@apps/web/src/lib/enrichment.ts`:
- Line 23: The subtitle currently falls back to the machine slug
(item.video?.slug) which is user-facing; change the fallback so subtitle uses
item.subtitleOverride ?? "" (do not expose item.video?.slug directly) and ensure
any related variable videoSlug remains the raw slug for programmatic use only
(add a separate display field like displaySubtitle or keep subtitle empty when
override is absent). Update the assignment for subtitle and, if present, remove
usages that render videoSlug to the UI so only the displaySubtitle/subtitle
(empty or override) is shown.
---
Nitpick comments:
In `@apps/web/src/components/sections/CTASection.tsx`:
- Around line 20-25: Replace the raw anchor used for the CTA with Next.js
client-side navigation: wrap the existing <a> that uses buttonLink and
buttonLabel in a next/link <Link> and move the href to the <Link> (keeping
className on the rendered anchor) so internal routes use client-side routing; do
the same change for MediaCollection's "View all" anchor to wrap it with <Link>
to avoid full-page reloads. Ensure imports include Link from "next/link" and
that props/buttonLink/buttonLabel usage remain unchanged.
In `@apps/web/src/components/sections/index.tsx`:
- Around line 17-27: The items prop in MediaCollectionSection duplicates the
unexported MediaItem shape; export MediaItem from enrichment.ts and update
MediaCollectionSection's items prop to use that exported type instead of the
inline type—replace the inline element type (the object with id, titleOverride,
subtitleOverride, imageOverride, video) with Array<MediaItem> | null (or
mediaItem[] as appropriate) and import MediaItem at the top of the file so
MediaCollectionSection and its items prop reference the single exported
MediaItem type.
In `@apps/web/src/components/sections/InfoBlocks.tsx`:
- Around line 1-21: InfoBlocksProps declares an id prop that InfoBlocks doesn't
destructure or use; either (A) include id in the component signature (export
function InfoBlocks({ id, heading, intro, description, blocks }:
InfoBlocksProps)) and apply it to the root element (e.g., section/div id={id})
so SectionRenderer-provided ids are preserved, or (B) remove id from the
InfoBlocksProps type if it's intentionally unused; apply the same fix pattern to
PromoBanner (ensure PromoBanner either destructures/uses id or remove it from
its props type).
In `@apps/web/src/components/sections/MediaCollection.tsx`:
- Around line 28-55: The ItemCard component must be moved out of the render
scope into module scope to avoid creating a new component type on each render;
create a top-level function/component named ItemCard that accepts props { item:
EnrichedMediaItem, index: number, showItemNumbers: boolean } (preserve the
EnrichedMediaItem type usage and Image usage) and render the same markup there,
then update the current in-render usage to pass showItemNumbers and index into
the top-level ItemCard; optionally wrap ItemCard with React.memo if you want to
prevent unnecessary re-renders.
In `@apps/web/src/lib/enrichment.ts`:
- Around line 1-11: The MediaItem type is duplicated; export the existing
MediaItem from enrichment.ts (add an export on the type declaration) and in
sections/index.tsx import MediaItem and replace the inline items element type
used in MediaCollectionSection (the inline shape for the items array elements)
with the imported MediaItem, then remove the duplicated inline type so there is
a single source of truth.
In `@apps/web/src/lib/locale.ts`:
- Around line 9-12: The parsed primary locale from acceptLanguage (variable
primary derived from acceptLanguage.split...) is not validated and may be "*" or
other invalid values; update the logic in apps/web/src/lib/locale.ts (where
acceptLanguage and primary are computed) to validate primary before
returning—either check against a supported-locale allowlist or apply a regex
like /^[a-z]{2,3}$/i to ensure it’s a valid ISO-like code, and only return
primary if it passes validation (otherwise fall back to the default locale or
undefined so downstream I18NLocaleCode queries don’t receive invalid values).
Comment on lines
+7
to
+12
| images: { | ||
| remotePatterns: [ | ||
| { hostname: "localhost", pathname: "/uploads/**" }, | ||
| { hostname: "127.0.0.1", pathname: "/uploads/**" }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Images will break in any non-local environment — production Strapi hostname is missing.
Only localhost and 127.0.0.1 are whitelisted. Protocol, hostname, and path are matched exactly, so any mismatch fails the check. Once Strapi is deployed to a staging/production domain, every next/image call referencing Strapi media will return a 400 and render a broken image.
The hostname for the production (and staging) Strapi instance must be added — ideally driven by an env-aware pattern or a separate entry. Additionally, when omitting protocol, the wildcard ** is implied, which is not recommended because it may allow malicious actors to optimize URLs you did not intend.
🛠️ Proposed fix
images: {
remotePatterns: [
- { hostname: "localhost", pathname: "/uploads/**" },
- { hostname: "127.0.0.1", pathname: "/uploads/**" },
+ { protocol: "http", hostname: "localhost", pathname: "/uploads/**" },
+ { protocol: "http", hostname: "127.0.0.1", pathname: "/uploads/**" },
+ // Add staging/production Strapi hostname(s), e.g.:
+ // { protocol: "https", hostname: "cms.example.com", pathname: "/uploads/**" },
],
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| images: { | |
| remotePatterns: [ | |
| { hostname: "localhost", pathname: "/uploads/**" }, | |
| { hostname: "127.0.0.1", pathname: "/uploads/**" }, | |
| ], | |
| }, | |
| images: { | |
| remotePatterns: [ | |
| { protocol: "http", hostname: "localhost", pathname: "/uploads/**" }, | |
| { protocol: "http", hostname: "127.0.0.1", pathname: "/uploads/**" }, | |
| // Add staging/production Strapi hostname(s), e.g.: | |
| // { protocol: "https", hostname: "cms.example.com", pathname: "/uploads/**" }, | |
| ], | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/next.config.mjs` around lines 7 - 12, The current
images.remotePatterns only whitelists localhost/127.0.0.1 so production/staging
Strapi images will 400; update next.config.mjs to add explicit remotePatterns
entries for your deployed Strapi host(s) and include the protocol to avoid the
implicit ** wildcard. Concretely, modify the images.remotePatterns array (in
next.config.mjs) to push one or more entries that use a secure protocol (e.g.
protocol: "https") and the production hostname driven from an env var (e.g.
process.env.STRAPI_HOSTNAME or NEXT_PUBLIC_STRAPI_URL) with the same pathname
"/uploads/**", or add a separate hardcoded staging/production hostname entry if
you prefer.
Comment on lines
+22
to
+32
| {( | ||
| experience.sections as Array<Section | { __typename: "Error" } | null> | ||
| ).map((section, i) => { | ||
| if ( | ||
| !section || | ||
| ("__typename" in section && section.__typename === "Error") | ||
| ) | ||
| return null | ||
| const key = "id" in section ? section.id : `section-${i}` | ||
| return <SectionRenderer key={key} section={section as Section} /> | ||
| })} |
There was a problem hiding this comment.
{ __typename: "Error" } sentinel branch is unreachable and the as cast silences type divergence.
Two related notes on this block:
-
getWatchExperiencenever returns sections; on failure it returns{ data: null, error: ... }which is caught by theresult.errorguard above (line 11). Thesection.__typename === "Error"branch is effectively dead code — safe to remove. -
The
as Array<Section | ...>cast bridges the generated Apollo type to the hand-rolledSectionunion. If any field nullability or shape differs between the two (e.g.ctaLink: stringrequired inSectionbut nullable in the generated type), TypeScript won't catch it and a runtime failure or silenthref={null}could result. This risk goes away ifSectionis derived from the generated types (see the comment inindex.tsx).
🐛 Proposed simplification (after aligning Section with generated types)
- {(
- experience.sections as Array<Section | { __typename: "Error" } | null>
- ).map((section, i) => {
- if (
- !section ||
- ("__typename" in section && section.__typename === "Error")
- )
- return null
- const key = "id" in section ? section.id : `section-${i}`
- return <SectionRenderer key={key} section={section as Section} />
- })}
+ {(experience.sections as Array<Section | null>).map((section, i) =>
+ section ? (
+ <SectionRenderer key={section.id ?? `section-${i}`} section={section} />
+ ) : null
+ )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/watch/page.tsx` around lines 22 - 32, The map over
experience.sections contains an unreachable sentinel check for { __typename:
"Error" } and uses an unsafe cast to a hand-rolled Section type; remove the
"__typename" branch and the Array<Section | ...> cast, and instead use the
generated Apollo section type (or derive your Section union from the generated
types in index.tsx) so TypeScript enforces correct nullability/shape; keep the
existing guard for falsy section, compute key as "id" in section ? section.id :
`section-${i}`, and return <SectionRenderer key={key} section={section} /> with
the section typed from the generated GraphQL types.
Comment on lines
+7
to
+60
| type MediaCollectionSection = { | ||
| __typename: "ComponentSectionsMediaCollection" | ||
| id: string | ||
| title?: string | null | ||
| subtitle?: string | null | ||
| description?: string | null | ||
| categoryLabel?: string | null | ||
| ctaLink?: string | null | ||
| showItemNumbers?: boolean | null | ||
| variant: "carousel" | "collection" | "grid" | "hero" | "player" | ||
| items?: Array<{ | ||
| id: string | ||
| titleOverride?: string | null | ||
| subtitleOverride?: string | null | ||
| imageOverride?: { url: string } | null | ||
| video?: { | ||
| title: string | ||
| slug: string | ||
| image?: { url: string } | null | ||
| } | null | ||
| }> | null | ||
| } | ||
|
|
||
| export type Section = | ||
| | MediaCollectionSection | ||
| | { | ||
| __typename: "ComponentSectionsPromoBanner" | ||
| id: string | ||
| heading: string | ||
| description: string | ||
| intro?: string | null | ||
| ctaLink: string | ||
| } | ||
| | { | ||
| __typename: "ComponentSectionsInfoBlocks" | ||
| id: string | ||
| heading?: string | null | ||
| intro?: string | null | ||
| description?: string | null | ||
| blocks?: Array<{ | ||
| id: string | ||
| title: string | ||
| description: string | ||
| icon: string | ||
| }> | null | ||
| } | ||
| | { | ||
| __typename: "ComponentSectionsCta" | ||
| id: string | ||
| heading: string | ||
| body: string | ||
| buttonLabel: string | ||
| buttonLink: string | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for generated type files that define the ComponentSections* types
rg -l "ComponentSectionsMediaCollection\|ComponentSectionsCta\|ComponentSectionsPromoBanner" \
--glob "!apps/web/src/components/**" \
--glob "!**/node_modules/**"
# Check for codegen output directories
fd -t d -n "generated" | grep -v node_modules | head -20
fd -t d -n "__generated__" | grep -v node_modules | head -20
# Look for any graphql-codegen config
fd -e yml -e yaml -e json -n "*codegen*" | grep -v node_modules | head -20Repository: JesusFilm/forge
Length of output: 751
🏁 Script executed:
#!/bin/bash
# Look for generated type files that define the ComponentSections* types
echo "=== Searching for ComponentSections* types outside apps/web/src/components ==="
rg "ComponentSectionsMediaCollection|ComponentSectionsCta|ComponentSectionsPromoBanner" \
--glob "!apps/web/src/components/**" \
--glob "!**/node_modules/**" \
-l
echo ""
echo "=== Looking for 'generated' directories ==="
fd "generated" -t d | grep -v node_modules | head -20
echo ""
echo "=== Looking for '__generated__' directories ==="
fd "__generated__" -t d | grep -v node_modules | head -20
echo ""
echo "=== Looking for codegen config files ==="
fd "codegen" -e yml -e yaml -e json | grep -v node_modules | head -20
echo ""
echo "=== Checking packages/graphql structure ==="
ls -la packages/graphql/ 2>/dev/null || echo "packages/graphql not found"
echo ""
echo "=== Searching for all mentions of ComponentSections in codebase ==="
rg "ComponentSections" --glob "!**/node_modules/**" | head -30Repository: JesusFilm/forge
Length of output: 18646
🏁 Script executed:
#!/bin/bash
echo "=== Reading apps/web/src/lib/content.ts ==="
cat -n apps/web/src/lib/content.ts
echo ""
echo "=== Checking for codegen config in package.json files ==="
rg "codegen" -A 3 --glob "package.json"
echo ""
echo "=== Looking for codegen config ==="
find . -name "codegen.ts" -o -name "codegen.js" -o -name "codegen.config.*" 2>/dev/null | grep -v node_modules
echo ""
echo "=== Checking apps/web package.json for codegen ==="
head -50 apps/web/package.jsonRepository: JesusFilm/forge
Length of output: 5115
🏁 Script executed:
#!/bin/bash
echo "=== Checking where Section type is used ==="
rg "as Section|type Section" apps/web/src --glob "*.ts" --glob "*.tsx" -B 2 -A 2
echo ""
echo "=== Checking watch/page.tsx ==="
cat -n apps/web/src/app/watch/page.tsx 2>/dev/null || echo "File not found, searching for watch page"
fd "page.tsx" apps/web/src/app/watch/ 2>/dev/null
echo ""
echo "=== Checking if `@forge/graphql` exports ResultOf ==="
cat packages/graphql/package.json
echo ""
echo "=== Checking packages/graphql/src structure ==="
ls -la packages/graphql/src/
</parameter>Repository: JesusFilm/forge
Length of output: 3697
🏁 Script executed:
#!/bin/bash
echo "=== Checking packages/graphql/src/index.ts ==="
cat packages/graphql/src/index.ts
echo ""
echo "=== Checking packages/graphql/src/graphql.ts ==="
cat packages/graphql/src/graphql.ts
echo ""
echo "=== Checking what type getWatchExperience returns in content.ts and how it flows ==="
cat -n apps/web/src/lib/content.ts | tail -30
echo ""
echo "=== Checking page component to see how Section is used ==="
head -50 apps/web/src/app/\[slug\]/\[locale\]/page.tsxRepository: JesusFilm/forge
Length of output: 3157
🏁 Script executed:
#!/bin/bash
echo "=== Checking the full structure of sections/index.tsx ==="
wc -l apps/web/src/components/sections/index.tsx
head -100 apps/web/src/components/sections/index.tsx
echo ""
echo "=== Verifying what type experiences.sections has by looking at query structure ==="
echo "The GET_WATCH_EXPERIENCE query has fragments for all 4 section types,"
echo "so the generated type from ResultOf will be a union of those 4 types."
echo ""
echo "Generated sections type should be something like:"
echo " Array<"
echo " | ComponentSectionsMediaCollection"
echo " | ComponentSectionsPromoBanner"
echo " | ComponentSectionsInfoBlocks"
echo " | ComponentSectionsCta"
echo " | Error"
echo " | null"
echo " >"
</parameter>Repository: JesusFilm/forge
Length of output: 3439
🏁 Script executed:
#!/bin/bash
echo "=== Checking if WatchExperience type is exported from content.ts ==="
grep "export" apps/web/src/lib/content.ts
echo ""
echo "=== Verifying type derivation would work ==="
echo "The correct approach would be to derive Section from WatchExperience:"
echo ""
echo "Option 1: Export from content.ts and import in sections/index.tsx"
echo " export type WatchExperience = WatchData['experiences'][number]"
echo " export type Section = NonNullable<WatchExperience['sections']>[number]"
echo ""
echo "Option 2: Derive inline in sections/index.tsx"
echo " import { type WatchExperience } from '@/lib/content'"
echo " type Section = NonNullable<WatchExperience['sections']>[number]"
</parameter>Repository: JesusFilm/forge
Length of output: 894
🏁 Script executed:
#!/bin/bash
echo "=== Checking if sections field in WatchExperience matches the manual Section type ==="
echo "From the GET_WATCH_EXPERIENCE query, the sections field contains:"
echo "- __typename (discriminator)"
echo "- All fields queried in each fragment"
echo ""
echo "The manual Section type defines the same structure, so deriving it from"
echo "WatchExperience would ensure they stay synchronized."
</parameter>Repository: JesusFilm/forge
Length of output: 526
Extract Section type from generated GraphQL types instead of hand-rolling it.
The Section union type is a manual reimplementation of what gql.tada automatically generates from the GET_WATCH_EXPERIENCE query fragments. This violates the coding guideline: "Do not handwrite API client logic duplicated from generated clients."
The generated WatchExperience type (derived from ResultOf<typeof GET_WATCH_EXPERIENCE>) already provides the exact union of the four section variants. The as Section casts in page components are only necessary because the manually-defined type doesn't match the generated one.
Refactor by exporting WatchExperience from content.ts and deriving Section from it:
// In content.ts
export type WatchExperience = WatchData["experiences"][number]
// In sections/index.tsx
import type { WatchExperience } from "@/lib/content"
type Section = NonNullable<WatchExperience["sections"]>[number]This eliminates the duplicate type definition and automatically keeps it synchronized with schema changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/sections/index.tsx` around lines 7 - 60, The file
defines a hand-rolled Section union that duplicates the generated GraphQL types;
instead export the generated WatchExperience type from content.ts (the type
derived from the GET_WATCH_EXPERIENCE result) and in
apps/web/src/components/sections/index.tsx remove the manual Section definition
and import WatchExperience, then derive Section from WatchExperience.sections
(use a NonNullable/array-element extract) and remove any unnecessary "as
Section" casts in consuming components so the code uses the single
source-of-truth WatchExperience type.
Comment on lines
+84
to
+91
| {ctaLink && ( | ||
| <a | ||
| href={ctaLink} | ||
| className="mt-4 inline-block font-medium text-blue-600 hover:underline" | ||
| > | ||
| View all | ||
| </a> | ||
| )} |
There was a problem hiding this comment.
Hardcoded "View all" is not i18n-aware or customizable.
The label is not derived from content or props. Consider accepting a ctaLabel prop from MediaCollectionProps (with an optional fallback) so CMS-driven label text can flow through.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/sections/MediaCollection.tsx` around lines 84 - 91,
The "View all" string in MediaCollection is hardcoded; update
MediaCollectionProps to accept an optional ctaLabel?: string, use that prop in
the JSX in place of the literal "View all" with a fallback like ctaLabel ??
"View all", and ensure any call sites or CMS wiring pass the CMS-driven label
into the ctaLabel prop when ctaLink is provided; update the MediaCollection
component signature and its prop type/interface to include ctaLabel and replace
the hardcoded text with the prop + fallback.
Comment on lines
+25
to
+30
| <a | ||
| href={ctaLink} | ||
| className="inline-block rounded bg-blue-600 px-6 py-3 font-medium text-white hover:bg-blue-700" | ||
| > | ||
| Learn more | ||
| </a> |
There was a problem hiding this comment.
Hardcoded CTA label "Learn more" is not locale-aware.
The rest of the page is locale-driven (via Strapi sections and URL locale param), but the CTA label is a fixed English string. Consider adding an optional ctaLabel field to PromoBannerProps (and fetching it in the GraphQL fragment) and falling back to a translated default, so the label can be customised per-locale in Strapi.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/sections/PromoBanner.tsx` around lines 25 - 30, The
CTA label is hardcoded in the PromoBanner component; update PromoBannerProps to
add an optional ctaLabel string, update the GraphQL fragment that feeds this
component to request ctaLabel from Strapi, and change the anchor to use
props.ctaLabel || t('learn_more') (or your app's translation helper) as the
visible text so it falls back to a locale-aware default when Strapi doesn't
provide a custom label. Ensure you export/accept ctaLabel in the PromoBanner
function signature and update any callers that construct PromoBannerProps if
needed.
|
|
||
| export function enrichMediaItem(item: MediaItem): EnrichedMediaItem { | ||
| const title = item.titleOverride ?? item.video?.title ?? "" | ||
| const subtitle = item.subtitleOverride ?? item.video?.slug ?? "" |
There was a problem hiding this comment.
subtitle falls back to video?.slug, which renders a raw URL slug as user-facing display text.
When subtitleOverride is absent, subtitle (and videoSlug) both resolve to video.slug — a machine-readable string like "jesus-film-project". If shown in the UI, this looks incorrect to users. Either fall back to "" or introduce a dedicated display field.
🐛 Proposed fix (conservative fallback)
- const subtitle = item.subtitleOverride ?? item.video?.slug ?? ""
+ const subtitle = item.subtitleOverride ?? ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const subtitle = item.subtitleOverride ?? item.video?.slug ?? "" | |
| const subtitle = item.subtitleOverride ?? "" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/enrichment.ts` at line 23, The subtitle currently falls back
to the machine slug (item.video?.slug) which is user-facing; change the fallback
so subtitle uses item.subtitleOverride ?? "" (do not expose item.video?.slug
directly) and ensure any related variable videoSlug remains the raw slug for
programmatic use only (add a separate display field like displaySubtitle or keep
subtitle empty when override is absent). Update the assignment for subtitle and,
if present, remove usages that render videoSlug to the UI so only the
displaySubtitle/subtitle (empty or override) is shown.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/app/page.tsx (1)
20-34: Extract the repeated rendering block into a shared component.The error-guard → empty-guard → typed section map pattern (including the
as Array<Section | { __typename: "Error" } | null>cast) is copy-pasted identically in all three page files (page.tsx,[param]/page.tsx,[slug]/[locale]/page.tsx). A single<ExperienceView>server component would remove the duplication and centralise the type cast:// apps/web/src/components/ExperienceView.tsx import { SectionRenderer, type Section } from "@/components/sections" import { ExperienceEmpty } from "@/components/ExperienceEmpty" import { ExperienceError } from "@/components/ExperienceError" import type { WatchExperienceResult } from "@/lib/content" export function ExperienceView({ result }: { result: WatchExperienceResult }) { if (result.error) return <ExperienceError message={result.error.message} /> const experience = result.data if (!experience?.sections?.length) return <ExperienceEmpty /> return ( <main className="min-h-screen"> {(experience.sections as Array<Section | { __typename: "Error" } | null>).map( (section, i) => { if (!section || ("__typename" in section && section.__typename === "Error")) return null const key = "id" in section ? section.id : `section-${i}` return <SectionRenderer key={key} section={section as Section} /> }, )} </main> ) }Each page then reduces to:
export default async function HomePage() { const locale = await getLocale() const result = await getWatchExperience(locale) return <ExperienceView result={result} /> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/page.tsx` around lines 20 - 34, Extract the repeated rendering logic into a new server component ExperienceView that accepts the WatchExperienceResult (prop name result), moves the error and empty guards there (using ExperienceError and ExperienceEmpty), and centralises the type cast and map that renders SectionRenderer; then update page.tsx, [param]/page.tsx and [slug]/[locale]/page.tsx to call getWatchExperience/getLocale as before and simply return <ExperienceView result={result} />. Ensure ExperienceView exposes the same behavior by checking result.error, using const experience = result.data, guarding against !experience?.sections?.length, and mapping (experience.sections as Array<Section | { __typename: "Error" } | null>) with the existing null/ "__typename" error guard and key logic (use "id" in section ? section.id : `section-${i}`) so no rendering changes occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/`[slug]/[locale]/page.tsx:
- Around line 11-12: Validate the locale URL segment before passing it to
getWatchExperience: use the existing isLocale(...) check (or compare against
SUPPORTED_LOCALES) to determine a safe locale value (e.g., const safeLocale =
isLocale(locale) ? locale : 'en') and pass safeLocale to getWatchExperience
instead of the raw params.locale so Strapi receives a valid locale and the page
falls back to English when the URL segment is invalid.
In `@apps/web/src/lib/locale.ts`:
- Around line 17-19: getLocale currently returns the raw primary language tag
from the Accept-Language header without validation; update getLocale to validate
the extracted primary value using isLocale(primary) (or check against
SUPPORTED_LOCALES) and only return it when valid, otherwise return the app
default/fallback locale (e.g., DEFAULT_LOCALE or SUPPORTED_LOCALES[0]) so
downstream functions like getWatchExperience won't receive unsupported tags that
lead to ExperienceEmpty.
---
Nitpick comments:
In `@apps/web/src/app/page.tsx`:
- Around line 20-34: Extract the repeated rendering logic into a new server
component ExperienceView that accepts the WatchExperienceResult (prop name
result), moves the error and empty guards there (using ExperienceError and
ExperienceEmpty), and centralises the type cast and map that renders
SectionRenderer; then update page.tsx, [param]/page.tsx and
[slug]/[locale]/page.tsx to call getWatchExperience/getLocale as before and
simply return <ExperienceView result={result} />. Ensure ExperienceView exposes
the same behavior by checking result.error, using const experience =
result.data, guarding against !experience?.sections?.length, and mapping
(experience.sections as Array<Section | { __typename: "Error" } | null>) with
the existing null/ "__typename" error guard and key logic (use "id" in section ?
section.id : `section-${i}`) so no rendering changes occur.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/env.ts (1)
10-10: Prefer the Zod v4 standalonez.url()schema for URL validation.The official
@t3-oss/env-nextjsdocs now showDATABASE_URL: z.url()as the idiomatic pattern — using the new Zod v4 standalone format instead of the Zod v3-stylez.string().url()chained refinement.✨ Suggested update
- NEXT_PUBLIC_GRAPHQL_URL: z.string().url().optional(), + NEXT_PUBLIC_GRAPHQL_URL: z.url().optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/env.ts` at line 10, Update the NEXT_PUBLIC_GRAPHQL_URL schema to use Zod v4 standalone URL schema instead of the chained refinement; replace the current z.string().url().optional() usage in the NEXT_PUBLIC_GRAPHQL_URL declaration with z.url().optional() (keeping the optional() call) so the env schema uses z.url() for proper URL validation; ensure the file still imports z from "zod" and run type checks to confirm compatibility with the rest of the env parsing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/lib/client.ts`:
- Around line 2-4: The NEXT_PUBLIC_GRAPHQL_URL fallback to
"http://localhost:1337/graphql" in the client (variable uri) hides missing
production config; update the zod schema in env.ts to make
NEXT_PUBLIC_GRAPHQL_URL required (remove .optional()) and remove the localhost
fallback in apps/web/src/lib/client.ts so uri is taken directly from
env.NEXT_PUBLIC_GRAPHQL_URL; if a local default is needed for development, set
it in .env.local or .env.example rather than in code.
---
Nitpick comments:
In `@apps/web/src/env.ts`:
- Line 10: Update the NEXT_PUBLIC_GRAPHQL_URL schema to use Zod v4 standalone
URL schema instead of the chained refinement; replace the current
z.string().url().optional() usage in the NEXT_PUBLIC_GRAPHQL_URL declaration
with z.url().optional() (keeping the optional() call) so the env schema uses
z.url() for proper URL validation; ensure the file still imports z from "zod"
and run type checks to confirm compatibility with the rest of the env parsing
logic.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…ePatterns, sanitize errors Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
Review feedback addressed (cc07276, ed68cb1)Fixed:
Not changed:
|
This was referenced Feb 18, 2026
8 tasks
Kneesal
pushed a commit
to Kneesal/forge
that referenced
this pull request
Mar 9, 2026
…lm#63) Co-authored-by: Cursor <cursoragent@cursor.com>
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
1 participant
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.
Resolves #41
Summary
Watch page renders sections from Strapi via Apollo Client. Locale resolved from URL, header, or default
en. Section components: MediaCollection, PromoBanner, InfoBlocks, CTASection. Enrichment uses video fields when override null. Empty/error states when no Experience or query fails. Routes:/watchand/watch/[locale].Contracts Changed
Regeneration Required
Validation
Summary by CodeRabbit