fix(NES-1636): translate journey when copying to another team#9161
Open
jianwei1 wants to merge 7 commits into
Open
fix(NES-1636): translate journey when copying to another team#9161jianwei1 wants to merge 7 commits into
jianwei1 wants to merge 7 commits into
Conversation
CopyToTeamDialog forwarded language and showTranslation to its submitAction, but DuplicateJourneyMenuItem only consumed teamId, so the translation toggle never triggered the journeyAiTranslate subscription. Mirror TranslateJourneyDialog: after journeyDuplicate resolves, start the translation subscription with the user-selected target language and the duplicated journey id, and let the subscription's onComplete close the dialog and refetch template stats.
Contributor
WalkthroughDuplicateJourneyMenuItem was extended to support an optional AI-translation step when copying a journey to another team: it captures translation variables, starts an AI translation subscription after duplication, surfaces progress into CopyToTeamDialog, shows completion/error snackbars, and preserves duplicate behavior when translation is disabled or fails. ChangesAI Translation on Copy-to-Team
Sequence DiagramsequenceDiagram
actor User
participant UI as DuplicateJourneyMenuItem
participant Dialog as CopyToTeamDialog
participant API as GraphQL API
participant Sub as AI Translate Subscription
User->>UI: open menu, choose team, enable translation, pick language
UI->>Dialog: open CopyToTeamDialog (isTranslating:true, progress:0)
Dialog->>UI: confirm duplicate (teamId, language, showTranslation)
UI->>API: mutation duplicateJourney(teamId,...)
API-->>UI: duplicate success (newJourneyId)
UI->>Sub: start AI translation subscription(variables)
Sub-->>UI: progress events (percent, status)
UI->>Dialog: update translationProgress
alt translation completes
Sub-->>UI: complete (translatedJourney)
UI->>API: refetch template stats (if fromTemplateId)
UI->>User: show success snackbar
UI->>Dialog: close
else subscription error
Sub-->>UI: error
UI->>User: show error snackbar (keep duplicate)
UI->>Dialog: keep open / update state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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 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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit 8d1a063
☁️ 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx (1)
25-33: ⚡ Quick winReuse the generated subscription variables type here.
TranslationVariablesis a hand-rolled copy of the GraphQL variables contract foruseJourneyAiTranslateSubscription. If the schema/codegen changes, this state can drift silently from the hook it feeds. Prefer importing the generatedJourneyAiTranslateCreateSubscriptionVariablestype instead of redefining it locally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx` around lines 25 - 33, TranslationVariables duplicates the GraphQL variables shape for useJourneyAiTranslateSubscription; replace this hand-rolled interface with the generated type JourneyAiTranslateCreateSubscriptionVariables by importing it and using it wherever TranslationVariables is referenced (e.g., in DuplicateJourneyMenuItem and any state/props tied to useJourneyAiTranslateSubscription) so the component uses the codegen-backed contract and won't drift from the hook's variables.apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsx (1)
950-973: ⚡ Quick winUse the standard admin test harness for these new cases.
These added renders bypass the repo's required provider stack, so the translation-path tests are not exercising the component under the same theme/loading context as the rest of the app. Please wrap them with the shared
ThemeProviderandApolloLoadingProvider, ideally by moving all three setups through one helper so the harness stays consistent. As per coding guidelines: "apps/journeys-admin/**/*.spec.tsx: Tests must wrap components with required providers: SnackbarProvider → MockedProvider → ThemeProvider" and "Use ApolloLoadingProvider from test/ApolloLoadingProvider.tsx as a custom test provider."Also applies to: 1128-1147, 1214-1234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsx` around lines 950 - 973, The renderForTranslation helper renders DuplicateJourneyMenuItem with MockedProvider and SnackbarProvider but omits the repo-standard ThemeProvider and ApolloLoadingProvider; update renderForTranslation (and the other similar helpers at the other ranges) to wrap the component with the shared test harness: SnackbarProvider → MockedProvider → ThemeProvider and include ApolloLoadingProvider (imported from test/ApolloLoadingProvider.tsx) in the provider stack (or delegate to the existing shared helper) so the tests use the same ThemeProvider/ApolloLoadingProvider context as the rest of apps/journeys-admin tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx`:
- Around line 116-137: When showTranslation is true but prerequisites
(language?.id, duplicatedJourneyId, or journeyData?.language) are missing, do
not fall through to the duplicate-success path; instead surface an explicit
error and stop the success flow. In the DuplicateJourneyMenuItem component check
these exact symbols (showTranslation, language?.id, duplicatedJourneyId,
journeyData?.language) and when any is missing call the existing error/snackbar
handler (or set an error state that triggers the error snackbar), ensure loading
is cleared, avoid calling setTranslationVariables or returning into the success
branch, and let the subscription onError/onComplete still handle cleanup
consistently.
- Around line 206-215: The dialog close handler only hides the dialog via
setOpen(false) but must also cancel and clear translation state so any in-flight
translation subscription stops and loading/snackbar side-effects cannot fire;
update the onClose passed to CopyToTeamDialog to call setOpen(false) and also
reset translationVariables (e.g., setTranslationVariables(null)/undefined), set
loading to false (setLoading(false)), clear translationProgress
(setTranslationProgress(0) or null) and invoke any existing cancel/unsubscribe
function (e.g., cancelTranslation() or unsubscribeTranslation()) so the
translation flow is detached when the user cancels.
---
Nitpick comments:
In
`@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsx`:
- Around line 950-973: The renderForTranslation helper renders
DuplicateJourneyMenuItem with MockedProvider and SnackbarProvider but omits the
repo-standard ThemeProvider and ApolloLoadingProvider; update
renderForTranslation (and the other similar helpers at the other ranges) to wrap
the component with the shared test harness: SnackbarProvider → MockedProvider →
ThemeProvider and include ApolloLoadingProvider (imported from
test/ApolloLoadingProvider.tsx) in the provider stack (or delegate to the
existing shared helper) so the tests use the same
ThemeProvider/ApolloLoadingProvider context as the rest of apps/journeys-admin
tests.
In
`@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx`:
- Around line 25-33: TranslationVariables duplicates the GraphQL variables shape
for useJourneyAiTranslateSubscription; replace this hand-rolled interface with
the generated type JourneyAiTranslateCreateSubscriptionVariables by importing it
and using it wherever TranslationVariables is referenced (e.g., in
DuplicateJourneyMenuItem and any state/props tied to
useJourneyAiTranslateSubscription) so the component uses the codegen-backed
contract and won't drift from the hook's variables.
🪄 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: f154bdca-fe32-49af-a9c1-78e0b9c85380
📒 Files selected for processing (4)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsxapps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsxdocs/plans/2026-05-05-001-fix-nes-1636-ai-translate-on-copy-to-team-plan.mddocs/residual-review-findings/jianweichong-nes-1636-fix-ai-translate-on-copy-to-team.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/plans/2026-05-05-001-fix-nes-1636-ai-translate-on-copy-to-team-plan.md (2)
114-114: ⚡ Quick winClarify the source language name derivation logic.
The plan specifies using
journey?.language.name.find(({ primary }) => !primary)?.valueto derive the source language name, which filters for the non-primary name. This seems counterintuitive—typically the primary name is what should be displayed to users.While the plan notes this mirrors
TranslateJourneyDialog.tsx:159, it would be helpful to add a brief comment in the plan explaining why the non-primary name is preferred (e.g., if primary is a localized key and non-primary is the display name, or similar).📝 Suggested clarification
Add a brief explanation after line 114:
- Source-language-name derivation: pull from `journey?.language.name.find(({ primary }) => !primary)?.value ?? ''` (matches `TranslateJourneyDialog.tsx:159`). + Note: The non-primary name is used because [explain the reason - e.g., "primary contains the language code while non-primary contains the display name"].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-05-05-001-fix-nes-1636-ai-translate-on-copy-to-team-plan.md` at line 114, The plan's source-language-name derivation currently uses journey?.language.name.find(({ primary }) => !primary)?.value (matching TranslateJourneyDialog.tsx:159) which selects the non-primary name; update the plan text to add a short explanatory comment after that line clarifying why the non-primary name is preferred (e.g., primary is a localization key or internal identifier while the non-primary entry holds the user-facing display name), and mention that if journey is absent it falls back to useJourney() context to mirror TranslateJourneyDialog behavior.
125-134: ⚡ Quick winAdd test scenario for dialog cancellation during active translation.
The test scenarios comprehensively cover happy paths, validation, and error conditions, but they don't address what happens if the user cancels the
CopyToTeamDialogwhile the AI translation subscription is actively running.This scenario is important because:
- It relates to the deferred question about
handleCloseMenutiming (line 86).- Without proper cleanup, a running subscription could continue writing to cache or showing snackbars after the dialog closes.
- It validates that
translationVariablesis properly reset and the subscription unsubscribes on unmount.🧪 Suggested additional test scenario
Add to the test scenarios list:
- _Edge case — dialog cancelled mid-translation_ — Open Copy-to-Team dialog → select different team → toggle Translation on → select target language → submit. Mock subscription to emit progress but not complete. User clicks Cancel/closes dialog. Assert: subscription unsubscribes (via component unmount), no completion snackbar shows, duplicated journey persists (no rollback), `translationVariables` state is cleaned up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-05-05-001-fix-nes-1636-ai-translate-on-copy-to-team-plan.md` around lines 125 - 134, The tests are missing an edge case for cancelling CopyToTeamDialog while an AI translation subscription is active; add a test that opens CopyToTeamDialog, selects a different team, enables translation and a target language, mocks JOUNREY_DUPLICATE to succeed and JOUNREY_AI_TRANSLATE_CREATE_SUBSCRIPTION to emit progress but not complete, then simulate user closing the dialog (unmount) and assert the subscription was unsubscribed (no further emissions affect cache or snackbars), no completion snackbar appears, duplicated journey remains, and component state translationVariables is cleared; reference CopyToTeamDialog, handleCloseMenu, translationVariables, JOURNEY_DUPLICATE and JOURNEY_AI_TRANSLATE_CREATE_SUBSCRIPTION when locating code to mock and where to assert cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/plans/2026-05-05-001-fix-nes-1636-ai-translate-on-copy-to-team-plan.md`:
- Line 86: Clarify and implement a deterministic fix for the race: in the code
path where handleDuplicateJourney triggers a translation subscription, defer
calling handleCloseMenu() until the subscription's onComplete handler fires
(i.e., only call handleCloseMenu from the translation subscription onComplete)
so TranslationDialogWrapper remains mounted during translation; alternatively,
if you choose Option B after testing, add an explicit comment in
handleDuplicateJourney and next to TranslationDialogWrapper explaining that
early unmounting does not cancel server-side translation and document why it’s
safe—ensure references to handleDuplicateJourney, handleCloseMenu,
TranslationDialogWrapper, and the subscription onComplete are used so reviewers
can find and verify the change.
---
Nitpick comments:
In `@docs/plans/2026-05-05-001-fix-nes-1636-ai-translate-on-copy-to-team-plan.md`:
- Line 114: The plan's source-language-name derivation currently uses
journey?.language.name.find(({ primary }) => !primary)?.value (matching
TranslateJourneyDialog.tsx:159) which selects the non-primary name; update the
plan text to add a short explanatory comment after that line clarifying why the
non-primary name is preferred (e.g., primary is a localization key or internal
identifier while the non-primary entry holds the user-facing display name), and
mention that if journey is absent it falls back to useJourney() context to
mirror TranslateJourneyDialog behavior.
- Around line 125-134: The tests are missing an edge case for cancelling
CopyToTeamDialog while an AI translation subscription is active; add a test that
opens CopyToTeamDialog, selects a different team, enables translation and a
target language, mocks JOUNREY_DUPLICATE to succeed and
JOUNREY_AI_TRANSLATE_CREATE_SUBSCRIPTION to emit progress but not complete, then
simulate user closing the dialog (unmount) and assert the subscription was
unsubscribed (no further emissions affect cache or snackbars), no completion
snackbar appears, duplicated journey remains, and component state
translationVariables is cleared; reference CopyToTeamDialog, handleCloseMenu,
translationVariables, JOURNEY_DUPLICATE and
JOURNEY_AI_TRANSLATE_CREATE_SUBSCRIPTION when locating code to mock and where to
assert cleanup.
🪄 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: 6c803bac-c358-4f9d-b033-a28dde87e9bb
📒 Files selected for processing (2)
docs/plans/2026-05-05-001-fix-nes-1636-ai-translate-on-copy-to-team-plan.mddocs/residual-review-findings/jianweichong-nes-1636-fix-ai-translate-on-copy-to-team.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/residual-review-findings/jianweichong-nes-1636-fix-ai-translate-on-copy-to-team.md
jianwei1
added a commit
that referenced
this pull request
May 6, 2026
…og close Address two CodeRabbit review comments on PR #9161: 1. When showTranslation is true but a prerequisite is missing (language.id, duplicatedJourneyId, journeyData.language), surface "Failed to translate journey" instead of falling through to the duplicate-only success snackbar. Recreating the silent-skip failure mode this PR was meant to fix would defeat the point. 2. The dialog's local onClose now also clears translationVariables and loading so cancelling cancels the in-flight translation subscription on the client. The duplicated journey already exists server-side; the user finds it under the destination team next time they navigate. Adds a unit test covering the new defensive branch (journeyDuplicate returns null with showTranslation enabled).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsx (1)
1085-1088: 💤 Low value
refetchTemplateStatsassertion is trivially true here.This test renders
DuplicateJourneyMenuItemwithoutfromTemplateId, sorefetchTemplateStatscannot be called via theonCompletebranch regardless of the error path. To meaningfully assert "no stats refetch on subscription error", addfromTemplateId="templateId123"to the rendered component (or move this assertion into a variant test that does pass it). As written it doesn't protect against a regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsx` around lines 1085 - 1088, The test currently asserts that refetchTemplateStats is not called on subscription error but renders DuplicateJourneyMenuItem without a fromTemplateId so refetchTemplateStats could never be invoked; update the test render of DuplicateJourneyMenuItem to include a fromTemplateId (e.g. "templateId123") so the onComplete branch that may call refetchTemplateStats is reachable (or alternatively move the refetchTemplateStats assertion into a separate variant test that renders with fromTemplateId), keeping assertions against handleCloseMenu and refetchTemplateStats unchanged.apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx (1)
19-33: ⚡ Quick winImport
JourneyLanguagefromCopyToTeamDialogto avoid duplicate type definitions.Both
CopyToTeamDialogandDuplicateJourneyMenuItemdefine identicalJourneyLanguageinterfaces with the same fields and optionality (id: string,localName?: string,nativeName?: string). The fallback logic at Line 143 (language.nativeName ?? language.localName ?? '') works correctly with both. However, importing the type from@core/journeys/ui/CopyToTeamDialogeliminates duplication and prevents future divergence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx` around lines 19 - 33, Duplicate JourneyLanguage definition found in DuplicateJourneyMenuItem.tsx; import the type from `@core/journeys/ui/CopyToTeamDialog` instead of redefining it. Remove the local JourneyLanguage interface in DuplicateJourneyMenuItem and add an import for JourneyLanguage from CopyToTeamDialog, ensuring existing code that uses JourneyLanguage (including the fallback logic using language.nativeName ?? language.localName ?? '') continues to type-check and compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx`:
- Around line 135-146: The code computes sourceLanguageName using
journeyData.language.name.find(({ primary }) => !primary)?.value ?? '' which
returns empty when only a primary entry exists; change the logic in
DuplicateJourneyMenuItem to first try a non-primary name then fall back to the
primary name's value (e.g., check for a primary entry and use its .value if no
non-primary found) before calling setTranslationVariables so journeyLanguageName
and userLanguageName are populated; update the tests that assert
journeyLanguageName: '' (lines referenced in the review) to expect the primary
language value instead.
---
Nitpick comments:
In
`@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsx`:
- Around line 1085-1088: The test currently asserts that refetchTemplateStats is
not called on subscription error but renders DuplicateJourneyMenuItem without a
fromTemplateId so refetchTemplateStats could never be invoked; update the test
render of DuplicateJourneyMenuItem to include a fromTemplateId (e.g.
"templateId123") so the onComplete branch that may call refetchTemplateStats is
reachable (or alternatively move the refetchTemplateStats assertion into a
separate variant test that renders with fromTemplateId), keeping assertions
against handleCloseMenu and refetchTemplateStats unchanged.
In
`@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx`:
- Around line 19-33: Duplicate JourneyLanguage definition found in
DuplicateJourneyMenuItem.tsx; import the type from
`@core/journeys/ui/CopyToTeamDialog` instead of redefining it. Remove the local
JourneyLanguage interface in DuplicateJourneyMenuItem and add an import for
JourneyLanguage from CopyToTeamDialog, ensuring existing code that uses
JourneyLanguage (including the fallback logic using language.nativeName ??
language.localName ?? '') continues to type-check and compile.
🪄 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: a82ea935-1ab9-463a-86ff-e26d748aca86
📒 Files selected for processing (2)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsxapps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx
Comment on lines
+135
to
146
| const sourceLanguageName = | ||
| journeyData.language.name.find(({ primary }) => !primary)?.value ?? '' | ||
|
|
||
| setTranslationVariables({ | ||
| journeyId: duplicatedJourneyId, | ||
| name: journeyData.title ?? '', | ||
| journeyLanguageName: sourceLanguageName, | ||
| textLanguageId: language.id, | ||
| textLanguageName: language.nativeName ?? language.localName ?? '', | ||
| userLanguageId: journeyData.language.id, | ||
| userLanguageName: sourceLanguageName | ||
| }) |
Contributor
There was a problem hiding this comment.
sourceLanguageName silently becomes empty for source languages that only expose a primary name entry.
find(({ primary }) => !primary)?.value ?? '' only returns the localized (non-primary) name. For a journey whose language.name only has the primary entry (e.g., English with [{ value: 'English', primary: true }]), this becomes '' and is then sent as both journeyLanguageName and userLanguageName to the AI translation subscription — losing useful prompt context for the translator. Falling back to the primary value preserves the language name in this case.
Note the existing tests at Lines 982-988 / 1108-1114 explicitly assert journeyLanguageName: '', so they will need to be updated alongside this change.
🛡️ Suggested fallback
- const sourceLanguageName =
- journeyData.language.name.find(({ primary }) => !primary)?.value ?? ''
+ const sourceLanguageName =
+ journeyData.language.name.find(({ primary }) => !primary)?.value ??
+ journeyData.language.name.find(({ primary }) => primary)?.value ??
+ ''📝 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.
Suggested change
| const sourceLanguageName = | |
| journeyData.language.name.find(({ primary }) => !primary)?.value ?? '' | |
| setTranslationVariables({ | |
| journeyId: duplicatedJourneyId, | |
| name: journeyData.title ?? '', | |
| journeyLanguageName: sourceLanguageName, | |
| textLanguageId: language.id, | |
| textLanguageName: language.nativeName ?? language.localName ?? '', | |
| userLanguageId: journeyData.language.id, | |
| userLanguageName: sourceLanguageName | |
| }) | |
| const sourceLanguageName = | |
| journeyData.language.name.find(({ primary }) => !primary)?.value ?? | |
| journeyData.language.name.find(({ primary }) => primary)?.value ?? | |
| '' | |
| setTranslationVariables({ | |
| journeyId: duplicatedJourneyId, | |
| name: journeyData.title ?? '', | |
| journeyLanguageName: sourceLanguageName, | |
| textLanguageId: language.id, | |
| textLanguageName: language.nativeName ?? language.localName ?? '', | |
| userLanguageId: journeyData.language.id, | |
| userLanguageName: sourceLanguageName | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx`
around lines 135 - 146, The code computes sourceLanguageName using
journeyData.language.name.find(({ primary }) => !primary)?.value ?? '' which
returns empty when only a primary entry exists; change the logic in
DuplicateJourneyMenuItem to first try a non-primary name then fall back to the
primary name's value (e.g., check for a primary entry and use its .value if no
non-primary found) before calling setTranslationVariables so journeyLanguageName
and userLanguageName are populated; update the tests that assert
journeyLanguageName: '' (lines referenced in the review) to expect the primary
language value instead.
Contributor
Author
There was a problem hiding this comment.
Skipping for scope. The sourceLanguageName derivation is a deliberate mirror of TranslateJourneyDialog.tsx:158-159:
const currentLanguageName =
journeyData.language.name.find(({ primary }) => !primary)?.value ?? ''That pattern has been in production since the AI translation feature shipped — same fall-through-to-empty for journeys whose language.name only carries a primary entry. NES-1636 was deliberately framed as a near-mechanical port of TranslateJourneyDialog's wiring (per the plan's "Mirror TranslateJourneyDialog's subscription wiring rather than introducing a shared hook" decision), so fixing this here only would diverge from the reference pattern: same input would now produce different translation prompts depending on which entry point fired.
The right shape for this fix is a follow-up that updates both callsites and their tests in lockstep — happy to file it as a separate ticket if translation quality from missing source-language context is showing up in QA. For this PR I'd rather hold the symmetry.
Contributor
There was a problem hiding this comment.
@jianwei1, that's a well-reasoned argument — the symmetry concern is valid. Having both entry points (DuplicateJourneyMenuItem and TranslateJourneyDialog) derive sourceLanguageName identically ensures the same journey produces the same translation prompt regardless of which path is taken. Fixing one in isolation would silently introduce a behavioral split that could be harder to debug later.
If you'd like, I can open a follow-up GitHub issue to track updating both callsites and their tests in lockstep. Just let me know.
(´･ω･`) noted for future reviews
✏️ Learnings added
Learnt from: jianwei1
Repo: JesusFilm/core PR: 9161
File: apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx:135-146
Timestamp: 2026-05-06T22:25:57.396Z
Learning: In `apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx`, the `sourceLanguageName` derivation `journeyData.language.name.find(({ primary }) => !primary)?.value ?? ''` is a deliberate mirror of the same pattern in `TranslateJourneyDialog.tsx` (around line 158-159: `journeyData.language.name.find(({ primary }) => !primary)?.value ?? ''`). Both callsites intentionally fall through to an empty string for journeys whose `language.name` only carries a primary entry, to maintain symmetry between entry points. Any fix must update both callsites and their tests in lockstep to avoid behavioral divergence. Do not flag this as a bug in either file independently.
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: jianwei1
Repo: JesusFilm/core PR: 8998
File: apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/ChatOption/Summary/Summary.tsx:68-130
Timestamp: 2026-04-09T02:33:33.597Z
Learning: In this repo’s editor command implementations that use `useCommand().add()` (e.g., command hooks/components like `useBlockCreateCommand`, `useBlockDeleteCommand`, `MoveBlock`, property controls, etc.), follow the established pattern: call mutation/fetch functions as fire-and-forget using `void` inside both `execute()` and `undo()` callbacks, and register the command synchronously via `CommandProvider.add()` (there’s no async registration path). If an undo/execute race could occur due to a closure variable not being set yet (e.g., `createdButtonId`), guard with an early null/undefined check (e.g., `if (createdButtonId == null) return`). Do not flag the `void` fire-and-forget usage or synchronous command registration as bugs in future reviews when this pattern (including the null-check guard) is present.
…ssing Address CodeRabbit feedback on PR #9161 (apps/.../DuplicateJourneyMenuItem.tsx line 137): when showTranslation is true but a prerequisite is missing (language.id, duplicatedJourneyId, journeyData.language), surface "Failed to translate journey" instead of falling through to the duplicate-only success snackbar. Recreating the silent-skip failure mode this PR was meant to fix would defeat the point. Yup blocks the common "no language selected" case before submit; this branch is a defensive backstop for partial GraphQL responses (e.g. gateway returning { data: { journeyDuplicate: null } } despite codegen marking the field non-null) and for callers without a journey context. Adds a unit test covering the new defensive branch. Skipped the second CodeRabbit comment (line 215, "Closing the dialog does not cancel the in-flight translation state"). TranslationDialogWrapper hides the Cancel and Create buttons while loading is true, and CopyToTeamDialog blocks backdrop click and Escape during translation, so onClose is not user-reachable while translationVariables is set.
jianwei1
force-pushed
the
jianweichong/nes-1636-fix-ai-translate-on-copy-to-team
branch
from
0340c29 to
8781f61
Compare
Contributor
|
The latest updates on your projects.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx`:
- Around line 102-133: Pre-validate translation prerequisites before calling
journeyDuplicate in DuplicateJourneyMenuItem: check showTranslation and ensure
language?.id and journeyData?.language are present (and return early with
enqueueSnackbar(t('Failed to translate'), { variant: 'error', preventDuplicate:
true }) and setLoading(false)) before invoking journeyDuplicate; only call
journeyDuplicate when prerequisites pass (use targetTeamId resolution as before)
so you don't create an untranslated duplicate then surface a translation error.
🪄 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: 1137e2ac-e0e0-4c12-995a-877f69d424fd
📒 Files selected for processing (2)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsxapps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsx
Comment on lines
102
to
+133
| setLoading(true) | ||
| try { | ||
| if (teamId != null) { | ||
| await journeyDuplicate({ | ||
| variables: { | ||
| id, | ||
| teamId | ||
| } | ||
| }) | ||
| } else if (activeTeam?.id != null) { | ||
| await journeyDuplicate({ | ||
| variables: { id, teamId: activeTeam.id } | ||
| const targetTeamId = teamId ?? activeTeam?.id | ||
| if (targetTeamId == null) { | ||
| setLoading(false) | ||
| return | ||
| } | ||
|
|
||
| const { data: duplicateData } = await journeyDuplicate({ | ||
| variables: { id, teamId: targetTeamId } | ||
| }) | ||
|
|
||
| const duplicatedJourneyId = duplicateData?.journeyDuplicate?.id | ||
|
|
||
| if (showTranslation === true) { | ||
| if ( | ||
| language?.id == null || | ||
| duplicatedJourneyId == null || | ||
| journeyData?.language == null | ||
| ) { | ||
| // Translation was explicitly requested but a prerequisite is missing | ||
| // (Yup normally blocks this — defensive against partial GraphQL | ||
| // responses or callers without a journey context). Surface an | ||
| // explicit error rather than falling through to the duplicate-only | ||
| // success snackbar. | ||
| enqueueSnackbar(t('Failed to translate journey'), { | ||
| variant: 'error', | ||
| preventDuplicate: true | ||
| }) | ||
| setLoading(false) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Pre-validate translation prerequisites before duplicating.
At Line 110, duplication runs before validating language?.id and journeyData?.language. In the missing-prereq path, this can still create a duplicate and then report translation failure, leaving an unintended untranslated copy.
Suggested minimal change
setLoading(true)
try {
+ if (showTranslation === true) {
+ if (language?.id == null || journeyData?.language == null) {
+ enqueueSnackbar(t('Failed to translate journey'), {
+ variant: 'error',
+ preventDuplicate: true
+ })
+ setLoading(false)
+ return
+ }
+ }
+
const targetTeamId = teamId ?? activeTeam?.id
if (targetTeamId == null) {
setLoading(false)
return
}
@@
- if (showTranslation === true) {
- if (
- language?.id == null ||
- duplicatedJourneyId == null ||
- journeyData?.language == null
- ) {
+ if (showTranslation === true) {
+ if (duplicatedJourneyId == null) {
enqueueSnackbar(t('Failed to translate journey'), {
variant: 'error',
preventDuplicate: true
})
setLoading(false)
return
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.tsx`
around lines 102 - 133, Pre-validate translation prerequisites before calling
journeyDuplicate in DuplicateJourneyMenuItem: check showTranslation and ensure
language?.id and journeyData?.language are present (and return early with
enqueueSnackbar(t('Failed to translate'), { variant: 'error', preventDuplicate:
true }) and setLoading(false)) before invoking journeyDuplicate; only call
journeyDuplicate when prerequisites pass (use targetTeamId resolution as before)
so you don't create an untranslated duplicate then surface a translation error.
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
When a user copied a journey to another team and enabled the AI translation toggle, the journey was duplicated but never translated — it landed in the destination team in the source language, with no error or feedback. The dialog (
CopyToTeamDialog) had been forwardinglanguageandshowTranslationto itssubmitActionsince PR #6761, but the consumer inDuplicateJourneyMenuItemonly consumedteamIdand silently dropped the rest. TypeScript could not catch the structural mismatch.After this fix,
DuplicateJourneyMenuItemmirrors the working pattern fromTranslateJourneyDialog: afterjourneyDuplicateresolves,useJourneyAiTranslateSubscriptionis started against the duplicated journey id and the user-selected target language. The subscription'sonCompletecloses the dialog, refetches template stats when applicable, and surfaces the success snackbar. The same-team direct-duplicate path (no dialog) and the translation-off path are unchanged.Linear: NES-1636 — bug in Production
Plan:
docs/plans/2026-05-05-001-fix-nes-1636-ai-translate-on-copy-to-team-plan.mdWhat changed
DuplicateJourneyMenuItem.handleDuplicateJourneynow accepts(teamId?, language?, showTranslation?)and conditionally fires the AI translate subscription afterjourneyDuplicateresolves.useJourneyAiTranslateSubscriptionis wired withonComplete(refetch template stats, success snackbar, close dialog, callhandleCloseMenu) andonError(surface error snackbar with at('Failed to translate journey')fallback when the upstream message is empty).isTranslatingandtranslationProgressare passed back toCopyToTeamDialogso its existing in-dialog progress UI renders during translation.journey?.language.nameon the prop, withuseJourney()as fallback. The existingGetAdminJourneysquery already returnslanguage.nameandtitle, so no GraphQL change.Test coverage
4 new tests added under a new
AI translation on copy to teamdescribe block, plus 1 strengthened error-path assertion. All 10 tests pass.onComplete, success snackbar showshandleCloseMenuandrefetchTemplateStatsare NOT called; the duplicate is not rolled backfromTemplateIdrefetch on translate completerefetchTemplateStatsis called with the template id after the subscription completesjourneyDuplicateerror pathhandleCloseMenuis NOT calledThe 5 existing tests (same-team direct-duplicate, dialog-shared-with-me path with and without journey prop, dialog cancel,
refetchTemplateStatson direct duplicate) continue to pass unchanged, locking in the no-regression contract.Residual review findings
ce-code-reviewautofix applied 3 safe local fixes (empty-error-message fallback, error-path assertion strengthening, duplicate-error-path test) and surfaced 16 residual findings recorded indocs/residual-review-findings/jianweichong-nes-1636-fix-ai-translate-on-copy-to-team.mdfor reviewer attention. Highlights:GetAdminJourneysrefetch fromupdateLastActiveTeamId.onCompletedcan unmount the menu item before the subscription'sonCompletefires, leaving an orphan untranslated journey with no UI feedback. The same hazard exists inTranslateJourneyDialog— shared subscription-lifecycle debt.onCloseonly setsopen=falsebut leavestranslationVariablesset, so the subscription keeps running andonCompletefires snackbar/handleCloseMenuon a dismissed dialog.journeyDuplicate: nullpartial response handling,TranslationVariablestype duplication, and a state-machine refactor that would consolidatesetLoading(false)calls scattered across 5 paths.None of these block the user-visible bug fix; they document follow-up work the reviewer can weigh against scope.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation