fix: team selection race condition in template customization (NES-1465)#8882
fix: team selection race condition in template customization (NES-1465)#8882
Conversation
Journey duplication was sending users' journeys to the wrong team due to two bugs: (1) Formik's enableReinitialize silently reset the team select value whenever async data re-rendered the component, and (2) the handleJourneyDuplication function ignored the form value entirely and always used lastActiveTeamId from the server. - Remove enableReinitialize; gate form render on data readiness - Pass user's selected team from form values to duplication mutation - Validate lastActiveTeamId against available teams (stale ID fallback) - Persist selected team as lastActiveTeamId after successful duplication - Remove setActiveTeam side effect from team dropdown onChange - Add try/catch around duplication to prevent frozen loading state Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Agent 2 - UI Engineer seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
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:
🚥 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)
📝 Coding Plan
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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run journeys-e2e:e2e |
❌ Failed | 3m 21s | View ↗ |
nx run journeys-admin-e2e:e2e |
✅ Succeeded | 32s | View ↗ |
nx run watch-e2e:e2e |
✅ Succeeded | 23s | View ↗ |
nx run player-e2e:e2e |
✅ Succeeded | 3s | View ↗ |
nx run resources-e2e:e2e |
✅ Succeeded | 9s | View ↗ |
nx run videos-admin-e2e:e2e |
✅ Succeeded | 4s | View ↗ |
nx run watch-modern-e2e:e2e |
✅ Succeeded | 3s | View ↗ |
nx run-many --target=vercel-alias --projects=jo... |
✅ Succeeded | 2s | View ↗ |
Additional runs (20) |
✅ Succeeded | ... | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-03-20 08:08:16 UTC
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx`:
- Around line 54-55: The current loading gate (const isDataReady = query?.data
!= null && journey != null in LanguageScreen) only detects missing data and
leaves the UI spinning when the team query errors; update the guard to also
check for query.error or query.isError and handle it by returning an
error/fallback UI with retry (or call a provided refetch) instead of the
spinner. Locate the isDataReady usage and the render path in LanguageScreen (and
the similar guard around lines referenced 303-333) and add an early branch that
renders an error message and a retry action when query.error is present,
otherwise proceed with the existing loading/data rendering logic.
🪄 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: 92f201b9-2d0b-4bba-bb0b-6a66192d6686
📒 Files selected for processing (3)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/JourneyCustomizeTeamSelect/JourneyCustomizeTeamSelect.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx
...src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx
Show resolved
Hide resolved
…eanup - Define LanguageFormValues interface replacing FormikValues (Record<string, any>) - Add error state UI when team query fails (prevents infinite spinner) - Add null guard for journey.language.id (removes unsafe `as string` cast) - Collapse duplicate if/else branches in handleSubmit - Remove unnecessary Boolean() wrapper in shouldSkipDuplicate Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
LanguageFormValues.languageSelect.id was typed as string | undefined but LanguageAutocomplete expects string. Default id to '' in initialValues and align interface with LanguageOption (id: string, optional localName/nativeName). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx (1)
226-226: Minor: RedundantsetLoading(false)call.This
setLoading(false)is now redundant sincehandleSubmitwraps the call in atry/catch/finallythat setssetLoading(false)in thefinallyblock (line 297). Consider removing for clarity.Suggested fix
enqueueSnackbar( t('Unable to create guest user. Please try again or sign in.'), { variant: 'error' } ) - setLoading(false) return null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx` at line 226, Remove the redundant setLoading(false) call inside the LanguageScreen submission flow; locate the duplicate call in the same function that invokes handleSubmit and delete it because handleSubmit already ensures setLoading(false) in its finally block, so keep the one in handleSubmit (the finally block) and remove the earlier redundant setLoading(false) to avoid confusion.
🤖 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/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx`:
- Line 226: Remove the redundant setLoading(false) call inside the
LanguageScreen submission flow; locate the duplicate call in the same function
that invokes handleSubmit and delete it because handleSubmit already ensures
setLoading(false) in its finally block, so keep the one in handleSubmit (the
finally block) and remove the earlier redundant setLoading(false) to avoid
confusion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6de3634-ed93-472b-8156-a56068f5c1f4
📒 Files selected for processing (1)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx
Replace onClick={() => {}} with onClick={undefined} on disabled
buttons in the loading and error states to satisfy no-empty-function.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…plate-bugs-with-email-field-and-team-selection
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
Found 1 test failure on Blacksmith runners: Failure
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.spec.tsx (1)
1149-1195: Add a regression test for the team-query error screen.This file now covers duplication failures, but the new
hasTeamLoadErrorearly return inLanguageScreen.tsxstill isn’t exercised. AGET_LAST_ACTIVE_TEAM_ID_AND_TEAMSmock witherrorplus assertions on the failure copy and disabled Next button would lock down the spinner fix too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.spec.tsx` around lines 1149 - 1195, Add a regression test that triggers the new hasTeamLoadError early-return in LanguageScreen by providing a MockedProvider mock for GET_LAST_ACTIVE_TEAM_ID_AND_TEAMS that returns an error; render LanguageScreen (same setup as existing tests), wait for the error UI copy to appear (the team-query failure message) and assert the "CustomizeFlowNextButton" is disabled (or shows the spinner state) to ensure the team-load error path and spinner fix are exercised; reference the GET_LAST_ACTIVE_TEAM_ID_AND_TEAMS mock and the hasTeamLoadError behavior in LanguageScreen when adding the new test.
🤖 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/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx`:
- Around line 327-329: The error message shown in LanguageScreen is not
announced to assistive tech; update the element rendering the failure (the
Typography instance in LanguageScreen) to be accessible by either adding
role="alert" to that Typography or replacing it with MUI's Alert component
(importing Alert from `@mui/material`) so screen readers immediately announce the
load failure.
- Around line 210-218: The guest duplication branch in handleJourneyDuplication
delegates to createGuestUser(), which returns getJourneyProfile.lastActiveTeamId
without verifying that the id exists in query.data.teams; update the guest
branch to validate the returned teamId against the current teams list
(query.data.teams) and, if it's missing/stale, fall back to the same fallback
used for signed-in users (e.g., use values.teamSelect or the first available
team id or undefined) before proceeding with duplication; reference
handleJourneyDuplication, createGuestUser, getJourneyProfile.lastActiveTeamId,
and query.data.teams when making this change.
---
Nitpick comments:
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.spec.tsx`:
- Around line 1149-1195: Add a regression test that triggers the new
hasTeamLoadError early-return in LanguageScreen by providing a MockedProvider
mock for GET_LAST_ACTIVE_TEAM_ID_AND_TEAMS that returns an error; render
LanguageScreen (same setup as existing tests), wait for the error UI copy to
appear (the team-query failure message) and assert the "CustomizeFlowNextButton"
is disabled (or shows the spinner state) to ensure the team-load error path and
spinner fix are exercised; reference the GET_LAST_ACTIVE_TEAM_ID_AND_TEAMS mock
and the hasTeamLoadError behavior in LanguageScreen when adding the new test.
🪄 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: dcd67494-347c-40e2-aa2f-202718b0b7a8
📒 Files selected for processing (3)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsxlibs/locales/en/journeys-ui.json
✅ Files skipped from review due to trivial changes (1)
- libs/locales/en/journeys-ui.json
| async function handleJourneyDuplication( | ||
| type: 'signedIn' | 'guest', | ||
| journeyId: string | ||
| journeyId: string, | ||
| selectedTeamId?: string | ||
| ): Promise<string | null> { | ||
| let teamId | ||
| if (type === 'signedIn') { | ||
| const teams = query?.data?.teams ?? [] | ||
| teamId = query?.data?.getJourneyProfile?.lastActiveTeamId ?? teams[0]?.id | ||
| teamId = selectedTeamId | ||
| } else { |
There was a problem hiding this comment.
Guest duplication still bypasses the stale-team fallback.
The signed-in path now respects values.teamSelect, but the guest branch still delegates to createGuestUser(), and that helper returns getJourneyProfile.lastActiveTeamId without checking that the id still exists in query.data.teams. Anonymous users with a deleted/stale profile id can still duplicate into the wrong or nonexistent team.
🐛 Suggested fix
async function createGuestUser(): Promise<{ teamId: string }> {
const teamName = t('My Team')
const isAnonymous = user?.isAnonymous ?? false
if (!isAnonymous) {
try {
await signInAnonymously(getAuth(getApp()))
@@
await loadUser()
const existingTeams = query?.data?.teams ?? []
- if (existingTeams.length > 0) {
- const teamId =
- query?.data?.getJourneyProfile?.lastActiveTeamId ?? existingTeams[0].id
- return { teamId }
+ if (existingTeams.length > 0 && defaultTeamId !== '') {
+ return { teamId: defaultTeamId }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx`
around lines 210 - 218, The guest duplication branch in handleJourneyDuplication
delegates to createGuestUser(), which returns getJourneyProfile.lastActiveTeamId
without verifying that the id exists in query.data.teams; update the guest
branch to validate the returned teamId against the current teams list
(query.data.teams) and, if it's missing/stale, fall back to the same fallback
used for signed-in users (e.g., use values.teamSelect or the first available
team id or undefined) before proceeding with duplication; reference
handleJourneyDuplication, createGuestUser, getJourneyProfile.lastActiveTeamId,
and query.data.teams when making this change.
| <Typography color="error" align="center"> | ||
| {t('Failed to load teams. Please refresh the page and try again.')} | ||
| </Typography> |
There was a problem hiding this comment.
Announce the load failure to assistive tech.
This replaces the spinner asynchronously, but plain Typography won’t be announced when it appears. Add role="alert" here or switch to MUI Alert so screen-reader users get the failure immediately.
♿ Suggested tweak
- <Typography color="error" align="center">
+ <Typography color="error" align="center" role="alert">
{t('Failed to load teams. Please refresh the page and try again.')}
</Typography>As per coding guidelines, apps/**/*.{js,jsx,ts,tsx}: Implement accessibility features on elements.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Typography color="error" align="center"> | |
| {t('Failed to load teams. Please refresh the page and try again.')} | |
| </Typography> | |
| <Typography color="error" align="center" role="alert"> | |
| {t('Failed to load teams. Please refresh the page and try again.')} | |
| </Typography> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx`
around lines 327 - 329, The error message shown in LanguageScreen is not
announced to assistive tech; update the element rendering the failure (the
Typography instance in LanguageScreen) to be accessible by either adding
role="alert" to that Typography or replacing it with MUI's Alert component
(importing Alert from `@mui/material`) so screen readers immediately announce the
load failure.
| const { loadUser } = useCurrentUserLazyQuery() | ||
| const [teamCreate] = useTeamCreateMutation() | ||
| const [loading, setLoading] = useState(false) | ||
| const isDataReady = query?.data != null && journey != null |
There was a problem hiding this comment.
Nit: Purely checking if the data is not null has the downside of showing loading state even if something errors out. We can consider using query.loading for some better checks.
| const teams = query?.data?.teams ?? [] | ||
| const lastActiveTeamId = | ||
| query?.data?.getJourneyProfile?.lastActiveTeamId ?? '' | ||
| const defaultTeamId = teams.some((t) => t.id === lastActiveTeamId) |
There was a problem hiding this comment.
We need to QA to check that team select is still hidden for a guest
| <Typography color="error" align="center"> | ||
| {t('Failed to load teams. Please refresh the page and try again.')} | ||
| </Typography> |
There was a problem hiding this comment.
Only the stuff inside really needs to be added, all of the other wrappers shouldnt get redefined, so we dont accidentally make inconsistent changes. Do the same for the circular progress
| if (shouldSkipDuplicate(journey, values)) { | ||
| // Skips journey duplicate | ||
| handleNext() | ||
| } else if (isSignedIn) { | ||
| // Duplicates journey for a signed in user | ||
| const duplicatedJourneyId = await handleJourneyDuplication( | ||
| 'signedIn', | ||
| journeyId | ||
| ) | ||
| return data.journeyDuplicate.id | ||
| } | ||
|
|
||
| if (duplicatedJourneyId != null) { | ||
| handleNext(duplicatedJourneyId) | ||
| } else { | ||
| setLoading(false) |
There was a problem hiding this comment.
This will also need some regression testing for guests
4 participants
Summary
enableReinitializesilently reset the user's team selection whenever async data re-rendered the componenthandleJourneyDuplicationignored the form'steamSelectvalue and always usedlastActiveTeamIdfrom the serverlastActiveTeamIdagainst available teams (falls back to first team if stale/deleted)lastActiveTeamIdafter successful duplication (fire-and-forget)setActiveTeamside effect from team dropdownonChangeto prevent mid-flow re-rendersTesting
updateLastActiveTeamIdafter duplication, network error handling, user-selected team honored overlastActiveTeamIdPost-Deploy Monitoring & Validation
journeyDuplicatemutation errors in Datadog/loggingjourneyProfileUpdatemutation call rateLinear Issue
NES-1465
🤖 Generated with Claude Opus 4.6 (1M context) via Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests