Conversation
…enticated When a guest (anonymous) user links their Firebase account to Google via linkWithPopup, the existing emailVerified sync branch in findOrFetchUser already calls auth.getUser() — but discarded the displayName, email, and photoURL from that response. This meant the DB record kept firstName as "Unknown User" and imageUrl as null after conversion. Fix: capture and write displayName (parsed into firstName/lastName), email, and photoURL to the DB in the same update that sets emailVerified: true. Belt-and-suspenders: also include imageUrl in the me query conversion branch so any JWT-derived imageUrl is preserved for edge cases where findOrFetchUser's emailVerified branch doesn't trigger. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
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:
WalkthroughDetects when a Firebase user’s Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UI as SignInServiceButton
participant AuthClient as Firebase Client
participant Server as /api/login (me resolver)
participant AdminAuth as Firebase Admin
participant DB as Prisma
Client->>UI: completes Google linkWithPopup
UI->>AuthClient: credential.user.getIdToken(true)
AuthClient-->>UI: fresh idToken
UI->>Server: POST /api/login with idToken
Server->>AdminAuth: getUserFromPayload / auth.getUser(uid)
AdminAuth-->>Server: uid, emailVerified, displayName, email, photoURL
alt existing DB user emailVerified == false and auth.emailVerified == true
Server->>DB: prisma.user.update(where:{userId}, data:{ emailVerified:true, email, firstName, lastName, imageUrl })
DB-->>Server: updated user
else guest -> authenticated with imageUrl
Server->>DB: prisma.user.update(..., data:{ imageUrl })
DB-->>Server: updated user
end
Server-->>Client: user record / session established
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
View your CI Pipeline Execution ↗ for commit cd14881
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apis/api-users/src/schema/user/findOrFetchUser.ts (1)
33-60: Extract duplicated displayName parsing into a helper.The nested-ternary parsing here (lines 40-49) duplicates the clearer
if/elselogic at lines 77-90 in the same file. This is a good opportunity to DRY both branches into a small helper that returns{ firstName, lastName }— it also improves readability since the nested ternary is harder to reason about than the equivalent block below.♻️ Proposed refactor
+function parseDisplayName(displayName?: string | null): { + firstName?: string + lastName?: string +} { + const parts = + displayName?.trim().split(' ').filter((p) => p.length > 0) ?? [] + if (parts.length === 0) return {} + if (parts.length === 1) return { firstName: parts[0] } + return { + firstName: parts.slice(0, -1).join(' '), + lastName: parts[parts.length - 1] + } +}Then at lines 33-60:
- const { - emailVerified, - displayName, - email, - photoURL - } = await auth.getUser(userId) + const { emailVerified, displayName, email, photoURL } = + await auth.getUser(userId) if (emailVerified) { - const nameParts = - displayName?.trim().split(' ').filter((p) => p.length > 0) ?? [] - const firstName = - nameParts.length >= 1 - ? nameParts.length === 1 - ? nameParts[0] - : nameParts.slice(0, -1).join(' ') - : undefined - const lastName = - nameParts.length > 1 ? nameParts[nameParts.length - 1] : undefined - + const { firstName, lastName } = parseDisplayName(displayName) return await prisma.user.update({The fallthrough creation branch at lines 73-95 can then reuse the same helper (applying its own
'Unknown User'fallback after the call).Also worth noting: this branch makes a second
auth.getUser(userId)call in addition to the one at line 71 in the creation path. For the verified-flip path that's fine since they're mutually exclusive code paths, just calling it out so it's intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-users/src/schema/user/findOrFetchUser.ts` around lines 33 - 60, The displayName parsing logic is duplicated (a nested ternary in the emailVerified/update path and a clearer if/else in the creation path); extract that into a small helper (e.g., parseDisplayName(displayName): { firstName?: string; lastName?: string }) and replace the nested ternary in the auth.getUser -> prisma.user.update branch with a call to parseDisplayName, and reuse the same helper in the prisma.user.create branch (apply the existing 'Unknown User' fallback after calling the helper in the create path); update references in findOrFetchUser.ts so auth.getUser, prisma.user.update, and prisma.user.create use the helper and remove the duplicated parsing code.apis/api-users/src/schema/user/findOrFetchUser.spec.ts (1)
53-85: LGTM.Test cleanly asserts the new conditional-spread payload (emailVerified + email + firstName + lastName + imageUrl) and the
whereclause. Good coverage for the happy path of the emailVerified-flip branch.One optional add for later: a case where
displayNameis a single token (assertsfirstNameset,lastNameomitted) and one wherephotoURL/data) would lock in the conditional-spread behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-users/src/schema/user/findOrFetchUser.spec.ts` around lines 53 - 85, Add two additional unit tests in findOrFetchUser.spec.ts targeting the findOrFetchUser behavior when emailVerified flips: (1) mock auth.getUser to return a single-token displayName (e.g., "Cher") and assert prismaMock.user.update is called with firstName set and lastName omitted; (2) mock auth.getUser to return null/undefined for photoURL and/or email and assert prismaMock.user.update's data object omits imageUrl and/or email keys. Use the same setup pattern as the existing "emailVerified transitions to true" test and assert the exact payload passed to prismaMock.user.update and the returned value from findOrFetchUser.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apis/api-users/src/schema/user/findOrFetchUser.spec.ts`:
- Around line 53-85: Add two additional unit tests in findOrFetchUser.spec.ts
targeting the findOrFetchUser behavior when emailVerified flips: (1) mock
auth.getUser to return a single-token displayName (e.g., "Cher") and assert
prismaMock.user.update is called with firstName set and lastName omitted; (2)
mock auth.getUser to return null/undefined for photoURL and/or email and assert
prismaMock.user.update's data object omits imageUrl and/or email keys. Use the
same setup pattern as the existing "emailVerified transitions to true" test and
assert the exact payload passed to prismaMock.user.update and the returned value
from findOrFetchUser.
In `@apis/api-users/src/schema/user/findOrFetchUser.ts`:
- Around line 33-60: The displayName parsing logic is duplicated (a nested
ternary in the emailVerified/update path and a clearer if/else in the creation
path); extract that into a small helper (e.g., parseDisplayName(displayName): {
firstName?: string; lastName?: string }) and replace the nested ternary in the
auth.getUser -> prisma.user.update branch with a call to parseDisplayName, and
reuse the same helper in the prisma.user.create branch (apply the existing
'Unknown User' fallback after calling the helper in the create path); update
references in findOrFetchUser.ts so auth.getUser, prisma.user.update, and
prisma.user.create use the helper and remove the duplicated parsing code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a08dbe78-2552-4df4-9663-136b1fd71f22
📒 Files selected for processing (4)
apis/api-users/src/schema/user/findOrFetchUser.spec.tsapis/api-users/src/schema/user/findOrFetchUser.tsapis/api-users/src/schema/user/user.spec.tsapis/api-users/src/schema/user/user.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/api-users/src/schema/user/findOrFetchUser.ts`:
- Around line 47-57: The update path for syncing Google display names leaves
lastName undefined for single-part names, so existing lastName is preserved;
change the logic in findOrFetchUser so lastName is set to an empty string for
single-part display names instead of undefined (i.e., compute lastName =
nameParts.length > 1 ? nameParts[nameParts.length - 1] : '' and ensure
prisma.user.update includes lastName unconditionally like the other fields), so
the update clears any stale lastName when only a first name is provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 383bc854-76d1-4aef-806e-6f89aaf37c1f
📒 Files selected for processing (1)
apis/api-users/src/schema/user/findOrFetchUser.ts
…e syncs immediately Without forceRefresh:true, getIdToken() returns the cached anonymous token after linkWithPopup, leaving ctx.currentUser without name/picture/email claims on the first post-conversion request. The me resolver fallback could not write imageUrl, and the findOrFetchUser Path C fix was the only safety net (which worked but had a narrow Firebase Admin propagation race window). Force-refreshing the token after linkWithPopup guarantees the session cookie carries Google's name, picture, and email claims, making both the findOrFetchUser sync and the me resolver fallback reliable on the very first page load after conversion. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/journeys-admin/src/components/SignIn/SignInServiceButton/SignInServiceButton.spec.tsx (1)
311-315: Use the standard journeys-admin test providers for this added render.This new render only wraps
MockedProvider; please use the repo’s standard helper/wrapper or add the required provider stack so the test runs under the same Snackbar and MUI theme context as production.As per coding guidelines,
apps/journeys-admin/**/*.spec.tsx: “Tests must wrap components with required providers: SnackbarProvider → MockedProvider → ThemeProvider”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/SignIn/SignInServiceButton/SignInServiceButton.spec.tsx` around lines 311 - 315, The test renders SignInServiceButton only with MockedProvider but must use the repo's standard provider stack; update the render call so the component is wrapped inside SnackbarProvider → MockedProvider → ThemeProvider (the same helper or wrapper used by other journeys-admin tests) so SignInServiceButton receives Snackbar and MUI theme context during tests.
🤖 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/journeys-admin/src/components/SignIn/SignInServiceButton/SignInServiceButton.spec.tsx`:
- Around line 311-315: The test renders SignInServiceButton only with
MockedProvider but must use the repo's standard provider stack; update the
render call so the component is wrapped inside SnackbarProvider → MockedProvider
→ ThemeProvider (the same helper or wrapper used by other journeys-admin tests)
so SignInServiceButton receives Snackbar and MUI theme context during tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 480d36fe-487c-472d-9973-f34c2cbd6439
📒 Files selected for processing (4)
apps/journeys-admin/src/components/SignIn/SignInServiceButton/SignInServiceButton.spec.tsxapps/journeys-admin/src/components/SignIn/SignInServiceButton/SignInServiceButton.tsxapps/journeys-admin/src/libs/auth/firebase.tsdocs/plans/2026-04-23-001-fix-guest-google-profile-sync-plan.md
✅ Files skipped from review due to trivial changes (1)
- apps/journeys-admin/src/libs/auth/firebase.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-04-23-001-fix-guest-google-profile-sync-plan.md`:
- Line 133: Replace the incorrect claim about forcing ID token refresh: remove
or update the sentence referencing "token force-refresh after account linking"
and instead instruct callers to call user.reload() to sync server-side profile
changes and then call getIdToken() (without the force true flag); avoid
recommending getIdToken(true) to prevent unnecessary network calls and quota
use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1915e5a6-db57-4b9c-a5f1-52780ed26dd8
📒 Files selected for processing (1)
docs/plans/2026-04-23-001-fix-guest-google-profile-sync-plan.md
| - `user.ts` me resolver: `apis/api-users/src/schema/user/user.ts:122` | ||
| - `firebase.ts` loginWithCredential: `apps/journeys-admin/src/libs/auth/firebase.ts:28` | ||
| - `SignInServiceButton.tsx` pending journey path: `apps/journeys-admin/src/components/SignIn/SignInServiceButton/SignInServiceButton.tsx:55` | ||
| - Firebase docs: token force-refresh after account linking operations is recommended when the new credential's claims must be reflected immediately |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Firebase Auth documentation: after account linking (e.g., linkWithPopup), is forcing ID token refresh via getIdToken(true) recommended when updated claims (name/picture/email) are needed immediately?
💡 Result:
No, forcing ID token refresh via getIdToken(true) is not generally recommended after account linking with linkWithPopup for immediate access to updated claims like name, picture, or email. The linkWithPopup operation returns a UserCredential with an updated user object (result.user) that reflects the linked providers in providerData. Standard claims (name, picture, email) from the new provider are available in the user's providerData array, where you can access the most relevant one (e.g., the last entry or filter by providerId). The ID token includes these claims based on the user's current profile, which may update to prioritize the primary provider's info after linking, but to ensure the latest server-side profile and token claims immediately, call user.reload first, then getIdToken (without forceRefresh, as it auto-refreshes if needed). getIdToken(true) forces a network refresh regardless of expiration, which can lead to quota issues if overused, as noted in docs and community reports. Official docs on linkWithPopup show accessing result.user directly post-linking without mentioning token refresh needs. For custom claims or guaranteed server sync, reload + getIdToken is the pattern used after server-side changes.
Citations:
- 1: https://firebase.google.com/docs/auth/web/account-linking
- 2: https://firebase.google.com/docs/reference/js/auth.user
- 3: https://stackoverflow.com/questions/44490036/force-an-update-to-firebase-user-profile-photo-after-first-logging-in-with-email
- 4: https://modularfirebase.web.app/reference/auth.linkwithpopup
- 5: Custom Claims Disappear from ID Token After updateEmail / updateProfile Calls, Requiring Full Re-authentication firebase/firebase-js-sdk#9260
Correct the claim about Firebase force-refresh recommendations.
The statement in line 133 contradicts Firebase's actual guidance. Per Firebase documentation, forcing ID token refresh via getIdToken(true) after account linking is not the recommended pattern. Instead, Firebase recommends calling user.reload() first to sync server-side profile updates, then getIdToken() without force refresh. The force parameter should be avoided as it can cause quota issues with unnecessary network calls. Replace line 133 with the correct pattern or remove the unsupported claim entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-04-23-001-fix-guest-google-profile-sync-plan.md` at line 133,
Replace the incorrect claim about forcing ID token refresh: remove or update the
sentence referencing "token force-refresh after account linking" and instead
instruct callers to call user.reload() to sync server-side profile changes and
then call getIdToken() (without the force true flag); avoid recommending
getIdToken(true) to prevent unnecessary network calls and quota use.
Contributor
Author
There was a problem hiding this comment.
Keeping the force-refresh intentionally, but scoped to a single call site.
During manual testing we confirmed that after linkWithPopup + updateProfile, a plain getIdToken() returns a cached anonymous token — Firebase auto-refreshes on expiry, not on profile changes. Without force-refresh the JWT sent to Apollo and exchanged by /api/login lacked the name/picture claims, which was the original bug's root cause.
The code now calls getIdToken(true) once per guest-link flow in SignInServiceButton.tsx:70 (one call per sign-in, not per request), so the quota concern doesn't apply. loginWithCredential was reverted to plain getIdToken() for the non-guest path. The plan doc is outdated relative to the shipped code.
Contributor
There was a problem hiding this comment.
@csiyang, understood — that's a well-reasoned explanation. You're right that Firebase's token cache is expiry-driven, not event-driven, so getIdToken() without force-refresh can return a stale anonymous token even immediately after linkWithPopup. Scoping the getIdToken(true) call to a single site in the guest-conversion path (once per sign-in, not per request) is the correct tradeoff and addresses the quota concern. I'll drop my objection.
✏️ Learnings added
Learnt from: csiyang
Repo: JesusFilm/core PR: 9066
File: docs/plans/2026-04-23-001-fix-guest-google-profile-sync-plan.md:133-133
Timestamp: 2026-04-24T00:14:43.029Z
Learning: In `apps/journeys-admin/src/components/SignIn/SignInServiceButton/SignInServiceButton.tsx`, calling `getIdToken(true)` (force-refresh) after `linkWithPopup` in the guest-conversion path is intentional and required. Manual testing confirmed that after `linkWithPopup` + `updateProfile`, a plain `getIdToken()` returns a cached anonymous token because Firebase only auto-refreshes the token on expiry, not on profile changes or account linking. Without force-refresh, the JWT sent to `/api/login` lacks Google `name`/`picture`/`email` claims, which was the root bug (NES-1593). The call is scoped to the guest-link flow only (once per sign-in), so quota concerns do not apply. `loginWithCredential` in `firebase.ts` uses plain `getIdToken()` for non-guest paths. Do not flag this `getIdToken(true)` call as unnecessary or quota-risky in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 8945
File: apps/journeys-admin/src/components/SignIn/PasswordPage/PasswordPage.tsx:51-53
Timestamp: 2026-04-02T01:24:49.916Z
Learning: In `apps/journeys-admin/src/components/SignIn/PasswordPage/PasswordPage.tsx`, calling `firebaseSignOut(auth)` before `signInWithEmailAndPassword` when `auth.currentUser?.isAnonymous === true` is intentional. Firebase requires the anonymous session to be cleared first because `signInWithEmailAndPassword` cannot run alongside an active anonymous session (that path requires `linkWithCredential`). The `pendingGuestJourney` data in `sessionStorage` persists across the sign-out, so a failed sign-in does not permanently orphan the guest journey — the user can retry. Do not flag this pre-sign-in sign-out as a risk of orphaning pending guest journey data.
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 8790
File: apps/journeys-admin/pages/journeys/[journeyId]/reports/visitors.tsx:373-376
Timestamp: 2026-03-07T00:23:36.368Z
Learning: In JesusFilm/core (`apps/journeys-admin`), including the Firebase ID token in `userSerialized` passed through `getServerSideProps` props is intentional and required. The token is needed to hydrate authenticated Apollo/GraphQL client state on initial page load. This is safe because: (1) pages using `getServerSideProps` are rendered per-request and never CDN-cached; (2) unauthenticated users are redirected before props are produced; (3) the Firebase ID token is short-lived (1h TTL) and designed for client-side use; (4) `toUser()` already produces a sanitized shape containing only `id`, `email`, `displayName`, `photoURL`, `phoneNumber`, `emailVerified`, and `token` — no refresh tokens or raw claims. Do not flag this pattern as a security issue in future reviews.
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 8790
File: apps/journeys-admin/pages/teams/new.tsx:88-89
Timestamp: 2026-03-07T00:41:06.607Z
Learning: In JesusFilm/core (`apps/journeys-admin`), do NOT flag `userSerialized: JSON.stringify(user)` in `getServerSideProps` as a security issue even though the `user` object includes a `token` field. Including the Firebase ID token in `userSerialized` is intentional: the token is needed to hydrate authenticated Apollo/GraphQL client state on initial page load. This is safe because pages using `getServerSideProps` are rendered per-request and never CDN-cached; unauthenticated users are redirected before props are produced; the Firebase ID token is short-lived (1h TTL) and designed for client-side use; and `toUser()` already produces a sanitized shape containing only `id`, `email`, `displayName`, `photoURL`, `phoneNumber`, `emailVerified`, and `token` — no refresh tokens or raw claims. This pattern appears consistently across all pages in `apps/journeys-admin/pages/` and must never be flagged as leaking sensitive credentials.
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 8790
File: apps/journeys-admin/src/libs/initAndAuthApp/initAndAuthApp.ts:66-66
Timestamp: 2026-03-07T02:37:26.434Z
Learning: In `apps/journeys-admin/src/libs/initAndAuthApp/initAndAuthApp.ts`, the `token` assignment `const token = user?.token ?? null` at line 66 does NOT need empty/whitespace normalization. The token originates from `toUser(tokens)` → `tokens.token` via `next-firebase-auth-edge`'s `getTokensFromObject()`, which only returns a `Tokens` object when a cryptographically valid Firebase ID JWT is present; it returns `null` otherwise. That `null` case is already handled upstream by `redirectToLogin` in `getAuthTokens`. A Firebase ID token is a JWT (header.payload.signature) and is structurally non-empty by definition. By the time `initAndAuthApp` is called with a non-null user, the token has already been verified. Do not flag empty-string token normalization as an issue in this file.
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/journeys/AGENTS.md:0-0
Timestamp: 2026-04-20T00:11:11.283Z
Learning: Applies to apps/journeys/src/**/*.{ts,tsx} : For anonymous visitors, use `signInAnonymously` with localStorage persistence to prevent duplicate visitor records
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 8790
File: apps/journeys-admin/pages/_app.tsx:25-25
Timestamp: 2026-03-07T00:48:42.538Z
Learning: In JesusFilm/core (apps/journeys-admin), calling `getFirebaseAuth()` at module scope in `apps/journeys-admin/pages/_app.tsx` is safe during SSR. The function uses `initializeApp()` and `getAuth()` from Firebase JS SDK v9+, which are isomorphic — the firebase npm package ships both browser and Node.js bundles, and bare initialization does not access browser-only APIs (localStorage, IndexedDB). Those persistence layers are only engaged during actual auth state checks, which occur client-side. Do not flag this pattern as an SSR safety issue.
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 8814
File: apis/api-journeys-modern/src/schema/journey/adminJourneys.query.ts:37-57
Timestamp: 2026-03-10T22:31:24.117Z
Learning: In `apis/api-journeys-modern/src/schema/journey/adminJourneys.query.ts`, the ordering of the `useLastActiveTeamId` check (Lines 37-44) before the `teamId` check (Line 52), and the behavior of setting `lastActiveApplied = true` even when `profile.lastActiveTeamId` is `null`, is intentional. This is a direct migration of the existing `adminJourneys` query from `api-journeys` to `api-journeys-modern`, and the logic must match the original implementation exactly. Do not flag this ordering or the `lastActiveApplied` flag behavior as a bug in future reviews.
Learnt from: csiyang
Repo: JesusFilm/core PR: 8955
File: apps/journeys-admin/src/libs/checkConditionalRedirect/checkConditionalRedirect.ts:39-42
Timestamp: 2026-04-08T02:23:11.723Z
Learning: In `apps/journeys-admin/src/libs/checkConditionalRedirect/checkConditionalRedirect.ts`, the escape hatch `if (resolvedUrl.startsWith('/users/verify${encodedRedirect}')) return` (around line 81) is never reached from the verify page itself because `apps/journeys-admin/pages/users/verify.tsx` intentionally omits `resolvedUrl` from `initAndAuthApp`, which prevents `checkConditionalRedirect` from being called for the verify page. The primary loop-prevention for the verify page is the earlier check inside the `emailVerified === false` block: `if (resolvedUrl.startsWith('/users/verify')) return`. Do not flag the line-81 escape hatch as a fragile or broken pattern.
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/journeys/AGENTS.md:0-0
Timestamp: 2026-04-20T00:11:11.283Z
Learning: Applies to apps/journeys/src/**/*.{ts,tsx} : Use Apollo Client with Firebase JWT auth via `setContext` link for all GraphQL requests
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 9070
File: apis/api-journeys-modern/src/schema/chatButton/chatButtonUpdate.mutation.ts:27-33
Timestamp: 2026-04-23T20:48:34.812Z
Learning: In `apis/api-journeys-modern/src/schema/chatButton/chatButtonUpdate.mutation.ts`, writing `journeyId` into the Prisma `update` data payload (`data: { journeyId, ...input }`) and calling `recalculateJourneyCustomizable(journeyId)` only for the provided `journeyId` is intentional — it is a faithful 1:1 migration of the legacy `apis/api-journeys` `chatButtonUpdate` resolver which has the same behavior. Do not flag the lack of cross-journey ownership validation or the single-journey recalculation call as bugs in future reviews of this file; any hardening should be tracked as a separate improvement issue.
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apis/AGENTS.md:0-0
Timestamp: 2026-04-01T05:09:07.711Z
Learning: api-gateway: Review changes for header propagation correctness, JWT configuration accuracy, and subgraph naming consistency
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 9070
File: apis/api-journeys-modern/src/schema/chatButton/chatButtonCreate.mutation.ts:12-33
Timestamp: 2026-04-23T20:48:46.619Z
Learning: In `apis/api-journeys-modern/src/schema/chatButton/chatButtonCreate.mutation.ts`, the `chatButtonCreate` mutation intentionally allows anonymous users (`isAnonymous: true`) and performs NO journey-ownership or authorization check beyond the 2-button limit. This is a 1:1 faithful port of the legacy `apis/api-journeys/src/app/modules/chatButton/chatButton.resolver.ts` `chatButtonCreate` method, which also has no `UseGuards`, no CASL guard, and no ownership check. Do not flag the missing ownership/auth check on `chatButtonCreate` as a bug in future reviews; adding such a check would be a new feature, not a migration bug fix.
Contributor
|
The latest updates on your projects.
|
Contributor
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
stage-branch-merger Bot
added a commit
that referenced
this pull request
Apr 23, 2026
…en-signing-up-with' (PR #9066) into stage
stage-branch-merger Bot
added a commit
that referenced
this pull request
Apr 23, 2026
…en-signing-up-with' (PR #9066) into stage
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/journeys-admin/src/components/SignIn/SignInServiceButton/SignInServiceButton.tsx`:
- Around line 43-47: Remove the debug console.log lines that print PII in
SignInServiceButton (the logs referencing user.displayName, user.photoURL,
user.email and providerData) and any JWT payload logging added in firebase.ts;
instead either delete these statements or replace them with non-PII-safe
diagnostics (e.g., a single production-guarded debug flag or a
masked/boolean-only log) so no user identifiers are emitted from the
SignInServiceButton component or firebase JWT handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3d029e3b-b826-4326-94e4-8ee7f7ff03af
📒 Files selected for processing (4)
apis/api-users/src/schema/user/findOrFetchUser.tsapis/api-users/src/schema/user/user.tsapps/journeys-admin/src/components/SignIn/SignInServiceButton/SignInServiceButton.tsxapps/journeys-admin/src/libs/auth/firebase.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apis/api-users/src/schema/user/user.ts
- apps/journeys-admin/src/libs/auth/firebase.ts
- apis/api-users/src/schema/user/findOrFetchUser.ts
Addresses multi-agent review of the guest-to-Google profile sync fix: - Extract splitDisplayName/sanitizeDisplayName/sanitizePhotoURL helpers in libs/yoga/src/firebaseClient so JWT-derived and admin-SDK-derived profile data are sanitized at a single boundary. Photo URLs must be https; display names get control-character stripping and length caps. - findOrFetchUser: drop the hardcoded provider whitelist, match any linked non-firebase provider via providerData; move resolution inside the emailVerified branch to avoid wasted work; share split logic with the create path; import UserRecord through @core/yoga/firebaseClient. - SignInServiceButton: wrap updateProfile/reload in try/catch (best-effort, server has providerData fallback); force-refresh the ID token once before journeyPublish so Apollo attaches fresh name/picture claims; reuse the refreshed token for /api/login instead of minting twice; add isSubmitting guard to prevent double-click races. - firebase.ts: revert loginWithCredential to plain getIdToken() — the force-refresh only belongs at the guest-link call site. - Tests: add providerData fallback case, non-whitelisted provider case, non-https photoURL rejection, control-char stripping, and preserve-existing-name-when-provider-has-none. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
stage-branch-merger Bot
added a commit
that referenced
this pull request
Apr 24, 2026
…en-signing-up-with' (PR #9066) into stage
stage-branch-merger Bot
added a commit
that referenced
this pull request
Apr 24, 2026
…en-signing-up-with' (PR #9066) into stage
Novel institutional knowledge not previously documented anywhere in the codebase: Firebase linkWithPopup on an anonymous user does not hoist the provider's displayName/photoURL onto the top-level user record. The data lives only in providerData[], which the ID token transform does not read — so force-refreshing the JWT cannot surface data that isn't there. Documents: - Why three prior fix attempts (server-only sync, JWT force-refresh, both combined without providerData fallback) each failed - The two-layer working fix (client-side updateProfile + reload, server providerData fallback) - Prevention patterns and test checklist for future Firebase auth work Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
stage-branch-merger Bot
added a commit
that referenced
this pull request
Apr 24, 2026
…en-signing-up-with' (PR #9066) into stage
Contributor
stage-branch-merger Bot
added a commit
that referenced
this pull request
Apr 24, 2026
…en-signing-up-with' (PR #9066) into stage
mikeallisonJS
approved these changes
Apr 24, 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
2 participants
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
Fixes NES-1593
Changes
Test plan
Post-Deploy Monitoring & Validation
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation