fix: new account creation stuck on terms and conditions#8900
Open
fix: new account creation stuck on terms and conditions#8900
Conversation
…n-hangs-during-training-at-film
Contributor
WalkthroughEnhanced error handling in the journey duplication resolver by adding contextual details to exception messages, including userId and user roles. Added diagnostic logging to the component calling the mutation to capture success/failure details for debugging purposes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 1879260
☁️ Nx Cloud last updated this comment at |
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
Contributor
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@apis/api-journeys/src/app/modules/journey/journey.resolver.ts`:
- Around line 625-635: The GraphQL error messages currently include internal
identifiers and raw role lists (e.g., userId, duplicateJourney.team?.userTeams,
duplicateJourney.userJourneys) — change the resolver's error handling to remove
sensitive values from the client-facing message and instead log the full
diagnostic server-side; update the throw/new ApolloError (or equivalent) in the
method that references duplicateJourney to return a sanitized message like "user
is not allowed to duplicate journey" while sending the detailed context (userId,
role arrays, original error) to the server logger (processLogger/errorLogger)
for debugging.
In
`@apps/journeys-admin/src/components/TermsAndConditions/TermsAndConditions.tsx`:
- Around line 96-150: Remove or gate the temporary DIAG console logging in the
try/catch around the journeyDuplicate call: either delete all
console.log/console.error statements in the block that reference
ONBOARDING_TEMPLATE_ID, newTeam.id, result, and the error introspection
(graphQLErrors/networkError), or wrap them behind a development-only check
(e.g., if (process.env.NODE_ENV === 'development') or a feature flag) so the
mutation call (journeyDuplicate) and its error handling stay but verbose
diagnostics are not emitted in production.
🪄 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: c73cc35c-e700-4f0a-a348-8b07d14acc9c
📒 Files selected for processing (2)
apis/api-journeys/src/app/modules/journey/journey.resolver.tsapps/journeys-admin/src/components/TermsAndConditions/TermsAndConditions.tsx
Comment on lines
+625
to
+635
| `user is not allowed to duplicate journey (userId: ${userId}, teamRoles: [${ | ||
| duplicateJourney.team?.userTeams | ||
| ?.filter((ut) => ut.userId === userId) | ||
| .map((ut) => ut.role) | ||
| .join(',') ?? '' | ||
| }], journeyRoles: [${ | ||
| duplicateJourney.userJourneys | ||
| ?.filter((uj) => uj.userId === userId) | ||
| .map((uj) => uj.role) | ||
| .join(',') ?? '' | ||
| }])`, |
Contributor
There was a problem hiding this comment.
Avoid returning internal identifiers and raw error details to clients.
Line 625 and Line 771 now expose internal context (userId, role sets, source/team IDs, original error metadata, and raw error text) in GraphQL responses. That increases information disclosure risk and should stay server-side only.
🔧 Proposed hardening (keep diagnostics in server logs, return sanitized GraphQL errors)
- throw new GraphQLError(
- `user is not allowed to duplicate journey (userId: ${userId}, teamRoles: [${
- duplicateJourney.team?.userTeams
- ?.filter((ut) => ut.userId === userId)
- .map((ut) => ut.role)
- .join(',') ?? ''
- }], journeyRoles: [${
- duplicateJourney.userJourneys
- ?.filter((uj) => uj.userId === userId)
- .map((uj) => uj.role)
- .join(',') ?? ''
- }])`,
- {
- extensions: { code: 'FORBIDDEN' }
- }
- )
+ throw new GraphQLError('user is not allowed to duplicate journey', {
+ extensions: { code: 'FORBIDDEN' }
+ })
@@
- throw new GraphQLError(
- `journeyDuplicate failed: ${err instanceof Error ? err.message : String(err)}`,
- {
- extensions: {
- code: err?.extensions?.code ?? 'INTERNAL_SERVER_ERROR',
- sourceJourneyId: id,
- teamId,
- originalError: err instanceof Error ? err.name : typeof err
- }
- }
- )
+ throw new GraphQLError('journeyDuplicate failed', {
+ extensions: {
+ code: err?.extensions?.code ?? 'INTERNAL_SERVER_ERROR'
+ }
+ })Also applies to: 770-780
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apis/api-journeys/src/app/modules/journey/journey.resolver.ts` around lines
625 - 635, The GraphQL error messages currently include internal identifiers and
raw role lists (e.g., userId, duplicateJourney.team?.userTeams,
duplicateJourney.userJourneys) — change the resolver's error handling to remove
sensitive values from the client-facing message and instead log the full
diagnostic server-side; update the throw/new ApolloError (or equivalent) in the
method that references duplicateJourney to return a sanitized message like "user
is not allowed to duplicate journey" while sending the detailed context (userId,
role arrays, original error) to the server logger (processLogger/errorLogger)
for debugging.
Comment on lines
+96
to
+150
| // DIAGNOSTIC: Wrap in try-catch to see the exact error in browser console | ||
| try { | ||
| console.log('[DIAG] journeyDuplicate: calling with', { | ||
| templateId: ONBOARDING_TEMPLATE_ID, | ||
| teamId: newTeam.id | ||
| }) | ||
| const result = await journeyDuplicate({ | ||
| variables: { | ||
| id: ONBOARDING_TEMPLATE_ID, | ||
| teamId: newTeam.id | ||
| } | ||
| }) | ||
| console.log('[DIAG] journeyDuplicate: SUCCESS', result.data) | ||
| } catch (error: unknown) { | ||
| console.error('[DIAG] journeyDuplicate: FAILED') | ||
| console.error( | ||
| '[DIAG] Error name:', | ||
| error instanceof Error ? error.name : 'unknown' | ||
| ) | ||
| console.error( | ||
| '[DIAG] Error message:', | ||
| error instanceof Error ? error.message : String(error) | ||
| ) | ||
| if ( | ||
| error != null && | ||
| typeof error === 'object' && | ||
| 'graphQLErrors' in error | ||
| ) { | ||
| const gqlErrors = ( | ||
| error as { | ||
| graphQLErrors: Array<{ | ||
| message: string | ||
| extensions?: Record<string, unknown> | ||
| }> | ||
| } | ||
| ).graphQLErrors | ||
| gqlErrors.forEach((gqlErr, i) => { | ||
| console.error( | ||
| `[DIAG] GraphQL error ${i}:`, | ||
| gqlErr.message, | ||
| gqlErr.extensions | ||
| ) | ||
| }) | ||
| } | ||
| if ( | ||
| error != null && | ||
| typeof error === 'object' && | ||
| 'networkError' in error | ||
| ) { | ||
| console.error( | ||
| '[DIAG] Network error:', | ||
| (error as { networkError: unknown }).networkError | ||
| ) | ||
| } | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Remove or gate temporary DIAG console logging before merge.
This block logs detailed mutation diagnostics (teamId, GraphQL errors, network errors) directly in browser console. Keep the non-blocking behavior, but avoid shipping verbose diagnostic logs in production.
🧹 Minimal cleanup while preserving behavior
- // DIAGNOSTIC: Wrap in try-catch to see the exact error in browser console
try {
- console.log('[DIAG] journeyDuplicate: calling with', {
- templateId: ONBOARDING_TEMPLATE_ID,
- teamId: newTeam.id
- })
- const result = await journeyDuplicate({
+ await journeyDuplicate({
variables: {
id: ONBOARDING_TEMPLATE_ID,
teamId: newTeam.id
}
})
- console.log('[DIAG] journeyDuplicate: SUCCESS', result.data)
- } catch (error: unknown) {
- console.error('[DIAG] journeyDuplicate: FAILED')
- console.error(
- '[DIAG] Error name:',
- error instanceof Error ? error.name : 'unknown'
- )
- console.error(
- '[DIAG] Error message:',
- error instanceof Error ? error.message : String(error)
- )
- if (
- error != null &&
- typeof error === 'object' &&
- 'graphQLErrors' in error
- ) {
- const gqlErrors = (
- error as {
- graphQLErrors: Array<{
- message: string
- extensions?: Record<string, unknown>
- }>
- }
- ).graphQLErrors
- gqlErrors.forEach((gqlErr, i) => {
- console.error(
- `[DIAG] GraphQL error ${i}:`,
- gqlErr.message,
- gqlErr.extensions
- )
- })
- }
- if (
- error != null &&
- typeof error === 'object' &&
- 'networkError' in error
- ) {
- console.error(
- '[DIAG] Network error:',
- (error as { networkError: unknown }).networkError
- )
- }
- }
+ } catch {
+ // Intentionally non-blocking: allow onboarding to continue
+ // even if template duplication fails.
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/journeys-admin/src/components/TermsAndConditions/TermsAndConditions.tsx`
around lines 96 - 150, Remove or gate the temporary DIAG console logging in the
try/catch around the journeyDuplicate call: either delete all
console.log/console.error statements in the block that reference
ONBOARDING_TEMPLATE_ID, newTeam.id, result, and the error introspection
(graphQLErrors/networkError), or wrap them behind a development-only check
(e.g., if (process.env.NODE_ENV === 'development') or a feature flag) so the
mutation call (journeyDuplicate) and its error handling stay but verbose
diagnostics are not emitted in production.
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 by CodeRabbit