refactor(web): colocate section fragments with components#66
Merged
refactor(web): colocate section fragments with components#66
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…cale] Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
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>
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. WalkthroughRefactors GraphQL fragment handling by collocating fragment definitions with their consuming section components instead of keeping them inline in the main query. Updates section components to accept fragment-typed props, consolidates null/error filtering in page components, makes certain environment variables required, and adds RxJS dependency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/preview/route.ts (1)
8-19:⚠️ Potential issue | 🟠 MajorOpen redirect:
new URL(redirect, url.origin)does not prevent absolute URL redirects
new URL("https://attacker.com/phish", url.origin)resolves tohttps://attacker.com/phish—the base is silently ignored when the first argument is an absolute URL. The same applies to protocol-relative URLs (//attacker.com). Any holder of a valid preview token can craft a phishing URL using this endpoint.🔒 Proposed fix — restrict redirect to relative paths
- const redirect = url.searchParams.get("redirect") ?? "/" + const rawRedirect = url.searchParams.get("redirect") ?? "/" + // Disallow absolute URLs and protocol-relative URLs to prevent open redirect + const redirect = rawRedirect.startsWith("/") && !rawRedirect.startsWith("//") + ? rawRedirect + : "/"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/preview/route.ts` around lines 8 - 19, The redirect parameter is vulnerable to open-redirects because new URL(redirect, url.origin) allows absolute and protocol-relative URLs; validate and restrict redirects to relative paths before calling NextResponse.redirect. In the route handler, replace the raw use of redirect/new URL with a whitelist check: ensure redirect is a non-empty string that starts with a single slash (redirect.startsWith('/') && !redirect.startsWith('//')) and does not contain '://' (or otherwise parse with new URL and verify its origin equals url.origin); if the check fails, set redirect = '/'. Then call draftMode(), draft.enable(), and NextResponse.redirect(new URL(redirect, url.origin)) using this sanitized redirect.
🧹 Nitpick comments (8)
apps/web/src/lib/client.ts (1)
9-12: Apollo Client singleton withInMemoryCacheshared across all server requestsThe module-level singleton means every concurrent and sequential Next.js server request shares one
InMemoryCache. With Apollo's defaultcache-firstpolicy, once a query is cached server-side it will never re-fetch from Strapi—meaningrevalidatePathpurges Next.js's page cache, but the in-flight Apollo cache continues to serve stale content. Additionally, the cache grows without bound on a long-running server.The standard approach for Next.js App Router is to create a new client per request or use
fetchPolicy: 'no-cache'for all server-side queries:♻️ Option A — per-request client factory (recommended)
-const uri = env.NEXT_PUBLIC_GRAPHQL_URL ?? "http://localhost:1337/graphql" -const headers: Record<string, string> = {} -if (env.STRAPI_API_TOKEN) { - headers.Authorization = `Bearer ${env.STRAPI_API_TOKEN}` -} -const client = new ApolloClient({ - link: new HttpLink({ uri, headers }), - cache: new InMemoryCache(), -}) - -export default client +import { cache } from "react" + +export const getClient = cache(() => { + const uri = env.NEXT_PUBLIC_GRAPHQL_URL ?? "http://localhost:1337/graphql" + const headers: Record<string, string> = {} + if (env.STRAPI_API_TOKEN) { + headers.Authorization = `Bearer ${env.STRAPI_API_TOKEN}` + } + return new ApolloClient({ + link: new HttpLink({ uri, headers }), + cache: new InMemoryCache(), + }) +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/client.ts` around lines 9 - 12, The module currently creates a module-level ApolloClient singleton (const client using new ApolloClient with InMemoryCache and HttpLink), causing a shared InMemoryCache across Next.js server requests and stale/unbounded caching; replace this pattern by turning that singleton into a factory that returns a fresh ApolloClient per request (e.g., createApolloClient(...) that constructs new ApolloClient({ link: new HttpLink({ uri, headers }), cache: new InMemoryCache(), defaultOptions: {...} })) or, if you must keep a shared instance, set server-side defaultOptions for queries/mutations to use fetchPolicy: 'no-cache' to avoid serving stale data—update all usages to call the factory (or rely on the no-cache default) instead of importing the module-level client.apps/web/src/components/sections/index.tsx (1)
9-22: Optional: add exhaustiveness check so new section types are caught at compile time
default: return nullsilently swallows unknown__typenamevalues. When a new section type is added to the GraphQL schema and theSectionunion is updated, the switch will silently no-op instead of producing a build error.♻️ Suggested exhaustiveness guard
default: - return null + // If TypeScript complains here, a new Section variant needs a case above. + // eslint-disable-next-line `@typescript-eslint/no-unused-vars` + const _exhaustive: never = section + return 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 9 - 22, The switch in SectionRenderer currently falls through to default returning null, which hides new Section.__typename variants; replace the default return with an exhaustive-check that causes a compile-time/type error for unknown types (e.g., add a helper like assertUnreachable(value: never) that throws and call it with the unmatched branch), or assign the unmatched value to a never-typed variable to force the compiler to complain; update SectionRenderer so the default branch calls that assert (include section.__typename in the thrown error message) to ensure new Section union members surface as build errors.apps/web/src/lib/locale.ts (1)
3-3:DEFAULT_LOCALEshould be exported so consuming files (like[slug]/[locale]/page.tsx) can reference it rather than re-hardcoding"en".♻️ Proposed fix
-const DEFAULT_LOCALE = "en" +export const DEFAULT_LOCALE = "en"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/locale.ts` at line 3, The constant DEFAULT_LOCALE is currently not exported; change DEFAULT_LOCALE to be exported so other modules can import it instead of hardcoding "en". Update the declaration of DEFAULT_LOCALE (the symbol DEFAULT_LOCALE in this module) to export it (e.g., export const DEFAULT_LOCALE = "en") and then replace any hardcoded "en" usages in consumers like [slug]/[locale]/page.tsx to import and reference DEFAULT_LOCALE.apps/web/src/env.ts (1)
11-11:NEXT_PUBLIC_GRAPHQL_URLbeing optional silently degrades all pages to error state at runtime.With
.optional(), a missing or misconfigured URL passes validation at startup. Every page call togetWatchExperiencethen returns{ error: "GraphQL URL not configured" }, renderingExperienceErrorfor every visitor. Failing loudly at startup (build or server init) is preferable.♻️ Proposed fix (if startup validation is acceptable)
- NEXT_PUBLIC_GRAPHQL_URL: z.string().url().optional(), + NEXT_PUBLIC_GRAPHQL_URL: z.string().url(),If the URL genuinely needs to be optional in some deployment contexts (e.g., staging), the current design is intentional — but add a comment to document that choice.
🤖 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 11, NEXT_PUBLIC_GRAPHQL_URL is marked optional causing silent startup validation passing and runtime failures; make it required by removing .optional() on the NEXT_PUBLIC_GRAPHQL_URL zod entry in apps/web/src/env.ts so the app fails loudly if the URL is missing, or if making it optional is deliberate, add a clear comment next to the NEXT_PUBLIC_GRAPHQL_URL symbol explaining why it may be omitted and how codepath getWatchExperience handles that deployment case.apps/web/src/app/[slug]/page.tsx (1)
34-35:"id" in sectionguard is always true after the type-predicate filter — use nullish coalescing.After the typed
filter(),sectionisSectionand allSectionvariants includeid. The structural check is redundant; the real concern is whetheridcan benull.♻️ Proposed fix
- const key = "id" in section ? section.id : `section-${i}` + const key = section.id ?? `section-${i}`🤖 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 34 - 35, The type-guard `"id" in section` is redundant after the typed filter; change the key generation in the sections.map callback (where key is assigned) to use nullish coalescing so it uses section.id when present and falls back to an index-based key otherwise — i.e., replace the `"id" in section` check with `section.id ??` fallback for `section-${i}` in the callback used to render sections.apps/web/src/app/[slug]/[locale]/page.tsx (1)
13-13: Hardcoded"en"should referenceDEFAULT_LOCALEfromlocale.ts.If
DEFAULT_LOCALEinlocale.tsever changes, this fallback silently diverges.♻️ Proposed fix
-import { isLocale } from "@/lib/locale" +import { isLocale, DEFAULT_LOCALE } from "@/lib/locale" ... - const locale = isLocale(rawLocale) ? rawLocale : "en" + const locale = isLocale(rawLocale) ? rawLocale : DEFAULT_LOCALEAlso export
DEFAULT_LOCALEfromapps/web/src/lib/locale.ts:-const DEFAULT_LOCALE = "en" +export const DEFAULT_LOCALE = "en"🤖 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 at line 13, Replace the hardcoded fallback "en" in page.tsx with the canonical DEFAULT_LOCALE from your locale module: import DEFAULT_LOCALE from the locale file and change the line using isLocale(rawLocale) ? rawLocale : DEFAULT_LOCALE; ensure the locale export exists by exporting DEFAULT_LOCALE from apps/web/src/lib/locale.ts (add an export if missing) so the fallback always follows the single source of truth; reference symbols: isLocale, rawLocale, DEFAULT_LOCALE, page.tsx, and apps/web/src/lib/locale.ts.apps/web/src/app/page.tsx (1)
27-27: Use nullish coalescing instead of the structural"id" in sectionguard.After the type-predicate filter on line 20-22, every element is a
Section, and allSectionvariants include anidfield, so"id" in sectionis alwaystrue. The guard effectively becomessection.id ??section-${i}``. Additionally, ifidis nullable in the schema, `key={null}` will be silently dropped by React, making the index-fallback unreachable and causing missing-key warnings.♻️ Proposed fix
- const key = "id" in section ? section.id : `section-${i}` + const key = section.id ?? `section-${i}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/page.tsx` at line 27, Replace the redundant type-guard expression that computes the React key; instead of `"id" in section ? section.id : \`section-${i}\``, use nullish coalescing to ensure a non-null key: compute the key as `section.id ?? \`section-${i}\`` so nullable ids fall back to the index-based string; update the code where `const key = ...` inside the map over `section` elements (page.tsx) to use this nullish coalescing pattern so React never receives a null/undefined key.apps/web/src/components/sections/MediaCollection.tsx (1)
120-127: Usenext/link<Link>instead of a plain<a>for the CTA.A bare
<a href={ctaLink}>bypasses Next.js client-side navigation, causing a full page reload for internal URLs and skipping prefetching.next/link's<Link>safely handles both relative (internal) and absolute (external) URLs.♻️ Proposed fix
At the top of the file:
import Image from "next/image" +import Link from "next/link"For the CTA anchor:
- <a - href={ctaLink} - className="mt-4 inline-block font-medium text-blue-600 hover:underline" - > - View all - </a> + <Link + href={ctaLink} + className="mt-4 inline-block font-medium text-blue-600 hover:underline" + > + View all + </Link>🤖 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 120 - 127, Replace the bare anchor used for the CTA in the MediaCollection component with Next.js client-side navigation: import Link from 'next/link' at the top of the file and render the CTA using Link around the CTA content (using the existing ctaLink prop) instead of <a href={ctaLink}> so internal routes use client-side routing and prefetching; if ctaLink may point to external hosts, ensure Link handles absolute URLs correctly or fall back to a normal anchor with target="_blank" and rel="noopener noreferrer" for external links.
🤖 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: Update the images.remotePatterns configuration in
next.config.mjs to allow a configurable production CMS hostname by reading
NEXT_PUBLIC_CMS_HOSTNAME and NEXT_PUBLIC_CMS_PROTOCOL (default to "https" if
unset) and push a third pattern using those env vars so next/image accepts
images from the deployed Strapi domain; also add NEXT_PUBLIC_CMS_HOSTNAME and
NEXT_PUBLIC_CMS_PROTOCOL entries to apps/web/.env.example to document the new
vars. Ensure you reference the images.remotePatterns array in next.config.mjs
and the environment variable names NEXT_PUBLIC_CMS_HOSTNAME and
NEXT_PUBLIC_CMS_PROTOCOL when implementing the change.
In `@apps/web/package.json`:
- Around line 13-20: The package.json dependencies are missing the rxjs peer
dependency required by `@apollo/client` v4; update the apps/web package.json
dependencies section to add "rxjs" with a compatible version (e.g., "^7.3.0") so
`@apollo/client` can resolve its Observable implementation, ensuring you add it
under dependencies (not devDependencies) alongside "@apollo/client".
In `@apps/web/src/app/`[slug]/[locale]/page.tsx:
- Around line 27-37: Replace the current inline null/"Error" checks and ad-hoc
cast on experience.sections with the same typed filter-then-map pattern used
elsewhere: create a type-predicate filter that narrows Array<Section | {
__typename: "Error" } | null> to Section[] (e.g., a function that checks section
!= null && "__typename" not "Error" && "id" in section), call that filter on
experience.sections to get a fully-typed Section[], then map that array to
render <SectionRenderer section={section} key={...}/> without using "as Section"
or inline null checks; update the key logic to use section.id when present
otherwise fallback to a stable index-based key.
In `@apps/web/src/app/`[slug]/page.tsx:
- Around line 15-17: The current dual-use of the path segment (slug) for locale
detection (isLocale(slug)) causes collisions with CMS slugs; update routing so
locale detection is not conflated with content slugs: stop treating a plain slug
equal to a locale code as the homepage—either (A) move locale handling into a
dedicated param/segment (e.g., a separate [locale] route or nested
[slug]/[locale] route) or (B) require a clear prefix (e.g., /locale/:locale)
before calling isLocale, and then call getWatchExperience(locale) only when a
true locale param is present; update the code paths that call isLocale and
getWatchExperience accordingly (refer to isLocale, slug, and getWatchExperience)
and document reserved locale codes for CMS authors if you keep any legacy
shortcut.
In `@apps/web/src/components/ExperienceEmpty.tsx`:
- Around line 1-7: Change ExperienceEmpty to avoid nesting by replacing the
outer <main> with a neutral container element like <section> (or <div>) and make
the displayed text localizable instead of hard-coded: update the ExperienceEmpty
component signature to accept an optional message prop or call the app’s
i18n/translation hook (e.g., useRouter.locale or the project’s translation
function) to render a locale-specific string with a sensible English fallback;
keep the existing className and layout but reference ExperienceEmpty and the
message prop or translation key so consumers can pass or derive translated text.
In `@apps/web/src/components/sections/CTASection.tsx`:
- Around line 24-29: CTASection currently renders an anchor even when buttonLink
may be null and lacks safe navigation attributes; update the CTASection
component to guard against falsy/empty buttonLink (e.g., don't render the <a> if
buttonLink is null/empty or render a disabled fallback) and add rel="noopener
noreferrer" to the anchor; if you decide to open external links in a new tab
also add target="_blank" (use buttonLink and buttonLabel to locate the element
and adjust rendering logic accordingly).
In `@apps/web/src/components/sections/InfoBlocks.tsx`:
- Around line 51-52: The InfoBlocks component currently renders block.title and
block.description unconditionally, which can produce empty <h3> or <p> tags when
those fields are nullable; update the JSX in InfoBlocks.tsx to null-guard both
block.title and block.description (like the existing guards for
heading/intro/description) so each element is only rendered when the
corresponding value is truthy (e.g., wrap the <h3> around block.title with a
conditional and do the same for the <p> showing block.description), referencing
the block object used in the map/render logic.
In `@apps/web/src/components/sections/MediaCollection.tsx`:
- Line 125: The "View all" string in the MediaCollection component is hardcoded
and must be localized; update MediaCollection.tsx to source the label from a
locale-aware source instead of literal text — either add a ctaLabel (or similar)
field to the CMS fragment used by this component and render that (e.g.,
reference the fragment/prop that supplies the collection CTA), or wire it to
your translation layer (use the i18n helper your app provides) so the label
respects getWatchExperience/I18NLocaleCode. Ensure you update the fragment or
props that feed MediaCollection to include the new ctaLabel and replace the
literal "View all" with that prop or a translation lookup.
- Around line 58-91: ItemCard is currently declared inside MediaCollection which
recreates its function on every render causing React to unmount/remount each
card; move the ItemCard function to module scope (outside MediaCollection) and
change its signature to accept props for item: EnrichedMediaItem, index: number,
showItemNumbers: boolean, and variant: MediaCollectionVariant (or the same type
used in MediaCollection), then update the JSX calls (<ItemCard ... />) inside
MediaCollection to pass those props so the component reference is stable across
renders.
In `@apps/web/src/components/sections/PromoBanner.tsx`:
- Around line 35-40: PromoBanner's CTA anchor uses ctaLink without guarding for
null/empty and lacks rel attributes; update the render in the PromoBanner
component to check ctaLink (the prop/variable) and only render a clickable
external anchor when ctaLink is a non-empty string, otherwise render a
non-navigating fallback (e.g., a disabled button or span) to avoid <a
href={null}> or empty-href behavior, and when you do render an external anchor
that uses target="_blank" add rel="noopener noreferrer" to the <a> element to
prevent window.opener access; ensure you reference the same ctaLink variable and
the anchor in the PromoBanner JSX when making these changes.
In `@apps/web/src/lib/client.ts`:
- Around line 5-8: Add the server-only sentinel import to the top of the module
to prevent accidental client-side imports: add import "server-only" as the first
statement in apps/web/src/lib/client.ts so that any use of env.STRAPI_API_TOKEN
(referenced when building the headers object) will throw a clear error if this
file is imported into a client component; keep the existing Apollo-related
imports (ApolloClient, HttpLink, InMemoryCache) and the headers const as-is,
just prepend the import "server-only".
In `@apps/web/src/lib/content.ts`:
- Line 90: The returned error is being downcast to Error which loses
ApolloError-specific fields; update the WatchExperienceResult type to allow
ApolloError (e.g., error: ApolloError | Error) and return result.error without
narrowing, or explicitly cast to ApolloError | Error so callers can access
.graphQLErrors/.networkError; locate the return site where result.error is used
(the result from client.query()) and adjust the union/type and the returned
error shape accordingly.
- Around line 71-73: The discriminated union WatchExperienceResult should use a
non-nullable success type so callers don't get {data: null, error: null}; change
the success branch to { data: NonNullable<WatchExperience>; error: null } (where
WatchExperience = WatchData["experiences"][number]) and update the place that
returns the success value (the code that checks `if (!exp)` around the `exp`
variable) to assert/cast the value to NonNullable<WatchExperience> when
returning so TypeScript accepts the narrowed type.
---
Outside diff comments:
In `@apps/web/src/app/api/preview/route.ts`:
- Around line 8-19: The redirect parameter is vulnerable to open-redirects
because new URL(redirect, url.origin) allows absolute and protocol-relative
URLs; validate and restrict redirects to relative paths before calling
NextResponse.redirect. In the route handler, replace the raw use of redirect/new
URL with a whitelist check: ensure redirect is a non-empty string that starts
with a single slash (redirect.startsWith('/') && !redirect.startsWith('//')) and
does not contain '://' (or otherwise parse with new URL and verify its origin
equals url.origin); if the check fails, set redirect = '/'. Then call
draftMode(), draft.enable(), and NextResponse.redirect(new URL(redirect,
url.origin)) using this sanitized redirect.
---
Nitpick comments:
In `@apps/web/src/app/`[slug]/[locale]/page.tsx:
- Line 13: Replace the hardcoded fallback "en" in page.tsx with the canonical
DEFAULT_LOCALE from your locale module: import DEFAULT_LOCALE from the locale
file and change the line using isLocale(rawLocale) ? rawLocale : DEFAULT_LOCALE;
ensure the locale export exists by exporting DEFAULT_LOCALE from
apps/web/src/lib/locale.ts (add an export if missing) so the fallback always
follows the single source of truth; reference symbols: isLocale, rawLocale,
DEFAULT_LOCALE, page.tsx, and apps/web/src/lib/locale.ts.
In `@apps/web/src/app/`[slug]/page.tsx:
- Around line 34-35: The type-guard `"id" in section` is redundant after the
typed filter; change the key generation in the sections.map callback (where key
is assigned) to use nullish coalescing so it uses section.id when present and
falls back to an index-based key otherwise — i.e., replace the `"id" in section`
check with `section.id ??` fallback for `section-${i}` in the callback used to
render sections.
In `@apps/web/src/app/page.tsx`:
- Line 27: Replace the redundant type-guard expression that computes the React
key; instead of `"id" in section ? section.id : \`section-${i}\``, use nullish
coalescing to ensure a non-null key: compute the key as `section.id ??
\`section-${i}\`` so nullable ids fall back to the index-based string; update
the code where `const key = ...` inside the map over `section` elements
(page.tsx) to use this nullish coalescing pattern so React never receives a
null/undefined key.
In `@apps/web/src/components/sections/index.tsx`:
- Around line 9-22: The switch in SectionRenderer currently falls through to
default returning null, which hides new Section.__typename variants; replace the
default return with an exhaustive-check that causes a compile-time/type error
for unknown types (e.g., add a helper like assertUnreachable(value: never) that
throws and call it with the unmatched branch), or assign the unmatched value to
a never-typed variable to force the compiler to complain; update SectionRenderer
so the default branch calls that assert (include section.__typename in the
thrown error message) to ensure new Section union members surface as build
errors.
In `@apps/web/src/components/sections/MediaCollection.tsx`:
- Around line 120-127: Replace the bare anchor used for the CTA in the
MediaCollection component with Next.js client-side navigation: import Link from
'next/link' at the top of the file and render the CTA using Link around the CTA
content (using the existing ctaLink prop) instead of <a href={ctaLink}> so
internal routes use client-side routing and prefetching; if ctaLink may point to
external hosts, ensure Link handles absolute URLs correctly or fall back to a
normal anchor with target="_blank" and rel="noopener noreferrer" for external
links.
In `@apps/web/src/env.ts`:
- Line 11: NEXT_PUBLIC_GRAPHQL_URL is marked optional causing silent startup
validation passing and runtime failures; make it required by removing
.optional() on the NEXT_PUBLIC_GRAPHQL_URL zod entry in apps/web/src/env.ts so
the app fails loudly if the URL is missing, or if making it optional is
deliberate, add a clear comment next to the NEXT_PUBLIC_GRAPHQL_URL symbol
explaining why it may be omitted and how codepath getWatchExperience handles
that deployment case.
In `@apps/web/src/lib/client.ts`:
- Around line 9-12: The module currently creates a module-level ApolloClient
singleton (const client using new ApolloClient with InMemoryCache and HttpLink),
causing a shared InMemoryCache across Next.js server requests and
stale/unbounded caching; replace this pattern by turning that singleton into a
factory that returns a fresh ApolloClient per request (e.g.,
createApolloClient(...) that constructs new ApolloClient({ link: new HttpLink({
uri, headers }), cache: new InMemoryCache(), defaultOptions: {...} })) or, if
you must keep a shared instance, set server-side defaultOptions for
queries/mutations to use fetchPolicy: 'no-cache' to avoid serving stale
data—update all usages to call the factory (or rely on the no-cache default)
instead of importing the module-level client.
In `@apps/web/src/lib/locale.ts`:
- Line 3: The constant DEFAULT_LOCALE is currently not exported; change
DEFAULT_LOCALE to be exported so other modules can import it instead of
hardcoding "en". Update the declaration of DEFAULT_LOCALE (the symbol
DEFAULT_LOCALE in this module) to export it (e.g., export const DEFAULT_LOCALE =
"en") and then replace any hardcoded "en" usages in consumers like
[slug]/[locale]/page.tsx to import and reference DEFAULT_LOCALE.
- Merge origin/main into feat/65 - Keep fragment colocation, adopt main env/client - section.id ?? fallback, filter-then-map in [locale] page - DEFAULT_LOCALE export, use in [locale] page - server-only in client.ts - CTASection/PromoBanner ctaLink/buttonLink null guard + rel - InfoBlocks block.title/description null guards - SectionRenderer exhaustiveness check Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
Review feedback addressed (56d8c14)Fixed:
Not changed:
|
- next.config: NEXT_PUBLIC_CMS_HOSTNAME/PROTOCOL for production images - package.json: add rxjs ^7.3.0 peer for @apollo/client - content.ts: NonNullable success type, ErrorLike for Apollo errors Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/lib/client.ts (1)
6-9:⚠️ Potential issue | 🟡 MinorInconsistency between
z.string()(required) schema and theifguard — could silently produce unauthenticated requests.
STRAPI_API_TOKENis declared asz.string()(non-optional) inenv.ts, which passes Zod validation even for an empty string"". If someone setsSTRAPI_API_TOKEN=in their environment,createEnvaccepts it (empty string is a validz.string()), but line 7's truthiness check evaluates tofalseand theAuthorizationheader is silently omitted — resulting in unauthenticated API calls at runtime instead of a clear startup-time failure.The fix should be in
apps/web/src/env.ts— tighten the schema to reject empty strings:🛡️ Proposed fix in
apps/web/src/env.tsserver: { - STRAPI_API_TOKEN: z.string(), + STRAPI_API_TOKEN: z.string().min(1), STRAPI_REVALIDATE_TOKEN: z.string().optional(), STRAPI_PREVIEW_TOKEN: z.string().optional(), },With
.min(1), an empty token fails at startup and theifguard on line 7 becomes reliably dead code (a token is always present and non-empty), making the intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/client.ts` around lines 6 - 9, The STRAPI_API_TOKEN env schema is currently z.string() which allows empty strings and causes the runtime Authorization guard in client.ts to be silently bypassed; open apps/web/src/env.ts and change the createEnv/schema entry for STRAPI_API_TOKEN from z.string() to z.string().min(1) (ensuring non-empty) so the app fails at startup on an empty token and the Authorization header guard in client.ts becomes reliably dead code.apps/web/src/app/[slug]/page.tsx (1)
24-36:⚠️ Potential issue | 🟡 MinorSame post-filter empty case: blank
<main>instead of<ExperienceEmpty />.Identical to the issue in
apps/web/src/app/page.tsx: the pre-filter guard on line 24 passes as long as the raw array is non-empty, but if all items arenullor{ __typename: "Error" }the filteredsectionsarray is empty and the component renders an empty<main>.🛡️ Proposed fix
const sections = experience.sections.filter( (s): s is Section => s !== null && s.__typename !== "Error", ) + + if (!sections.length) { + return <ExperienceEmpty /> + } return (🤖 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 24 - 36, The pre-filter guard checks only the raw experience.sections but after filtering null/Error items the derived sections array can be empty, causing an empty <main>; update the rendering logic in the component that defines sections (the const sections = experience.sections.filter(...) block and subsequent return) to check the filtered array and return <ExperienceEmpty /> when sections.length === 0 before mapping to <SectionRenderer /> (i.e., after computing sections, if (!sections.length) return <ExperienceEmpty />).
🧹 Nitpick comments (1)
apps/web/src/components/sections/index.tsx (1)
19-22: Exhaustiveness check is correct — ESLint warning is a false positive.The
_exhaustive: neverassignment is the standard TypeScript pattern to get a compile-time error when a new union member is unhandled. The underscore prefix signals intentional non-use. Consider suppressing the lint rule on this line if the CI enforces--max-warnings=0.Proposed suppression
default: { + // eslint-disable-next-line `@typescript-eslint/no-unused-vars` const _exhaustive: never = section return 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 19 - 22, The default case uses the TypeScript exhaustiveness pattern via const _exhaustive: never = section but ESLint flags it as an unused variable; keep the existing assignment (in the default branch) and suppress the linter for that line by adding an inline ESLint disable for the unused-var rule (e.g. eslint-disable-next-line `@typescript-eslint/no-unused-vars` or eslint-disable-next-line no-unused-vars) immediately above the const _exhaustive declaration so the compile-time check remains while CI stops treating it as a warning.
🤖 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/page.tsx`:
- Around line 16-27: The current check uses the raw experience.sections length
but doesn't account for filtering out null/Error items, so when the filtered
sections array is empty the component renders an empty <main>; update the logic
to compute the filtered sections first (the variable sections created from
experience.sections.filter) and then if sections.length === 0 return
<ExperienceEmpty />; ensure you reference the existing symbols:
experience.sections, sections (the filtered array), SectionRenderer, and
ExperienceEmpty so the guard runs after filtering and the map only runs when
sections has items.
In `@apps/web/src/lib/content.ts`:
- Line 3: Remove the unused import of env from the top-level import list (the
import statement that reads "import { env } from ...") in the content.ts module;
delete the env import so only used symbols remain imported (or consolidate the
import if other symbols are needed), and run lint/type checks to confirm no
remaining references to env exist.
---
Outside diff comments:
In `@apps/web/src/app/`[slug]/page.tsx:
- Around line 24-36: The pre-filter guard checks only the raw
experience.sections but after filtering null/Error items the derived sections
array can be empty, causing an empty <main>; update the rendering logic in the
component that defines sections (the const sections =
experience.sections.filter(...) block and subsequent return) to check the
filtered array and return <ExperienceEmpty /> when sections.length === 0 before
mapping to <SectionRenderer /> (i.e., after computing sections, if
(!sections.length) return <ExperienceEmpty />).
In `@apps/web/src/lib/client.ts`:
- Around line 6-9: The STRAPI_API_TOKEN env schema is currently z.string() which
allows empty strings and causes the runtime Authorization guard in client.ts to
be silently bypassed; open apps/web/src/env.ts and change the createEnv/schema
entry for STRAPI_API_TOKEN from z.string() to z.string().min(1) (ensuring
non-empty) so the app fails at startup on an empty token and the Authorization
header guard in client.ts becomes reliably dead code.
---
Duplicate comments:
In `@apps/web/package.json`:
- Line 20: No change required: the "rxjs" dependency declaration under the
dependencies key with version "^7.3.0" is correct and satisfies Apollo Client
4's peer requirement; leave the "rxjs" entry as-is in package.json (no code
changes needed).
In `@apps/web/src/app/`[slug]/page.tsx:
- Around line 15-17: The code is conflating the route param slug with a locale,
making CMS pages with names like "en" unreachable; stop using slug to detect
locale—derive the locale from a dedicated source (e.g., Next.js i18n or a
separate params.locale) and keep slug strictly as the CMS identifier. Replace
the isLocale(slug) check with isLocale(routeLocale) where routeLocale is pulled
from the proper locale source (or defaultLocale), and call
getWatchExperience(locale) vs getWatchExperience(locale, { slug }) based on that
check; update variable names (e.g., routeLocale, slug) so getWatchExperience and
isLocale references are unambiguous.
In `@apps/web/src/lib/client.ts`:
- Around line 1-3: The import "server-only" is correctly added to prevent
client-side imports of STRAPI_API_TOKEN and Apollo setup; no code changes
required—leave the top-level import statement intact in
apps/web/src/lib/client.ts and keep the ApolloClient, HttpLink, InMemoryCache
imports as-is to ensure server-only behavior for the Apollo setup.
---
Nitpick comments:
In `@apps/web/src/components/sections/index.tsx`:
- Around line 19-22: The default case uses the TypeScript exhaustiveness pattern
via const _exhaustive: never = section but ESLint flags it as an unused
variable; keep the existing assignment (in the default branch) and suppress the
linter for that line by adding an inline ESLint disable for the unused-var rule
(e.g. eslint-disable-next-line `@typescript-eslint/no-unused-vars` or
eslint-disable-next-line no-unused-vars) immediately above the const _exhaustive
declaration so the compile-time check remains while CI stops treating it as a
warning.
This was referenced Feb 19, 2026
Kneesal
pushed a commit
to Kneesal/forge
that referenced
this pull request
Mar 9, 2026
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.
Summary
Colocate GraphQL fragments with section components (MediaCollection, PromoBanner, InfoBlocks, CTASection). Each component defines its fragment and accepts
FragmentOf<typeof fragment>. GET_WATCH_EXPERIENCE composes fragments via spreads. Section type derived from query result.Resolves #65
Contracts Changed
Regeneration Required
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Refactor