feat(mobile-ios): GraphQL client and codegen#116
Merged
Ur-imazing merged 19 commits intomainfrom Feb 26, 2026
Merged
Conversation
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…LI symlink Made-with: Cursor
4 tasks
|
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:
WalkthroughThis PR integrates Apollo iOS as the GraphQL client for iOS, adding schema codegen configuration, a GraphQLContentClient to fetch Experience data from Strapi, Strapi token management via build phases, and updates the app structure to support configurable endpoints and authentication for dev/prod environments. Changes
Sequence DiagramsequenceDiagram
actor User
participant ForgeRootView
participant ContentRepository
participant GraphQLContentClient
participant Apollo
participant Strapi
User->>ForgeRootView: App loads
ForgeRootView->>ContentRepository: fetchHome(locale: "en")
ContentRepository->>GraphQLContentClient: getContent(locale: "en", slug: "experience")
GraphQLContentClient->>GraphQLContentClient: Build GetWatchExperience query
GraphQLContentClient->>Apollo: Execute GraphQL query
Apollo->>Strapi: POST /graphql (with bearer token)
Strapi-->>Apollo: Experience data + sections
Apollo-->>GraphQLContentClient: Query result
GraphQLContentClient->>GraphQLContentClient: Map experience to MobileContentItem
GraphQLContentClient-->>ContentRepository: MobileContentItem
ContentRepository-->>ForgeRootView: Content result
ForgeRootView->>ForgeRootView: Render GraphQLTestView with content
ForgeRootView-->>User: Display fetched content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Ur-imazing
changed the title
…ld via .env.local 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: 4
🧹 Nitpick comments (3)
mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift (2)
33-37: Consider simplifying thewithCheckedContinuationverbose type annotation.The explicit
(continuation: CheckedContinuation<Result<GraphQLResult<ForgeSchema.GetWatchExperienceQuery.Data>, Error>, Never>)annotation is very long. Type inference handles this automatically when you declare the result variable inline.♻️ Suggested simplification
- let result = await withCheckedContinuation { (continuation: CheckedContinuation<Result<GraphQLResult<ForgeSchema.GetWatchExperienceQuery.Data>, Error>, Never>) in + let result = await withCheckedContinuation { continuation in apollo.fetch(query: query, cachePolicy: .fetchIgnoringCacheData) { result in continuation.resume(returning: result) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift` around lines 33 - 37, The continuation closure currently includes a very long explicit type ((continuation: CheckedContinuation<Result<GraphQLResult<ForgeSchema.GetWatchExperienceQuery.Data>, Error>, Never>))—remove that annotation and let Swift infer the type by writing withCheckedContinuation { continuation in ... } (keep the apollo.fetch call and continuation.resume(returning: result) as-is) so the code uses type inference for withCheckedContinuation.
61-61: Remove redundantString()initializer ondocumentIdApollo iOS maps the GraphQL
IDscalar to SwiftStringvia typealias, sodocumentIdis already aString. TheString(...)call is unnecessary.♻️ Suggested cleanup
- id: String(experience.documentId), + id: experience.documentId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift` at line 61, In GraphQLContentClient.swift replace the redundant String(...) wrapper around the GraphQL ID by passing experience.documentId directly (remove String(experience.documentId) and use experience.documentId for the id field) since Apollo maps ID to Swift String; update any nearby code that constructs the same id field in the same initializer or function to follow the same pattern.mobile/ios/GraphQL/Operations/GetWatchExperience.graphql (1)
9-9:__typenameis redundant — Apollo iOS injects it automatically.Apollo iOS's codegen unconditionally emits
__typenameon every type it processes, so this explicit selection is a no-op. It's harmless but can be removed to reduce noise.♻️ Suggested cleanup
sections { - __typename ... on ComponentSectionsMediaCollection {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/GraphQL/Operations/GetWatchExperience.graphql` at line 9, Remove the explicit "__typename" field selection from the GetWatchExperience GraphQL operation (GetWatchExperience.graphql) because Apollo iOS codegen already injects __typename; update the operation definition to omit the __typename line so the generated code and query are cleaner and avoid redundant selections.
🤖 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/ios/apollo-codegen-configuration.json`:
- Line 4: The schemaSearchPaths entry in apollo-codegen-configuration.json
currently uses a relative path ("../../apps/cms/schema.graphql") that can break
when CI runs codegen from a different cwd; update CI to invoke apollo-ios-cli
generate from the mobile/ios directory or call the CLI with the
--project-directory option so the relative path is resolved correctly, or change
schemaSearchPaths to a path anchored to the repository root (or an environment
variable) so it does not depend on the working directory; locate and update the
"schemaSearchPaths" entry in apollo-codegen-configuration.json and adjust the CI
job that runs apollo-ios-cli accordingly.
In `@mobile/ios/Package.swift`:
- Line 6: Remove the macOS platform entry from the Package.swift platforms
declaration so the package is iOS-only; locate the platforms array (platforms:
[.iOS(.v17), .macOS(.v12)]) and change it to only include .iOS(.v17), removing
the .macOS(.v12) element and any trailing comma, and ensure no comments or other
references to macOS remain in the Package.swift or related package
configuration.
In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift`:
- Around line 65-66: The MobileContentItem creation in
GraphQLContentClient.swift currently hardcodes body: "" and state: "published";
update the code in the mapping/construction logic (where MobileContentItem is
assembled) to extract body and publication state from the GraphQL response
fields (use the response property names the client receives) instead of
hardcoding, or if those fields are intentionally absent, replace the literals
with clear TODO comments documenting why they are stubbed and the expected
schema field names (e.g., body, state) so future callers don’t assume published
state; ensure you adjust any uses of MobileContentItem.state accordingly to
avoid silently treating drafts as published.
- Around line 76-78: In firstSectionTitle, the PromoBanner branch returns
promo.promoBannerHeading unconditionally
(section.asComponentSectionsPromoBanner), which can prematurely exit the loop
when heading is nil/empty; change this branch to only return when
promo.promoBannerHeading is non-nil and not empty (mirror other branches'
checks) so the loop continues to examine later sections and falls back to
experience.slug only if no valid title is found.
---
Nitpick comments:
In `@mobile/ios/GraphQL/Operations/GetWatchExperience.graphql`:
- Line 9: Remove the explicit "__typename" field selection from the
GetWatchExperience GraphQL operation (GetWatchExperience.graphql) because Apollo
iOS codegen already injects __typename; update the operation definition to omit
the __typename line so the generated code and query are cleaner and avoid
redundant selections.
In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift`:
- Around line 33-37: The continuation closure currently includes a very long
explicit type ((continuation:
CheckedContinuation<Result<GraphQLResult<ForgeSchema.GetWatchExperienceQuery.Data>,
Error>, Never>))—remove that annotation and let Swift infer the type by writing
withCheckedContinuation { continuation in ... } (keep the apollo.fetch call and
continuation.resume(returning: result) as-is) so the code uses type inference
for withCheckedContinuation.
- Line 61: In GraphQLContentClient.swift replace the redundant String(...)
wrapper around the GraphQL ID by passing experience.documentId directly (remove
String(experience.documentId) and use experience.documentId for the id field)
since Apollo maps ID to Swift String; update any nearby code that constructs the
same id field in the same initializer or function to follow the same pattern.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (22)
mobile/ios/Sources/ForgeMobile/Generated/ForgeSchema.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Operations/Queries/GetWatchExperienceQuery.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/CustomScalars/DateTime.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/CustomScalars/I18NLocaleCode.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/CustomScalars/ID.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/Enums/ENUM_COMPONENTSECTIONSMEDIACOLLECTION_VARIANT.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/InputObjects/BooleanFilterInput.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/InputObjects/DateTimeFilterInput.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/InputObjects/ExperienceFiltersInput.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/InputObjects/IDFilterInput.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/InputObjects/StringFilterInput.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/Objects/ComponentSectionsCta.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/Objects/ComponentSectionsInfoBlock.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/Objects/ComponentSectionsInfoBlocks.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/Objects/ComponentSectionsMediaCollection.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/Objects/ComponentSectionsPromoBanner.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/Objects/Error.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/Objects/Experience.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/Objects/Query.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/SchemaConfiguration.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/SchemaMetadata.graphql.swiftis excluded by!**/generated/**mobile/ios/Sources/ForgeMobile/Generated/Schema/Unions/ExperienceSectionsDynamicZone.graphql.swiftis excluded by!**/generated/**
📒 Files selected for processing (7)
mobile/ios/.gitignoremobile/ios/GraphQL/Operations/GetWatchExperience.graphqlmobile/ios/Package.resolvedmobile/ios/Package.swiftmobile/ios/README.mdmobile/ios/Sources/ForgeMobile/GraphQLContentClient.swiftmobile/ios/apollo-codegen-configuration.json
…oBanner heading Made-with: Cursor
Contributor
Author
Review feedback addressed (2a3b753, e95cc9b)Fixed:
Not changed: n/a — all four CodeRabbit comments were addressed. |
Made-with: Cursor
Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/ci.yml (1)
107-110: Addenvconfiguration toturbo.jsonfor thebuildtask to pass secrets via process environment instead of.env.local.
STRAPI_API_TOKENis a backend secret that should not be written to disk in CI, as it risks exposure via Turbo's remote cache, CI caches, or build artifacts. Since the workflow already provides these as environment variables, declare them inturbo.json:"build": { "dependsOn": ["^build"], "outputs": ["dist/**", ".next/**", "build/**"], "env": ["STRAPI_API_TOKEN", "NEXT_PUBLIC_GRAPHQL_URL"] }This allows Turborepo (in Strict Mode) to pass them through to
next buildwithout requiring a.env.localfile. Next.js and Turborepo's framework inference will handle both backend and frontend variable usage correctly at build time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 107 - 110, Update the Turborepo build task so CI secrets are passed via process env instead of writing to disk: add the "env" array to the "build" task in turbo.json and include "STRAPI_API_TOKEN" and "NEXT_PUBLIC_GRAPHQL_URL". Remove or stop using the step that echoes these variables into apps/web/.env.local in the CI workflow (the echo lines in .github/workflows/ci.yml) so the secrets are not written to disk; Turborepo will forward the declared env vars to next build in strict mode.mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift (3)
33-37:withCheckedContinuationis correct for Apollo iOS 1.0, but upgrading to 2.x enables native async/await.Swift structured concurrency introduces an entirely new mental model for cancellation and isolation; Apollo iOS 2.0 leans into that world with a
Sendable-aware client surface so GraphQL workflows compose naturally with tasks and cancellation semantics. In contrast, in Apollo iOS 1.0,RequestChainwas a protocol with a provided implementationInterceptorRequestChainandfetchis strictly callback-based — meaning the currentwithCheckedContinuationbridging is the correct approach for the pinned 1.0.0 version. There is no native async overload to call.The trade-off to be aware of: if the calling
Taskis cancelled while awaiting, the underlying Apollo 1.x HTTP request is not cancelled — it completes in the background and the continuation simply resumes with the result. This is benign but wasteful.Consider tracking an upgrade to Apollo iOS 2.x (which exposes
try await apollo.fetch(query:)natively) so this bridging wrapper and its cancellation gap can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift` around lines 33 - 37, The current GraphQLContentClient.swift bridges the callback-based apollo.fetch using withCheckedContinuation (wrapping apollo.fetch) which works for Apollo iOS 1.x but leaves a cancellation gap where cancelling the calling Task does not cancel the underlying HTTP request; fix by upgrading to Apollo iOS 2.x and replacing the continuation wrapper with the native async API (call try await apollo.fetch(query:) directly in the GraphQLContentClient method, convert the method signature to async/throws as needed, remove the withCheckedContinuation block and associated callback handling, and ensure the client code uses the Sendable-aware Apollo client surface so Task cancellation propagates).
63-63: Remove the redundantString()cast onexperience.documentId.
ForgeSchema.IDis a typealias forString, makingString(experience.documentId)a no-op conversion. Change to:Diff
id: String(experience.documentId),to
id: experience.documentId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift` at line 63, The id assignment using String(experience.documentId) is a redundant cast because ForgeSchema.ID is a String typealias; update the id initializer in GraphQLContentClient (the assignment that uses experience.documentId) to pass experience.documentId directly (remove the String(...) wrapper) so the field uses the existing String value without re-casting.
63-63: Remove redundantString()conversion ondocumentId.
ForgeSchema.IDis a typealias forString, soexperience.documentIdis alreadyString. TheString(…)wrapper is a no-op cast—remove it to simplify the code:id: experience.documentId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift` at line 63, The id field is being wrapped in an unnecessary String(...) when ForgeSchema.ID is already a typealias for String; locate the initializer/assignment where you set id: String(experience.documentId) in GraphQLContentClient.swift and replace it with the direct value id: experience.documentId (remove the String(...) wrapper) so the code uses the existing String-typed ForgeSchema.ID without a redundant conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 108-110: The CI step currently appends environment variables to
apps/web/.env.local using >> which can create duplicate keys; change the two
echo commands that write STRAPI_API_TOKEN and NEXT_PUBLIC_GRAPHQL_URL to use >
(overwrite) for the first write and >> (append) for the second, or simply write
both into a fresh file in one overwrite command so apps/web/.env.local is
recreated cleanly before pnpm build; update the echo invocations in the workflow
block that currently read echo "STRAPI_API_TOKEN=$STRAPI_API_TOKEN" and echo
"NEXT_PUBLIC_GRAPHQL_URL=$NEXT_PUBLIC_GRAPHQL_URL" accordingly.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 107-110: Update the Turborepo build task so CI secrets are passed
via process env instead of writing to disk: add the "env" array to the "build"
task in turbo.json and include "STRAPI_API_TOKEN" and "NEXT_PUBLIC_GRAPHQL_URL".
Remove or stop using the step that echoes these variables into
apps/web/.env.local in the CI workflow (the echo lines in
.github/workflows/ci.yml) so the secrets are not written to disk; Turborepo will
forward the declared env vars to next build in strict mode.
In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift`:
- Around line 33-37: The current GraphQLContentClient.swift bridges the
callback-based apollo.fetch using withCheckedContinuation (wrapping
apollo.fetch) which works for Apollo iOS 1.x but leaves a cancellation gap where
cancelling the calling Task does not cancel the underlying HTTP request; fix by
upgrading to Apollo iOS 2.x and replacing the continuation wrapper with the
native async API (call try await apollo.fetch(query:) directly in the
GraphQLContentClient method, convert the method signature to async/throws as
needed, remove the withCheckedContinuation block and associated callback
handling, and ensure the client code uses the Sendable-aware Apollo client
surface so Task cancellation propagates).
- Line 63: The id assignment using String(experience.documentId) is a redundant
cast because ForgeSchema.ID is a String typealias; update the id initializer in
GraphQLContentClient (the assignment that uses experience.documentId) to pass
experience.documentId directly (remove the String(...) wrapper) so the field
uses the existing String value without re-casting.
- Line 63: The id field is being wrapped in an unnecessary String(...) when
ForgeSchema.ID is already a typealias for String; locate the
initializer/assignment where you set id: String(experience.documentId) in
GraphQLContentClient.swift and replace it with the direct value id:
experience.documentId (remove the String(...) wrapper) so the code uses the
existing String-typed ForgeSchema.ID without a redundant conversion.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
mobile/ios/Sources/ForgeMobile/Generated/Operations/Queries/GetWatchExperienceQuery.graphql.swiftis excluded by!**/generated/**
📒 Files selected for processing (5)
.github/workflows/ci.ymlmobile/ios/GraphQL/Operations/GetWatchExperience.graphqlmobile/ios/Package.swiftmobile/ios/README.mdmobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- mobile/ios/Package.swift
- mobile/ios/README.md
- mobile/ios/GraphQL/Operations/GetWatchExperience.graphql
…QLContentClient Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ios/Sources/ForgeMobile/GraphQLContentClient.swift`:
- Around line 18-22: The current headers assignment in
GraphQLContentClient.swift sets Authorization even for blank bearerToken; change
the conditional that builds headers to require a non-empty, non-whitespace token
(e.g., guard let token = bearerToken, !token.trimmingCharacters(in:
.whitespacesAndNewlines).isEmpty) before adding ["Authorization": "Bearer
\(token)"], otherwise return an empty dictionary; update the code that defines
headers (the let headers ... using bearerToken) to perform this check so blank
tokens do not produce an Authorization header.
- Around line 49-55: The success case in the response handling (case
.success(let graphQLResult) inside GraphQLContentClient) only throws GraphQL
errors when data is nil, which lets responses with partial data and errors slip
through; update the logic to check graphQLResult.errors first (if let errors =
graphQLResult.errors, !errors.isEmpty) and throw
GraphQLContentClientError.graphQLErrors(errors) even when graphQLResult.data is
non-nil, then proceed to guard-let data from graphQLResult.data and return it as
before.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift
… error text - ForgeRootView: optional ContentRepository, fetch home on appear, show result/error - ForgeApp: GraphQL client with token from env or build-generated StrapiToken.swift - Run Script: read STRAPI_FULL_ACCESS_TOKEN from apps/cms/.env, write Generated/StrapiToken.swift - GraphQLContentClientError: LocalizedError with real Strapi error message - Info.plist: ATS exception for localhost; gitignore Generated/ Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift (2)
18-22:⚠️ Potential issue | 🟡 MinorSkip
Authorizationwhen token is empty/whitespace.Line 18 currently adds bearer auth for any non-
niltoken, including blank values.🛠️ Suggested fix
- let headers: [String: String] = if let token = bearerToken { - ["Authorization": "Bearer \(token)"] + let trimmedToken = bearerToken?.trimmingCharacters(in: .whitespacesAndNewlines) + let headers: [String: String] = if let token = trimmedToken, !token.isEmpty { + ["Authorization": "Bearer \(token)"] } else { [:] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift` around lines 18 - 22, The current headers construction in GraphQLContentClient adds an Authorization header for any non-nil bearerToken including empty or whitespace-only strings; change the logic that builds headers (the let headers: [String: String] assignment that references bearerToken) to trim whitespace and only include ["Authorization": "Bearer \(token)"] when token.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty is false, otherwise return an empty dictionary. Ensure you reference bearerToken and the headers binding so the header is entirely skipped for nil/blank tokens.
49-55:⚠️ Potential issue | 🟠 MajorThrow GraphQL errors even when
datais present.Line 50 only throws
graphQLErrorswhendataisnil; GraphQL responses can contain partial data plus errors, which should not be silently accepted.🛠️ Suggested fix
switch result { case .success(let graphQLResult): - guard let data = graphQLResult.data else { - if let errors = graphQLResult.errors, !errors.isEmpty { - throw GraphQLContentClientError.graphQLErrors(errors) - } - return nil - } + if let errors = graphQLResult.errors, !errors.isEmpty { + throw GraphQLContentClientError.graphQLErrors(errors) + } + guard let data = graphQLResult.data else { return nil } guard let first = data.experiences.compactMap({ $0 }).first else { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift` around lines 49 - 55, The GraphQL response handling in GraphQLContentClient currently only throws graphQLErrors when data == nil; update the success branch that handles graphQLResult in the response parsing (the case .success(let graphQLResult) block) to check graphQLResult.errors even when data is non-nil and throw GraphQLContentClientError.graphQLErrors(errors) if errors exist and are not empty, before returning the partial data; keep the existing behavior of returning nil only when data is nil and there are no errors.
🧹 Nitpick comments (2)
mobile/ios/App/ForgeApp/Info.plist (1)
5-15: Limit ATS localhost exception to Debug builds only.Line 5–15 currently applies cleartext localhost traffic allowance to Release too (same plist for all configs). Prefer a Debug-only plist (or config-gated ATS setting) so production builds don’t carry this exception.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/App/ForgeApp/Info.plist` around lines 5 - 15, The Info.plist currently grants NSExceptionAllowsInsecureHTTPLoads for localhost globally; restrict this to Debug by creating a Debug-only plist or by moving the NSAppTransportSecurity/NSExceptionDomains/localhost entry into a Debug-specific file (e.g., Info-Debug.plist) and configuring the Xcode target to use that plist for the Debug build configuration (or alternatively inject the NSExceptionAllowsInsecureHTTPLoads key via a Debug xcconfig/Build Settings plist merge), ensuring the main Info.plist used for Release does not contain NSAppTransportSecurity/NSExceptionAllowsInsecureHTTPLoads for localhost; update references to the symbols NSAppTransportSecurity, NSExceptionDomains, localhost, and NSExceptionAllowsInsecureHTTPLoads when making the change.mobile/ios/Sources/ForgeMobile/ForgeRootView.swift (1)
11-13: GateGraphQLTestViewbehind a debug/feature flag.Because
ForgeAppalways injects a repository, this path becomes the default runtime UI. Prefer#if DEBUGor an explicit feature flag for this temporary validation surface.Also applies to: 20-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/ForgeRootView.swift` around lines 11 - 13, The GraphQLTestView is always shown because ForgeApp always injects contentRepository; gate its rendering in ForgeRootView by adding a debug/feature-flag check around the GraphQLTestView usage (either wrap the repository branch that returns GraphQLTestView inside a compile-time `#if` DEBUG block or consult an explicit feature flag boolean like isGraphQLTestEnabled injected into ForgeRootView/environment) and fall back to the existing production UI when the flag is false; update both the initial occurrence and the repeated block spanning lines 20–57 so only when DEBUG or isGraphQLTestEnabled is true will GraphQLTestView(repository: repo) be created.
🤖 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/ios/App/ForgeApp.xcodeproj/project.pbxproj`:
- Around line 140-145: The build currently generates and compiles a
high-privilege credential into the app via the "Generate Strapi token" build
phase that outputs StrapiToken.swift and then includes it in the target; remove
this build phase and stop compiling StrapiToken.swift, and instead switch to a
runtime-safe approach: have the app fetch short-lived or scoped tokens from a
secure backend endpoint (or use Keychain/secure enclave for tokens obtained at
runtime), ensure the code that referenced StrapiToken.swift is updated to call
the runtime token provider, and add a lint/check to prevent regenerating/adding
any apps/* env secrets into mobile/** in future.
In `@mobile/ios/App/ForgeApp/ForgeApp.swift`:
- Line 6: Replace the hardcoded static URL by loading the GraphQL endpoint from
app configuration (Info.plist / build configs) and handling it safely: read
Bundle.main.object(forInfoDictionaryKey:) for a key like "GraphQLEndpoint",
validate and create a URL (avoid force-unwrapping), provide a sensible fallback
or fatal error with a clear message for missing/invalid values, and use that URL
when initializing GraphQLContentClient (update the static let graphQLURL and the
GraphQLContentClient initialization sites referenced in ForgeApp.swift lines
~14-19 to use the configured value).
---
Duplicate comments:
In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift`:
- Around line 18-22: The current headers construction in GraphQLContentClient
adds an Authorization header for any non-nil bearerToken including empty or
whitespace-only strings; change the logic that builds headers (the let headers:
[String: String] assignment that references bearerToken) to trim whitespace and
only include ["Authorization": "Bearer \(token)"] when
token.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty is false,
otherwise return an empty dictionary. Ensure you reference bearerToken and the
headers binding so the header is entirely skipped for nil/blank tokens.
- Around line 49-55: The GraphQL response handling in GraphQLContentClient
currently only throws graphQLErrors when data == nil; update the success branch
that handles graphQLResult in the response parsing (the case .success(let
graphQLResult) block) to check graphQLResult.errors even when data is non-nil
and throw GraphQLContentClientError.graphQLErrors(errors) if errors exist and
are not empty, before returning the partial data; keep the existing behavior of
returning nil only when data is nil and there are no errors.
---
Nitpick comments:
In `@mobile/ios/App/ForgeApp/Info.plist`:
- Around line 5-15: The Info.plist currently grants
NSExceptionAllowsInsecureHTTPLoads for localhost globally; restrict this to
Debug by creating a Debug-only plist or by moving the
NSAppTransportSecurity/NSExceptionDomains/localhost entry into a Debug-specific
file (e.g., Info-Debug.plist) and configuring the Xcode target to use that plist
for the Debug build configuration (or alternatively inject the
NSExceptionAllowsInsecureHTTPLoads key via a Debug xcconfig/Build Settings plist
merge), ensuring the main Info.plist used for Release does not contain
NSAppTransportSecurity/NSExceptionAllowsInsecureHTTPLoads for localhost; update
references to the symbols NSAppTransportSecurity, NSExceptionDomains, localhost,
and NSExceptionAllowsInsecureHTTPLoads when making the change.
In `@mobile/ios/Sources/ForgeMobile/ForgeRootView.swift`:
- Around line 11-13: The GraphQLTestView is always shown because ForgeApp always
injects contentRepository; gate its rendering in ForgeRootView by adding a
debug/feature-flag check around the GraphQLTestView usage (either wrap the
repository branch that returns GraphQLTestView inside a compile-time `#if` DEBUG
block or consult an explicit feature flag boolean like isGraphQLTestEnabled
injected into ForgeRootView/environment) and fall back to the existing
production UI when the flag is false; update both the initial occurrence and the
repeated block spanning lines 20–57 so only when DEBUG or isGraphQLTestEnabled
is true will GraphQLTestView(repository: repo) be created.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.gitignoreapps/cms/.env.examplemobile/ios/App/ForgeApp.xcodeproj/project.pbxprojmobile/ios/App/ForgeApp/ForgeApp.swiftmobile/ios/App/ForgeApp/Info.plistmobile/ios/README.mdmobile/ios/Sources/ForgeMobile/ContentRepository.swiftmobile/ios/Sources/ForgeMobile/ForgeRootView.swiftmobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- mobile/ios/README.md
- Info-Debug.plist: localhost HTTP exception for local Strapi (Debug only) - Info-Release.plist: no ATS override for Archive / App Store Connect - Project: INFOPLIST_FILE per configuration; README documents plist usage Made-with: Cursor
- Sources/ForgeMobile/Generated (Apollo) - App/ForgeApp/Generated (StrapiToken.swift) so pnpm lint passes for @forge/mobile-ios Made-with: Cursor
- GraphQLContentClient: only set Authorization when token non-empty (trim whitespace) - GraphQLContentClient: throw graphQLErrors when errors present even if data exists - GraphQLContentClient: use experience.documentId directly (ID is String) - GetWatchExperience.graphql: remove redundant __typename (Apollo injects) - ForgeRootView: show GraphQLTestView only in DEBUG builds - ForgeApp: document localhost URL as default for local dev - Run Script: omit token in Release so app binary never contains credential Made-with: Cursor
Contributor
Author
Review feedback addressed (cae66e8)Fixed:
Not changed:
|
… from plist - Token phase: only emit nil stub; never read apps/cms/.env (Critical) - ForgeApp: read GraphQLEndpoint from Info.plist; Debug fallback, Release requires value - GraphQLContentClient: simplify withCheckedContinuation (nitpick) Made-with: Cursor
Contributor
Author
Review feedback addressed (383815e)Fixed:
Not changed:
Please mark the related review threads as resolved when satisfied. |
…or runs - Build phase reads STRAPI_FULL_ACCESS_TOKEN from apps/cms/.env in Debug only - Release always emits nil so shipped binary has no token - README: document token source and Forbidden access troubleshooting Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift (1)
41-45: Handle cooperative task cancellation ingetContent.The
apollo.fetch()method returns aCancellablethat is not captured or managed. When a task callinggetContentis cancelled, the fetch continues running in the background. Add cancellation checks to make the method cooperatively cancellable.♻️ Proposed change
public func getContent(locale: String, slug: String) async throws -> MobileContentItem? { + try Task.checkCancellation() let filters = ForgeSchema.ExperienceFiltersInput( slug: .some(ForgeSchema.StringFilterInput(eq: .some(slug))) ) @@ let result = await withCheckedContinuation { continuation in apollo.fetch(query: query, cachePolicy: .fetchIgnoringCacheData) { result in continuation.resume(returning: result) } } + try Task.checkCancellation() switch result {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift` around lines 41 - 45, Capture the Cancellable returned by apollo.fetch in getContent and cancel it when the Swift task is cancelled: wrap the await withCheckedContinuation block in a withTaskCancellationHandler (or use withCheckedThrowingContinuation + withTaskCancellationHandler) so the cancellation handler calls cancellable.cancel(), store the result of apollo.fetch in a local var (e.g., cancellable) inside the continuation, and ensure you only resume the continuation once (guard against double-resume) when the apollo callback completes or when cancellation occurs; reference getContent, apollo.fetch, withCheckedContinuation, and the Cancellable to locate and update the code.
🤖 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/ios/Sources/ForgeMobile/ForgeRootView.swift`:
- Around line 25-63: The GraphQLTestView struct (and its async fetch logic /
ContentRepository dependency) must be wrapped in a compiler condition so it only
exists in debug builds: move the entire GraphQLTestView definition inside a `#if`
DEBUG ... `#endif` block (so GraphQLTestView, its `@State` properties and .task
async call are not compiled into Release). Also make ForgeRootView’s
contentRepository storage and initializer conditional (`#if` DEBUG) so release
builds don’t keep dead storage or init parameters: define contentRepository and
the init parameter only under `#if` DEBUG and provide a parameterless public init
in the `#else` branch.
---
Nitpick comments:
In `@mobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift`:
- Around line 41-45: Capture the Cancellable returned by apollo.fetch in
getContent and cancel it when the Swift task is cancelled: wrap the await
withCheckedContinuation block in a withTaskCancellationHandler (or use
withCheckedThrowingContinuation + withTaskCancellationHandler) so the
cancellation handler calls cancellable.cancel(), store the result of
apollo.fetch in a local var (e.g., cancellable) inside the continuation, and
ensure you only resume the continuation once (guard against double-resume) when
the apollo callback completes or when cancellation occurs; reference getContent,
apollo.fetch, withCheckedContinuation, and the Cancellable to locate and update
the code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
mobile/ios/.swiftlint.ymlmobile/ios/App/ForgeApp.xcodeproj/project.pbxprojmobile/ios/App/ForgeApp/ForgeApp.swiftmobile/ios/App/ForgeApp/Info-Debug.plistmobile/ios/App/ForgeApp/Info-Release.plistmobile/ios/GraphQL/Operations/GetWatchExperience.graphqlmobile/ios/README.mdmobile/ios/Sources/ForgeMobile/ForgeRootView.swiftmobile/ios/Sources/ForgeMobile/GraphQLContentClient.swift
✅ Files skipped from review due to trivial changes (1)
- mobile/ios/App/ForgeApp/Info-Release.plist
🚧 Files skipped from review as they are similar to previous changes (4)
- mobile/ios/GraphQL/Operations/GetWatchExperience.graphql
- mobile/ios/App/ForgeApp/ForgeApp.swift
- mobile/ios/App/ForgeApp.xcodeproj/project.pbxproj
- mobile/ios/README.md
…odels/Views structure Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
mobile/ios/Sources/ForgeMobile/ViewModels/WatchHomeViewModel.swift (1)
17-30: Minor: RedundanthomeError = nilassignment on line 25.Line 19 already sets
homeError = nilat the start ofload(). The assignment on line 25 after success is redundant.Proposed simplification
do { let item = try await repository.fetchHome(locale: locale) homeItem = item - homeError = nil } catch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/ViewModels/WatchHomeViewModel.swift` around lines 17 - 30, In load(locale:) remove the redundant homeError = nil after a successful fetch: keep the initial homeError = nil at the start of the method, assign homeItem = item on success and only set homeError in the catch block; update the success path in the load(locale:) function (which calls repository.fetchHome) to drop the unnecessary homeError = nil assignment so the state logic relies on the initial reset and the catch-only error assignment.mobile/ios/App/ForgeApp/ForgeApp.swift (1)
4-11: Repository may be recreated on each body evaluation.
AppContentRepositoryFactory.makeContentRepository()is called inside thebodycomputed property. SwiftUI may re-evaluatebodymultiple times, potentially creating newApolloClientinstances and losing cached data.Consider storing the repository as a property to ensure a single instance:
Proposed fix
`@main` struct ForgeApp: App { + private let contentRepository = AppContentRepositoryFactory.makeContentRepository() + var body: some Scene { WindowGroup { - ForgeRootView(contentRepository: AppContentRepositoryFactory.makeContentRepository()) + ForgeRootView(contentRepository: contentRepository) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/App/ForgeApp/ForgeApp.swift` around lines 4 - 11, The App currently calls AppContentRepositoryFactory.makeContentRepository() inside ForgeApp.body which can be evaluated multiple times and recreate the repository (and ApolloClient); move the repository creation out of the body into a stored property on ForgeApp (e.g., a let or lazy var property like contentRepository) and then pass that property into ForgeRootView(contentRepository:) so the repository/ApolloClient is created once and reused across body reevaluations.mobile/ios/Sources/ForgeMobile/Views/ForgeRootView.swift (1)
3-10: Consider makingcontentRepositoryandviewModelDEBUG-only to avoid dead storage in Release.In Release builds,
contentRepositoryandviewModelare stored but never used (sinceGraphQLTestViewis only instantiated in DEBUG). This creates unnecessary memory overhead.Proposed conditional compilation
public struct ForgeRootView: View { + `#if` DEBUG private let contentRepository: ContentRepository? `@State` private var viewModel: WatchHomeViewModel? public init(contentRepository: ContentRepository? = nil) { self.contentRepository = contentRepository _viewModel = State(initialValue: contentRepository.map { WatchHomeViewModel(repository: $0) }) } + `#else` + public init(contentRepository: ContentRepository? = nil) {} + `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/Views/ForgeRootView.swift` around lines 3 - 10, The ForgeRootView currently stores contentRepository and viewModel unconditionally even though they're only used for GraphQLTestView in DEBUG; remove dead storage in Release by wrapping the contentRepository property, the `@State` viewModel, and the initializer logic that initializes WatchHomeViewModel in `#if` DEBUG / `#endif` blocks (i.e., make contentRepository: ContentRepository?, `@State` private var viewModel: WatchHomeViewModel?, and the init(contentRepository:) only exist under DEBUG) and keep the public API and body for Release unchanged so no DEBUG-only state is allocated when building Release.mobile/ios/Sources/ForgeMobile/Data/GraphQLContentClient.swift (1)
63-101: Consider documenting the title fallback priority.The
firstSectionTitlefunction iterates through sections in order and returns the first non-empty title from media collection, promo banner, info blocks, or CTA. This prioritization is implicit—a brief doc comment explaining the order would aid future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/ios/Sources/ForgeMobile/Data/GraphQLContentClient.swift` around lines 63 - 101, Add a short doc comment above firstSectionTitle describing its fallback priority and behavior: explain that firstSectionTitle(from:) iterates sections in order and returns the first non-empty title it finds using the following precedence: media collection (media.title), promo banner (promo.promoBannerHeading), info blocks (info.infoBlocksHeading), then CTA (cta.ctaHeading), and returns nil if none are present; also note that mapExperienceToContentItem uses this helper to derive the MobileContentItem.title and falls back to experience.slug when nil.
🤖 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/ios/Sources/ForgeMobile/Data/ContentRepository.swift`:
- Around line 22-24: The iOS fetchHome implementation uses slug "experience"
which differs from Android's "home"; update the MobileContentItem retrieval so
fetchHome calls client.getContent(locale:locale, slug: "home") (i.e., change the
slug in the fetchHome function) so both platforms fetch the same content; keep
the function name fetchHome and the client.getContent call unchanged otherwise.
---
Nitpick comments:
In `@mobile/ios/App/ForgeApp/ForgeApp.swift`:
- Around line 4-11: The App currently calls
AppContentRepositoryFactory.makeContentRepository() inside ForgeApp.body which
can be evaluated multiple times and recreate the repository (and ApolloClient);
move the repository creation out of the body into a stored property on ForgeApp
(e.g., a let or lazy var property like contentRepository) and then pass that
property into ForgeRootView(contentRepository:) so the repository/ApolloClient
is created once and reused across body reevaluations.
In `@mobile/ios/Sources/ForgeMobile/Data/GraphQLContentClient.swift`:
- Around line 63-101: Add a short doc comment above firstSectionTitle describing
its fallback priority and behavior: explain that firstSectionTitle(from:)
iterates sections in order and returns the first non-empty title it finds using
the following precedence: media collection (media.title), promo banner
(promo.promoBannerHeading), info blocks (info.infoBlocksHeading), then CTA
(cta.ctaHeading), and returns nil if none are present; also note that
mapExperienceToContentItem uses this helper to derive the
MobileContentItem.title and falls back to experience.slug when nil.
In `@mobile/ios/Sources/ForgeMobile/ViewModels/WatchHomeViewModel.swift`:
- Around line 17-30: In load(locale:) remove the redundant homeError = nil after
a successful fetch: keep the initial homeError = nil at the start of the method,
assign homeItem = item on success and only set homeError in the catch block;
update the success path in the load(locale:) function (which calls
repository.fetchHome) to drop the unnecessary homeError = nil assignment so the
state logic relies on the initial reset and the catch-only error assignment.
In `@mobile/ios/Sources/ForgeMobile/Views/ForgeRootView.swift`:
- Around line 3-10: The ForgeRootView currently stores contentRepository and
viewModel unconditionally even though they're only used for GraphQLTestView in
DEBUG; remove dead storage in Release by wrapping the contentRepository
property, the `@State` viewModel, and the initializer logic that initializes
WatchHomeViewModel in `#if` DEBUG / `#endif` blocks (i.e., make contentRepository:
ContentRepository?, `@State` private var viewModel: WatchHomeViewModel?, and the
init(contentRepository:) only exist under DEBUG) and keep the public API and
body for Release unchanged so no DEBUG-only state is allocated when building
Release.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
mobile/ios/App/ForgeApp.xcodeproj/project.pbxprojmobile/ios/App/ForgeApp/AppContentRepositoryFactory.swiftmobile/ios/App/ForgeApp/ForgeApp.swiftmobile/ios/Sources/ForgeMobile/Data/ContentRepository.swiftmobile/ios/Sources/ForgeMobile/Data/GraphQLContentClient.swiftmobile/ios/Sources/ForgeMobile/ViewModels/WatchHomeViewModel.swiftmobile/ios/Sources/ForgeMobile/Views/ForgeRootView.swift
Made-with: Cursor
Kneesal
pushed a commit
to Kneesal/forge
that referenced
this pull request
Mar 9, 2026
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 #102
Summary
Adds Apollo iOS and codegen for the iOS app: schema from
apps/cms/schema.graphql, one query (GetWatchExperience), generated Swift in ForgeMobile, and aGraphQLContentClientthat implementsContentClientwith configurable endpoint and optional bearer token. Enables the app to fetch watch experience data from the CMS GraphQL API.Contracts Changed
Regeneration Required
Validation
swift buildfrommobile/ios)Summary by CodeRabbit
New Features
Documentation
Chores