feat(web,cms): video hero, experience blocks rename, easter seed#301
Merged
Kneesal merged 12 commits intoJesusFilm:mainfrom Mar 9, 2026
Merged
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
- CMS: rename Experience sections to blocks; add streamingUrl to video-hero - CMS: seed script (pnpm seed) for Easter experience + Video Hero block - Web: VideoHero component (video.js), GET_WATCH_EXPERIENCE with blocks - Web: ExperienceSectionRenderer for blocks; typedRoutes in next.config - Web: 401 error copy and ExperienceError known messages - GraphQL + mobile: WatchExperience blocks type and usage - Docs: DEV-COMMANDS.md, VIDEO-HERO-MANUAL-SETUP.md, package seed script Resolves JesusFilm#164 Made-with: Cursor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRenames Experience dynamic zone from sections → blocks across CMS, web, mobile, and GraphQL types; adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Web Client
participant Web as Next.js App
participant CMS as Strapi GraphQL
participant CDN as Video CDN (HLS)
Client->>Web: GET /watch/:slug
Web->>CMS: GraphQL query for Experience (blocks) + fragments (VideoHero)
CMS-->>Web: Experience.blocks (includes VideoHero with streamingUrl)
Web-->>Client: Render page HTML + VideoHero shell
Client->>CDN: Video.js requests streamingUrl (HLS)
CDN-->>Client: HLS manifest/segments
Client->>Client: Video.js playback, scroll/mute controls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
Made-with: Cursor
Kneesal
commented
Mar 9, 2026
- Remove DEV-COMMANDS.md and VIDEO-HERO-MANUAL-SETUP.md - Revert mobile/ios lint script to swiftlint lint - Revert next.config to experimental.typedRoutes; keep images - Revert next-env.d.ts to reference path for routes - Revert env.ts STRAPI_API_TOKEN to z.string() - Revert client.ts to uri/headers variables style Made-with: Cursor
Member
Author
Review feedback addressed (9388849)Fixed:
|
Made-with: Cursor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/graphql/src/watchExperience.ts (1)
15-69:⚠️ Potential issue | 🔴 CriticalFetch
ComponentSectionsVideoHeroin this blocks query.The renderer in
apps/web/src/components/sections/index.tsxnow switches onComponentSectionsVideoHero, but this query never selects that union member. A seeded Easter experience with a video hero will therefore come back with at most__typename, so/watch/eastercannot render the hero content orstreamingUrl.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/graphql/src/watchExperience.ts` around lines 15 - 69, The blocks selection in watchExperience.ts is missing the union member ComponentSectionsVideoHero, so queries for experiences with a video hero return only __typename; add a new inline fragment for ComponentSectionsVideoHero under blocks (matching the same pattern as other fragments) and select the renderer-used fields such as id, title/heading fields, subtitle/body, image (url), streamingUrl (and any video/document fields the renderer expects) so the renderer in apps/web/src/components/sections/index.tsx receives full hero content.apps/cms/src/api/experience/content-types/experience/schema.json (1)
94-116:⚠️ Potential issue | 🔴 CriticalFix mobile client field access:
experience.sections→experience.blocks.The CMS schema renamed the dynamic zone from
sectionstoblocks(line 94), and GraphQL queries were correctly updated to fetchblocks. However, mobile client implementations still try to accessexperience.sections, which no longer exists:
mobile/ios/Sources/ForgeMobile/Data/GraphQLContentClient.swift: accessesexperience.sectionsmobile/android/app/src/main/kotlin/com/forge/mobile/GraphQLContentClient.kt: accessesexperience.sectionsBoth iOS and Android apps will fail at runtime when trying to access the renamed field. Update these files to use
blocksinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cms/src/api/experience/content-types/experience/schema.json` around lines 94 - 116, The mobile clients still access the renamed dynamic zone field experience.sections which was changed to experience.blocks in the CMS schema; update both GraphQLContentClient.swift and GraphQLContentClient.kt to read the blocks field instead of sections, including any JSON/GQL parsing keys, property names, decoder/mapper logic and any model classes or accessors (e.g., change occurrences of experience.sections, Sections or related mapping methods to experience.blocks, Blocks or corresponding accessor names) so the GraphQL response mapping and runtime access use "blocks" consistently on iOS (GraphQLContentClient.swift) and Android (GraphQLContentClient.kt).
🧹 Nitpick comments (5)
apps/web/src/components/ExperienceError.tsx (1)
8-11: Consider using user-friendly messages without internal setup instructions.The error messages expose implementation details (
.env.localpath, Strapi Admin navigation) that are developer-facing. In production, end users seeing these would be confused and it leaks internal infrastructure information.Consider either:
- Showing a generic user-facing message like "Configuration error. Please contact support."
- Conditionally showing detailed messages only in development mode.
💡 Example: Generic user-facing message
"Response not successful: Received status code 401": - "Invalid or missing API token. Set STRAPI_API_TOKEN in apps/web/.env.local (create token in Strapi Admin → Settings → API Tokens).", + "Unable to authenticate with the content service. Please contact support if this persists.", "Missing or invalid credentials": - "Invalid or missing API token. Set STRAPI_API_TOKEN in apps/web/.env.local (create token in Strapi Admin → Settings → API Tokens).", + "Unable to authenticate with the content service. Please contact support if this persists.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ExperienceError.tsx` around lines 8 - 11, Update the user-facing error strings in ExperienceError (the mapping entries for "Response not successful: Received status code 401" and "Missing or invalid credentials") to avoid exposing internal setup details; replace the current ".env.local / Strapi Admin" instructions with a generic message like "Configuration error. Please contact support." and, if you want dev details, conditionally append the original detailed guidance only when running in development (check NODE_ENV or a runtime isDevelopment flag) so production users never see infrastructure paths.mobile/android/app/src/main/graphql/ExperienceBySlug.graphql (2)
2-2: Stale comment references "section variants".The comment still says "all section variants" but the field was renamed to
blocks. Update for consistency.📝 Suggested fix
# Android-owned operation — mirrors web query intent, NOT shared with iOS. -# Fetches a single Experience by slug with all section variants. +# Fetches a single Experience by slug with all block variants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/android/app/src/main/graphql/ExperienceBySlug.graphql` at line 2, Update the top-line comment to reflect the renamed field: replace "section variants" with "blocks" so the comment reads that it fetches a single Experience by slug with all blocks; edit the comment near the ExperienceBySlug GraphQL definition (the header comment above the query) to reference "blocks" instead of "section variants" to keep documentation consistent with the schema change.
11-73: VideoHero fragment missing in Android query.The web query includes
... on ComponentSectionsVideoHero(lines 53–54 inapps/web/src/lib/content.ts), but the AndroidExperienceBySlug.graphqldoes not. If Android should support VideoHero content, add the fragment; otherwise, this can be deferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/android/app/src/main/graphql/ExperienceBySlug.graphql` around lines 11 - 73, The Android GraphQL query ExperienceBySlug.graphql is missing the ComponentSectionsVideoHero fragment so VideoHero blocks won't be fetched; add a branch inside the blocks selection matching "... on ComponentSectionsVideoHero" and include the same fields used by web (e.g., __typename, id, heading, intro, video { documentId, title, slug, image { url, alternativeText, width, height } } or whichever fields your app needs) to mirror the web implementation and ensure Android can consume VideoHero content.apps/web/src/components/sections/VideoHero.tsx (1)
39-48: Consider a scroll threshold instead of exactscrollY === 0for resuming playback.Currently, the video only resumes when the user scrolls to the absolute top (
scrollY === 0). Users who scroll partially back up won't see the video resume, which may feel unresponsive.♻️ Suggested threshold-based resume
const pauseOnScrollAway = useCallback(() => { const scrollY = window.scrollY if (playerRef.current) { if (scrollY > 100) { playerRef.current.pause() - } else if (scrollY === 0) { + } else if (scrollY < 50) { void playerRef.current.play() } } }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sections/VideoHero.tsx` around lines 39 - 48, The pauseOnScrollAway handler currently only resumes playback when scrollY === 0; change it to use a resume threshold (e.g., resume when scrollY <= 100) so partial upward scrolling restarts the video. Update the logic in pauseOnScrollAway (referenced function) to compare against a numeric threshold (or a named constant like resumeThreshold) instead of strict zero, keeping the existing pause condition (scrollY > 100) and ensuring you reference playerRef.current.pause() and playerRef.current.play() accordingly.apps/web/src/lib/content.ts (1)
145-150: The type cast for accessing GraphQL errors is pragmatic but could be typed more explicitly.The cast to
{ errors?: Array<{ message?: string }> }works but loses type safety. If Apollo Client's types already includeerrorson the result, the cast is unnecessary; if not, consider augmenting the type.That said, the error handling logic itself is correct and properly aggregates multiple error messages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/content.ts` around lines 145 - 150, The current cast (result as { errors?: Array<{ message?: string }> }) is imprecise; instead import and use the appropriate Apollo type (e.g., FetchResult or ApolloQueryResult from `@apollo/client` or `@apollo/client/core`) or add a narrow type guard so you can access errors safely; update the code around the graphqlErrors assignment (the graphqlErrors variable and the result value returned from the GraphQL call) to use the correct imported type or a small type-guard function that checks for an errors array, then keep the existing mapping logic that builds msg and returns { data: null, error: new Error(msg) } unchanged.
🤖 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/cms/src/components/sections/video-hero.json`:
- Around line 20-23: The schema currently defines "streamingUrl" as optional
which allows publishing a Video Hero without any playable source; update the
video hero schema so "streamingUrl" is required (set "required": true for the
"streamingUrl" property) unless you add and validate a real fallback playback
source (e.g., a new "fallbackUrl" or "fallbackAsset" property and logic to
ensure at least one of "streamingUrl" or the fallback is present) so editors
cannot save a hero with no playable source.
In `@apps/cms/VIDEO-HERO-MANUAL-SETUP.md`:
- Around line 36-38: Replace the hard-coded year in the example subheading to
avoid stale content: update the "Subheading" example (the line labeled
**Subheading** in the diff) to remove the year (e.g., change "Easter 2025 —
videos & resources about Lent, Holy Week, Resurrection" to a yearless example
such as "Easter — videos & resources about Lent, Holy Week, Resurrection") so
editors won't publish an outdated year on /watch/easter.
In `@apps/web/next-env.d.ts`:
- Line 3: Remove the manual import statement `import
"./.next/dev/types/routes.d.ts"` from the auto-generated next-env.d.ts file;
next-env.d.ts is regenerated by Next.js and should not be edited manually, and
route types are already provided via tsconfig.json includes
(`.next/types/**/*.ts` and `.next/dev/types/**/*.ts`), so delete that import to
avoid committing generated changes.
In `@apps/web/package.json`:
- Around line 21-28: The repository's package.json now adds dependencies
("video.js" and "zod") but the pnpm lockfile wasn't updated, causing CI to fail
with pnpm install --frozen-lockfile; regenerate the pnpm lockfile locally (run
pnpm install) so pnpm-lock.yaml includes the new entries, verify the changes,
and commit the updated pnpm-lock.yaml alongside the package.json changes so CI
can install reproducibly.
In `@apps/web/src/app/`[slug]/page.tsx:
- Around line 23-30: The early emptiness check uses experience.blocks which may
include null/Error entries; first filter invalid blocks with the existing filter
(the const blocks = experience.blocks.filter(...) that narrows to Section) and
then check if blocks.length is zero to return <ExperienceEmpty />; update the
flow in page.tsx so the filtered "blocks" (not experience.blocks) are used for
the emptiness guard and subsequent rendering (keep the Section type guard and
ExperienceEmpty return).
In `@apps/web/src/components/sections/index.tsx`:
- Around line 22-24: The switch default case in the sections renderer currently
does "return null" for unhandled CMS block types which silently drops content;
change the default branch in the switch inside components/sections/index.tsx to
surface an explicit signal: log a warning (including the block type/identifier)
via the existing logger or console.warn and render a visible
placeholder/fallback React element that indicates an unsupported block (so
missing content is detectable), rather than returning null. Update the default
case handling where "return null" appears and include the block's type/id in the
warning to aid debugging.
In `@apps/web/src/env.ts`:
- Line 6: The new env schema requires STRAPI_API_TOKEN to be non-empty; update
the example environment template and setup docs so they no longer include an
empty assignment for STRAPI_API_TOKEN and instead either provide a placeholder
value (e.g., STRAPI_API_TOKEN=your_token_here) or document that users must set
this variable before startup; change the example env file and any README/setup
docs that reference STRAPI_API_TOKEN to reflect the new requirement and include
a short note on how to obtain or configure the token.
In `@DEV-COMMANDS.md`:
- Line 3: Update the sentence "From repo root (`qyi`). Install first: `pnpm
install`." in DEV-COMMANDS.md to remove the stray directory name or replace
`qyi` with the actual repository directory name; specifically edit that line so
it reads either "From repo root. Install first: `pnpm install`." or "From the
repo root (replace-with-repo-name). Install first: `pnpm install`." to avoid
confusion.
In `@mobile/android/app/src/main/graphql/ExperienceBySlug.graphql`:
- Line 11: Update GraphQLContentClient.kt to use the regenerated GraphQL
property name `blocks` instead of `sections`: replace both occurrences of
`experience.sections?.mapNotNull` with `experience.blocks?.mapNotNull` (these
appear in the methods that build/transform experience content in
GraphQLContentClient.kt), ensuring any downstream variables/names expecting
`sections` are updated to `blocks` as well so the code compiles with the new
Apollo-generated types.
In `@mobile/ios/GraphQL/Operations/GetWatchExperience.graphql`:
- Line 9: The GraphQL operation was changed to use the field `blocks` but the
generated Swift query and mapper still reference `sections`, causing a
selection-set mismatch; regenerate the Apollo iOS artifacts for the
GetWatchExperience operation (re-run the Apollo codegen/cli or the build phase
that generates GraphQL files) so the generated query class/struct and the mapper
use `blocks` instead of `sections`, and then verify/update any references to the
generated types (e.g., GetWatchExperienceQuery, its selectionSet/mapping code,
and any mapper helper) to match the new `blocks` property.
In `@mobile/ios/package.json`:
- Line 5: The current "lint" npm script in package.json is masking a missing
SwiftLint binary by returning success; change the "lint" script (the "lint"
entry) to require SwiftLint and exit non-zero when it's not present (so CI/local
runs fail if SwiftLint is missing), and add a new optional script (e.g.,
"lint:optional" or "lint:local") that performs the conditional check and echoes
the skip message for convenience. Update only the "lint" and add the optional
script entries so linting behavior matches other packages and CI remains strict.
---
Outside diff comments:
In `@apps/cms/src/api/experience/content-types/experience/schema.json`:
- Around line 94-116: The mobile clients still access the renamed dynamic zone
field experience.sections which was changed to experience.blocks in the CMS
schema; update both GraphQLContentClient.swift and GraphQLContentClient.kt to
read the blocks field instead of sections, including any JSON/GQL parsing keys,
property names, decoder/mapper logic and any model classes or accessors (e.g.,
change occurrences of experience.sections, Sections or related mapping methods
to experience.blocks, Blocks or corresponding accessor names) so the GraphQL
response mapping and runtime access use "blocks" consistently on iOS
(GraphQLContentClient.swift) and Android (GraphQLContentClient.kt).
In `@packages/graphql/src/watchExperience.ts`:
- Around line 15-69: The blocks selection in watchExperience.ts is missing the
union member ComponentSectionsVideoHero, so queries for experiences with a video
hero return only __typename; add a new inline fragment for
ComponentSectionsVideoHero under blocks (matching the same pattern as other
fragments) and select the renderer-used fields such as id, title/heading fields,
subtitle/body, image (url), streamingUrl (and any video/document fields the
renderer expects) so the renderer in apps/web/src/components/sections/index.tsx
receives full hero content.
---
Nitpick comments:
In `@apps/web/src/components/ExperienceError.tsx`:
- Around line 8-11: Update the user-facing error strings in ExperienceError (the
mapping entries for "Response not successful: Received status code 401" and
"Missing or invalid credentials") to avoid exposing internal setup details;
replace the current ".env.local / Strapi Admin" instructions with a generic
message like "Configuration error. Please contact support." and, if you want dev
details, conditionally append the original detailed guidance only when running
in development (check NODE_ENV or a runtime isDevelopment flag) so production
users never see infrastructure paths.
In `@apps/web/src/components/sections/VideoHero.tsx`:
- Around line 39-48: The pauseOnScrollAway handler currently only resumes
playback when scrollY === 0; change it to use a resume threshold (e.g., resume
when scrollY <= 100) so partial upward scrolling restarts the video. Update the
logic in pauseOnScrollAway (referenced function) to compare against a numeric
threshold (or a named constant like resumeThreshold) instead of strict zero,
keeping the existing pause condition (scrollY > 100) and ensuring you reference
playerRef.current.pause() and playerRef.current.play() accordingly.
In `@apps/web/src/lib/content.ts`:
- Around line 145-150: The current cast (result as { errors?: Array<{ message?:
string }> }) is imprecise; instead import and use the appropriate Apollo type
(e.g., FetchResult or ApolloQueryResult from `@apollo/client` or
`@apollo/client/core`) or add a narrow type guard so you can access errors safely;
update the code around the graphqlErrors assignment (the graphqlErrors variable
and the result value returned from the GraphQL call) to use the correct imported
type or a small type-guard function that checks for an errors array, then keep
the existing mapping logic that builds msg and returns { data: null, error: new
Error(msg) } unchanged.
In `@mobile/android/app/src/main/graphql/ExperienceBySlug.graphql`:
- Line 2: Update the top-line comment to reflect the renamed field: replace
"section variants" with "blocks" so the comment reads that it fetches a single
Experience by slug with all blocks; edit the comment near the ExperienceBySlug
GraphQL definition (the header comment above the query) to reference "blocks"
instead of "section variants" to keep documentation consistent with the schema
change.
- Around line 11-73: The Android GraphQL query ExperienceBySlug.graphql is
missing the ComponentSectionsVideoHero fragment so VideoHero blocks won't be
fetched; add a branch inside the blocks selection matching "... on
ComponentSectionsVideoHero" and include the same fields used by web (e.g.,
__typename, id, heading, intro, video { documentId, title, slug, image { url,
alternativeText, width, height } } or whichever fields your app needs) to mirror
the web implementation and ensure Android can consume VideoHero content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02852911-a3cd-47a9-b2df-cb2e333749ce
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
DEV-COMMANDS.mdapps/cms/VIDEO-HERO-MANUAL-SETUP.mdapps/cms/package.jsonapps/cms/schema.graphqlapps/cms/scripts/seed-easter.cjsapps/cms/src/api/experience/content-types/experience/schema.jsonapps/cms/src/components/sections/video-hero.jsonapps/web/next-env.d.tsapps/web/next.config.mjsapps/web/package.jsonapps/web/src/app/[slug]/[locale]/page.tsxapps/web/src/app/[slug]/page.tsxapps/web/src/app/page.tsxapps/web/src/app/watch/easter/page.tsxapps/web/src/components/ExperienceError.tsxapps/web/src/components/sections/VideoHero.tsxapps/web/src/components/sections/index.tsxapps/web/src/components/sections/videoHeroFragment.tsapps/web/src/env.tsapps/web/src/lib/client.tsapps/web/src/lib/content.tsmobile/android/app/src/main/graphql/ExperienceBySlug.graphqlmobile/expo/src/lib/experienceService.test.tsmobile/expo/src/lib/experienceService.tsmobile/expo/src/lib/graphql/queries.tsmobile/ios/GraphQL/Operations/GetWatchExperience.graphqlmobile/ios/package.jsonpackage.jsonpackages/graphql/src/graphql-env.d.tspackages/graphql/src/index.tspackages/graphql/src/watchExperience.ts
Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/web/src/lib/experience-metadata.ts (1)
24-30: Consider using a self-hosted default OG image.The default OG image points to an external Unsplash URL. If Unsplash becomes unavailable or rate-limits requests, social previews will break. Consider hosting a default image on
jesusfilm.orgor a CDN you control for reliability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/experience-metadata.ts` around lines 24 - 30, DEFAULT_OG_IMAGE currently references an external Unsplash URL which can be rate-limited or unavailable; update the DEFAULT_OG_IMAGE constant to point to a self-hosted or controlled CDN URL (e.g., an image served from jesusfilm.org or our CDN), keeping the same shape (url, width, height, alt, type) and preserving the "image/jpeg" as const typing so clients consuming DEFAULT_OG_IMAGE (the constant symbol) continue to work.apps/web/src/components/sections/VideoHero.tsx (2)
55-74: Callback dependencies may cause unintended player recreation.The
useEffectdepends ononMutedChangeandonPlayerReady. If the parent doesn't memoize these callbacks (which it does viauseCallbackon lines 155-161), changing parent state would dispose and recreate the player. The current implementation is correct, but this is fragile — consider documenting this requirement or using refs for callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sections/VideoHero.tsx` around lines 55 - 74, The effect is fragile because it re-runs when parent-provided callbacks change; change the component to store onMutedChange and onPlayerReady in refs and use the refs inside the effect so the effect only depends on src (and videoRef/playerRef), or alternatively document that those callbacks must be memoized; specifically create refs like onMutedChangeRef and onPlayerReadyRef, update them on each render, call onPlayerReadyRef.current(player) after creating the player, and inside player.on("volumechange") call onMutedChangeRef.current(player.muted() ?? true) so the player is not disposed/recreated when parent callbacks change.
39-48: Throttle scroll handler to avoid performance issues.The
pauseOnScrollAwaycallback fires on every scroll event without throttling. On pages with heavy scroll activity, this can cause performance degradation.♻️ Proposed throttle implementation
+import { useCallback, useEffect, useRef, useState, useMemo } from "react" + +function throttle<T extends (...args: unknown[]) => void>(fn: T, ms: number): T { + let lastCall = 0 + return ((...args) => { + const now = Date.now() + if (now - lastCall >= ms) { + lastCall = now + fn(...args) + } + }) as T +} // Inside VideoHeroPlayer: - const pauseOnScrollAway = useCallback(() => { + const pauseOnScrollAway = useMemo(() => throttle(() => { const scrollY = window.scrollY if (playerRef.current) { if (scrollY > 100) { playerRef.current.pause() - } else if (scrollY === 0) { + } else if (scrollY <= 10) { void playerRef.current.play() } } - }, []) + }, 100), [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/sections/VideoHero.tsx` around lines 39 - 48, The scroll handler pauseOnScrollAway currently runs on every scroll event; wrap it with a throttle (or use requestAnimationFrame batching) and wire the throttled function as the window 'scroll' listener with proper cleanup to avoid performance issues. Concretely, create a stable throttled handler (e.g., using lodash.throttle or an rAF-based ref/ticking pattern) that calls pauseOnScrollAway logic, ensure the throttled function is memoized or stored in a ref so addEventListener/removeEventListener use the same reference, and add/remove the listener inside a useEffect; reference pauseOnScrollAway, playerRef, and the throttled handler when implementing these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/components/sections/VideoHero.tsx`:
- Around line 55-74: The effect is fragile because it re-runs when
parent-provided callbacks change; change the component to store onMutedChange
and onPlayerReady in refs and use the refs inside the effect so the effect only
depends on src (and videoRef/playerRef), or alternatively document that those
callbacks must be memoized; specifically create refs like onMutedChangeRef and
onPlayerReadyRef, update them on each render, call
onPlayerReadyRef.current(player) after creating the player, and inside
player.on("volumechange") call onMutedChangeRef.current(player.muted() ?? true)
so the player is not disposed/recreated when parent callbacks change.
- Around line 39-48: The scroll handler pauseOnScrollAway currently runs on
every scroll event; wrap it with a throttle (or use requestAnimationFrame
batching) and wire the throttled function as the window 'scroll' listener with
proper cleanup to avoid performance issues. Concretely, create a stable
throttled handler (e.g., using lodash.throttle or an rAF-based ref/ticking
pattern) that calls pauseOnScrollAway logic, ensure the throttled function is
memoized or stored in a ref so addEventListener/removeEventListener use the same
reference, and add/remove the listener inside a useEffect; reference
pauseOnScrollAway, playerRef, and the throttled handler when implementing these
changes.
In `@apps/web/src/lib/experience-metadata.ts`:
- Around line 24-30: DEFAULT_OG_IMAGE currently references an external Unsplash
URL which can be rate-limited or unavailable; update the DEFAULT_OG_IMAGE
constant to point to a self-hosted or controlled CDN URL (e.g., an image served
from jesusfilm.org or our CDN), keeping the same shape (url, width, height, alt,
type) and preserving the "image/jpeg" as const typing so clients consuming
DEFAULT_OG_IMAGE (the constant symbol) continue to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c121d1fb-e7fe-40ed-8e88-5ba4c760413e
📒 Files selected for processing (6)
apps/web/src/app/[slug]/[locale]/page.tsxapps/web/src/app/[slug]/page.tsxapps/web/src/app/watch/easter/metadata.tsapps/web/src/app/watch/easter/page.tsxapps/web/src/components/sections/VideoHero.tsxapps/web/src/lib/experience-metadata.ts
💤 Files with no reviewable changes (2)
- apps/web/src/app/watch/easter/page.tsx
- apps/web/src/app/watch/easter/metadata.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/[slug]/page.tsx
…(review) Made-with: Cursor
…eview) Made-with: Cursor
Member
Author
Review feedback addressed (b3768ec, b496a60)Fixed:
Not changed:
|
Kneesal
force-pushed
the
feat/video-hero-and-experience-blocks
branch
from
b496a60 to
0c75c74
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mobile/android/app/src/main/kotlin/com/forge/mobile/GraphQLContentClient.kt (1)
47-63:⚠️ Potential issue | 🟠 MajorHandle
ComponentSectionsVideoHeroin the Android block mapper.
experience.blocksnow includesComponentSectionsVideoHero, but this summary logic still only reads CTA/PromoBanner/InfoBlocks. For a VideoHero-only experience—like the Easter content added in this PR—Android will fall back toslugand an empty body becausemobile/android/app/src/main/graphql/ExperienceBySlug.graphqlalso omits that inline fragment. Please add the VideoHero selection to the query and map its text fields here so the new block type produces usable content on Android.Based on learnings: When GraphQL schema changes, update Kotlin call sites to match regenerated types in
app/build/generated/source/apollo/forge/com/forge/mobile/graphql/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/android/app/src/main/kotlin/com/forge/mobile/GraphQLContentClient.kt` around lines 47 - 63, The block mapper is missing handling for the new ComponentSectionsVideoHero so VideoHero-only experiences get empty title/body; update mobile/android/app/src/main/graphql/ExperienceBySlug.graphql to include the inline fragment for ComponentSectionsVideoHero (select its text fields such as heading, body, description as appropriate), regenerate the Apollo Kotlin models, then in GraphQLContentClient.kt add mapping for experience.blocks?.mapNotNull to check section?.onComponentSectionsVideoHero and return its heading for title and its body/description for body alongside the existing onComponentSectionsCta/onComponentSectionsPromoBanner/onComponentSectionsInfoBlocks checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@mobile/android/app/src/main/kotlin/com/forge/mobile/GraphQLContentClient.kt`:
- Around line 47-63: The block mapper is missing handling for the new
ComponentSectionsVideoHero so VideoHero-only experiences get empty title/body;
update mobile/android/app/src/main/graphql/ExperienceBySlug.graphql to include
the inline fragment for ComponentSectionsVideoHero (select its text fields such
as heading, body, description as appropriate), regenerate the Apollo Kotlin
models, then in GraphQLContentClient.kt add mapping for
experience.blocks?.mapNotNull to check section?.onComponentSectionsVideoHero and
return its heading for title and its body/description for body alongside the
existing
onComponentSectionsCta/onComponentSectionsPromoBanner/onComponentSectionsInfoBlocks
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e99e2574-ee7e-4b36-a00a-41358613a0f2
📒 Files selected for processing (5)
apps/cms/src/components/sections/video-hero.jsonapps/web/src/components/ExperienceError.tsxapps/web/src/components/sections/VideoHero.tsxmobile/android/app/src/main/graphql/ExperienceBySlug.graphqlmobile/android/app/src/main/kotlin/com/forge/mobile/GraphQLContentClient.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- mobile/android/app/src/main/graphql/ExperienceBySlug.graphql
- apps/web/src/components/ExperienceError.tsx
- apps/web/src/components/sections/VideoHero.tsx
Made-with: Cursor
Made-with: Cursor
Kneesal
force-pushed
the
feat/video-hero-and-experience-blocks
branch
from
0c75c74 to
6c9e770
Compare
…ection type Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/web/src/app/[slug]/page.tsx (1)
38-44:⚠️ Potential issue | 🟠 MajorUse the filtered block list for the empty-state guard.
This still treats
nullandErrorentries as renderable content, so an experience with no valid blocks falls through to a blank page. The empty-state check should run on the narrowedblocksarray instead.Suggested fix
- const experience = result.data - if (!experience?.blocks?.length) { - return <ExperienceEmpty /> - } - - const blocks = experience.blocks.filter( - (b): b is Section => b !== null && b.__typename !== "Error", - ) + const blocks = + result.data?.blocks?.filter( + (b): b is Section => b !== null && b.__typename !== "Error", + ) ?? [] + + if (blocks.length === 0) { + return <ExperienceEmpty /> + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`[slug]/page.tsx around lines 38 - 44, The empty-state check uses experience?.blocks length before filtering out null/Error items, so pages with only invalid blocks render blank; update the guard to compute the filtered array (const blocks = experience.blocks.filter((b): b is Section => b !== null && b.__typename !== "Error") ) first and then return <ExperienceEmpty /> when blocks.length === 0, and thereafter render using the filtered blocks variable (blocks) instead of experience.blocks.apps/web/src/app/[slug]/[locale]/page.tsx (1)
35-40:⚠️ Potential issue | 🟠 MajorCheck emptiness after filtering invalid blocks.
experience.blocks.lengthstill countsnullandErrorentries, so a response made entirely of invalid blocks skipsExperienceEmptyand renders a blank page instead. Filter first, then guard onblocks.length.Suggested fix
- const experience = result.data - if (!experience?.blocks?.length) { - return <ExperienceEmpty /> - } - - const blocks = experience.blocks.filter( - (b): b is Section => b !== null && b.__typename !== "Error", - ) + const blocks = + result.data?.blocks?.filter( + (b): b is Section => b !== null && b.__typename !== "Error", + ) ?? [] + + if (blocks.length === 0) { + return <ExperienceEmpty /> + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`[slug]/[locale]/page.tsx around lines 35 - 40, The emptiness check currently runs before filtering invalid entries, so null/Error items can make the page skip ExperienceEmpty; change the order: first create the filtered const blocks = experience.blocks.filter((b): b is Section => b !== null && b.__typename !== "Error"), then check if (!blocks.length) return <ExperienceEmpty />; update any references that used the pre-filtered value to use this filtered blocks variable (symbols: experience, blocks, Section, ExperienceEmpty).apps/web/src/components/sections/index.tsx (1)
25-27:⚠️ Potential issue | 🟠 MajorDon’t silently drop unsupported block types.
return nullstill turns schema/query drift into missing page content with no signal. Please make this branch exhaustive or emit a warning/fallback so unsupported CMS blocks are detectable during rollout.🤖 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 25 - 27, The default branch in the switch that handles sections currently does "return null", which hides schema/query drift; update the default case in apps/web/src/components/sections/index.tsx to surface unsupported blocks by logging a clear warning (include the block identifier like block._type or block._key / block.id) and rendering a visible, testable fallback (e.g., a placeholder element with a data attribute indicating the unsupported block type) so missing/unknown CMS blocks are detectable in QA/rollout; alternatively, make the switch exhaustive by adding explicit handlers for the missing block types if known and throw a dev-only error to fail fast during development.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mobile/expo/src/lib/graphql/queries.ts`:
- Line 13: The import of the type WatchExperienceBlock is currently placed after
the export block which violates module-order rules; move the line importing
WatchExperienceBlock from "@forge/graphql" to the top of the module before any
export statements (ensure the import sits above the export of whatever is being
exported in this file) so the type import is hoisted and the file remains
lint-clean; update any references to WatchExperienceBlock accordingly if their
relative position changed.
In `@turbo.json`:
- Around line 29-30: The generate task currently only declares the output
"src/graphql-env.d.ts" causing stale cache when the CMS schema changes; update
the "generate" task in turbo.json (the "generate" entry that lists outputs) to
include the CMS schema file as an input/global dependency (e.g., add
"../../apps/cms/schema.graphql" to the task's inputs/globals) so Turbo will
invalidate and rerun codegen when the schema changes.
---
Duplicate comments:
In `@apps/web/src/app/`[slug]/[locale]/page.tsx:
- Around line 35-40: The emptiness check currently runs before filtering invalid
entries, so null/Error items can make the page skip ExperienceEmpty; change the
order: first create the filtered const blocks = experience.blocks.filter((b): b
is Section => b !== null && b.__typename !== "Error"), then check if
(!blocks.length) return <ExperienceEmpty />; update any references that used the
pre-filtered value to use this filtered blocks variable (symbols: experience,
blocks, Section, ExperienceEmpty).
In `@apps/web/src/app/`[slug]/page.tsx:
- Around line 38-44: The empty-state check uses experience?.blocks length before
filtering out null/Error items, so pages with only invalid blocks render blank;
update the guard to compute the filtered array (const blocks =
experience.blocks.filter((b): b is Section => b !== null && b.__typename !==
"Error") ) first and then return <ExperienceEmpty /> when blocks.length === 0,
and thereafter render using the filtered blocks variable (blocks) instead of
experience.blocks.
In `@apps/web/src/components/sections/index.tsx`:
- Around line 25-27: The default branch in the switch that handles sections
currently does "return null", which hides schema/query drift; update the default
case in apps/web/src/components/sections/index.tsx to surface unsupported blocks
by logging a clear warning (include the block identifier like block._type or
block._key / block.id) and rendering a visible, testable fallback (e.g., a
placeholder element with a data attribute indicating the unsupported block type)
so missing/unknown CMS blocks are detectable in QA/rollout; alternatively, make
the switch exhaustive by adding explicit handlers for the missing block types if
known and throw a dev-only error to fail fast during development.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1ff80d3-7cc3-4972-8289-157e866eadb1
📒 Files selected for processing (10)
apps/web/src/app/[slug]/[locale]/page.tsxapps/web/src/app/[slug]/page.tsxapps/web/src/app/page.tsxapps/web/src/components/sections/index.tsxapps/web/src/lib/content.tsmobile/expo/src/lib/experienceService.test.tsmobile/expo/src/lib/experienceService.tsmobile/expo/src/lib/graphql/queries.tspackage.jsonturbo.json
🚧 Files skipped from review as they are similar to previous changes (4)
- mobile/expo/src/lib/experienceService.test.ts
- mobile/expo/src/lib/experienceService.ts
- apps/web/src/app/page.tsx
- package.json
Member
Author
Review feedback (5f0225b)Code changes:
Replied & resolved:
All outstanding review threads have been replied to and marked resolved. |
Made-with: Cursor
22 tasks
Ur-imazing
added a commit
that referenced
this pull request
Mar 10, 2026
* feat(ios): add GraphQL fragments and expand query for 10 section types - 8 named fragments for leaf types (MediaCollection, CTA, VideoHero, Text, RelatedQuestions, BibleQuotesCarousel, Card, Video) reused across 3 nesting levels - Expand GetWatchExperience query with all 12 inline fragments including nested Container slots and Section wrapper content - PromoBanner and InfoBlocks get id-only fragments (graceful skip) - Broaden codegen operationSearchPaths to GraphQL/**/*.graphql Made-with: Cursor * chore(ios): regenerate Apollo codegen for expanded query Generated types now include all 12 Experience section types, Container slots, Section content zones, and 8 named fragments. Made-with: Cursor * feat(ios): add section model structs for leaf and structural types - SectionModels.swift: shared types (UploadFileModel, VideoModel), enums, two-enum model (SectionContent + ExperienceSection) - SectionLeafModels.swift: 8 leaf model structs with Codable + Sendable - SectionStructuralModels.swift: ContainerSlot, ContainerSection, SectionWrapperSection for 3-level nesting - ExperienceContent gains Codable for debug JSON encoding - PromoBanner/InfoBlocks models removed (not applicable for mobile) Made-with: Cursor * feat(ios): expand GraphQLContentClient mappers for all section types - SectionMappers.swift: fragment-based leaf mappers (8 types) as extension on GraphQLContentClient with shared helpers - GraphQLContentClient rewritten: mapSection dispatches to fragment mappers + structural mappers for Container and Section wrapper - Recursive nesting: Experience → Section → Container → leaf (3 levels) - firstSectionTitle checks VideoHero, Text, Card, MediaCollection, CTA - PromoBanner/InfoBlocks return nil (graceful skip) Made-with: Cursor * feat(ios): add debug view fetching sample Experience as JSON - WatchHomeViewModel: add loadExperience(slug:locale:) exposing ExperienceContent with parsed sections - ForgeRootView: ExperienceDebugView fetches experience and displays scrollable JSON dump of all parsed section types - SwiftLint config: allow h1-h6 and id identifier names Made-with: Cursor * fix(ios): use correct slug 'easter' in debug view experience fetch The debug view was fetching slug "experience" which doesn't exist; the sample Experience has slug "easter". Made-with: Cursor * fix(ios): complete firstSectionTitle scan for all 10 section types - card.cardTitle was returned unconditionally, stopping the scan even for empty titles; now guarded with !isEmpty - Add missing checks for RelatedQuestions, BibleQuotesCarousel, Video - Move firstSectionTitle to SectionMappers.swift to stay under class body length limit Addresses CodeRabbit review on PR #298. Made-with: Cursor * fix(ios): add @mainactor to WatchHomeViewModel for UI thread safety @observable state mutations (isLoading, homeError, experienceContent) around awaits must happen on the main actor. SwiftUI expects UI-bound state to be synchronized on @mainactor. Addresses CodeRabbit review on PR #298. Made-with: Cursor * fix(ios): align data layer with CMS blocks rename and streamingUrl Update iOS data layer for CMS schema changes from #301 and #224: - sections → blocks field rename (ExperienceBlocksDynamicZone) - Add streamingUrl to VideoHero fragment, model, and mapper - Regenerate Apollo codegen against updated schema Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ios): revert CMS schema change, make VideoHero streamingUrl nullable Revert accidental schema.graphql modification (bounded context violation). Update VideoHero model and generated code to treat streamingUrl as optional (String?) matching the current CMS schema. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ios): remove streamingUrl from VideoHero fragment until CMS data populated The live CMS declares streamingUrl as non-null on VideoHero but existing content has no value set, causing a GraphQL error at runtime. Remove the field from the query for now; the model retains the optional property so it can be re-added once CMS content is seeded. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ios): use partial GraphQL data when sections have null required fields Strapi returns errors alongside partial data when non-null fields are null for individual sections (e.g. VideoHero without video or streamingUrl). Instead of throwing on any GraphQL error, use partial data when available — null sections are already skipped by compactMap in the mapper. Only throw if no data is returned at all. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ios): propagate task cancellation into Apollo fetch Replace withCheckedContinuation with withTaskCancellationHandler + withCheckedThrowingContinuation so that Swift Task cancellation cancels the in-flight Apollo network request via a thread-safe ApolloRequestBox wrapper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.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.
Summary
Adds Video Hero block to Strapi and web, renames Experience
sectionstoblocks, and adds a runnable seed script for the Easter watch experience. The /watch/easter page is driven by CMS content (single block renderer). Web usesSTRAPI_API_TOKENfrom.env.local; 401 handling and error copy clarified.Resolves #152
Contracts Changed
Experience.blocks,ComponentSectionsVideoHerowithstreamingUrl)Regeneration Required
Validation
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Refactor