fix: regenerate interface files for local template changes #8468
Conversation
WalkthroughIntroduces distinction between local and global templates via teamId filtering and permission checks. Local templates belong to user teams, while global templates belong to the 'jfp-team'. Updates query variables, permission logic, UI rendering, and refactors the journey list component structure with new JourneyListView and JourneyListContent components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring special attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 a686243
☁️ Nx Cloud last updated this comment at |
|
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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/journeys/ui/src/components/TemplateView/UseThisTemplateButton/UseThisTemplateButton.tsx (1)
37-45: Enforce sign-in check logic for menu-item variant.The condition on line 38 assumes
variant='menu-item'always indicates a signed-in user, but this assumption is not enforced. The logicif (variant === 'menu-item' || signedIn)bypasses the account dialog based on variant alone, which could cause issues if menu-item is used in other contexts in the future.Either make the
signedInprop required whenvariant='menu-item'(via TypeScript constraints or explicit validation), or require callers to passsignedInexplicitly rather than relying on the variant to determine sign-in status.
♻️ Duplicate comments (3)
apps/journeys-admin/src/components/TemplateList/TrashedTemplateList/TrashedTemplateList.tsx (1)
42-43: Same hardcoded team ID issue.The
'jfp-team'string literal is repeated here. See the previous comment about extracting this into a shared constant.apps/journeys-admin/pages/templates/[journeyId].tsx (1)
159-160: Same hardcoded team ID in SSR context.The
'jfp-team'string is hardcoded here in the server-side props generation. Extracting this to a shared constant would be beneficial, especially since this needs to stay in sync with client-side queries.apps/journeys-admin/src/components/TemplateList/ArchivedTemplateList/ArchivedTemplateList.tsx (1)
42-43: Same hardcoded team ID issue.The
'jfp-team'string literal is repeated here. See earlier comments about extracting this into a shared constant.
🧹 Nitpick comments (24)
apps/journeys-admin/pages/index.tsx (2)
37-41: Normalizerouter.query.typeinstead of asserting, and extract a type aliasThe logic is correct, but the type assertion masks the actual Next.js router type (
string | string[] | undefined):const currentContentType = (router?.query?.type as 'journeys' | 'templates') ?? 'journeys'Two suggestions:
- Introduce an explicit type alias (per TS guidelines) and reuse it:
type ContentType = 'journeys' | 'templates'
- Normalize the query param instead of asserting, so unexpected values or arrays fall back safely:
type ContentType = 'journeys' | 'templates' const typeParam = router.query.type const currentContentType: ContentType = typeParam === 'templates' ? 'templates' : 'journeys' const showSidePanel = currentContentType === 'journeys'This keeps runtime behavior the same for valid values, handles
string[]/invalid cases, and removes the unchecked cast. As per coding guidelines, definingContentTypetightens typing here and for any related components.
60-75: Conditional side panel rendering looks good; consider minor React/TS idiomsThe conditional rendering of the side panel and title is clear and matches the intent to only show onboarding on the journeys tab:
sidePanelChildren={showSidePanel ? <OnboardingPanel /> : undefined} sidePanelTitle={ showSidePanel ? ( <> <Typography variant="subtitle1">{t('Create a New Journey')}</Typography> <HelpScoutBeacon userInfo={{ name: user?.displayName ?? '', email: user?.email ?? '' }} /> </> ) : undefined }Two optional refinements:
- Use
nullinstead ofundefinedfor JSX conditionals, which is a bit more idiomatic forReactNodeprops:sidePanelChildren={showSidePanel ? <OnboardingPanel /> : null} sidePanelTitle={ showSidePanel ? ( <> {/* … */} </> ) : null }
- Optionally hoist
userInfointo a const to avoid recreating the object inline every render and make the prop clearer, e.g.:const userInfo = { name: user?.displayName ?? '', email: user?.email ?? '' } // … <HelpScoutBeacon userInfo={userInfo} />These are style/readability tweaks; behavior is already correct.
libs/journeys/ui/src/components/TemplateSections/TemplateSections.tsx (1)
32-45: Team scoping looks correct; please confirm this component is only used for global templatesAdding
teamId: 'jfp-team'to thewherefilter aligns this component with the new team-scoped template behavior and matches the updated mocks/type policies elsewhere, so the query shape looks consistent.Two small follow‑ups to consider:
- If
TemplateSectionsis ever used outside the “global templates” context, this will now silently filter out non‑jfp-teamtemplates. It’s worth double‑checking call sites to ensure that’s intended for all usages, or that local‑template flows use a different component/path.- Since
'jfp-team'is now used across multiple queries in the PR, consider centralizing it in a shared constant (e.g.,GLOBAL_TEMPLATES_TEAM_ID) to avoid future drift if the value changes. Optional but improves maintainability.If you want, I can provide a small
rg/ast-grepscript to enumerate allTemplateSectionsusages and verify they’re only in global-template contexts.libs/journeys/ui/src/components/TemplateSections/TemplateSections.spec.tsx (1)
168-249: Mocks correctly updated to match new team‑scoped query variablesThe three mocks now include
teamId: 'jfp-team'inwhere, which keeps them aligned with the updatedTemplateSectionsquery and ensures tests will fail if the component ever drops or changes that argument. This is a good way to implicitly enforce the new contract.As a minor follow‑up, you might consider sharing a common
GLOBAL_TEMPLATES_TEAM_IDconstant between the component and these specs to avoid hard‑coding the same literal in multiple places, but that’s optional.apps/journeys-admin/src/components/JourneyList/JourneyListMenu/JourneyListMenu.tsx (1)
43-51: Add aria-label for accessibility.Per coding guidelines, interactive elements should include accessibility attributes. The
IconButtonis missing anaria-labelto describe its purpose for screen readers.<IconButton edge="end" color="inherit" sx={{ mx: 3 }} onClick={handleShowMenu} data-testid="JourneyListMenuButton" + aria-label={t('Journey list menu')} > <MoreIcon data-testid="MoreIcon" /> </IconButton>apps/journeys-admin/src/components/JourneyList/JourneyListMenu/JourneyListMenu.spec.tsx (1)
112-120: Misleading test names in 'trashed' describe block.The test names don't match the functionality being tested:
- Line 112:
'should call archiveAllActive'testsrestoreAllTrashed- Line 122:
'should call trashAllArchived'testsdeleteAllTrashedConsider renaming for clarity:
- it('should call archiveAllActive', () => { + it('should call restoreAllTrashed', () => {- it('should call trashAllArchived', () => { + it('should call deleteAllTrashed', () => {Also applies to: 122-130
apps/journeys-admin/src/libs/useJourneyCreateMutation/useJourneyCreateMutation.spec.tsx (1)
145-160: Cache assertion handles dynamic keyArgs correctly, but fallback is weak.The prefix-based search for
adminJourneysentries correctly accommodates the dynamic cache keys generated bykeyArgs. However, the fallback assertion (lines 156-158) only verifies the journey exists in cache, not that it was properly added to anadminJourneysentry.If the fallback branch executes, it may mask a cache update failure. Consider whether this should fail explicitly:
} else { - // Fallback: check if journey exists in cache (cache.modify updates all entries) - expect(cache.extract()?.['Journey:createdJourneyId']).toBeDefined() + // If no adminJourneys entry exists, the cache update didn't work as expected + fail('Expected adminJourneys cache entry to exist') }Alternatively, if the fallback is intentional for certain test scenarios, add a comment explaining when it's expected to trigger.
apis/api-journeys/src/app/modules/journey/journey.resolver.ts (2)
1038-1046: Consider reordering the null check before accessing journey properties.The
isGlobalTemplatecheck on line 1038 accessesjourney?.team?.idbefore the null check forjourneyon line 1039. While optional chaining prevents a runtime error, it's cleaner to check for existence first:+ if (journey == null) + throw new GraphQLError('journey not found', { + extensions: { code: 'NOT_FOUND' } + }) const isGlobalTemplate = journey?.team?.id === 'jfp-team' - if (journey == null) - throw new GraphQLError('journey not found', { - extensions: { code: 'NOT_FOUND' } - }) if ( isGlobalTemplate && ability.cannot(Action.Manage, subject('Journey', journey), 'template') )
584-584: Consider extracting'jfp-team'to a constant.The magic string
'jfp-team'is used here and injourneyTemplate(line 1038). Extracting it to a shared constant would improve maintainability and reduce the risk of typos.+const GLOBAL_TEMPLATE_TEAM_ID = 'jfp-team' + // In journeyDuplicate: - const isLocalTemplate = journey.teamId !== 'jfp-team' && journey.template + const isLocalTemplate = journey.teamId !== GLOBAL_TEMPLATE_TEAM_ID && journey.template // In journeyTemplate: - const isGlobalTemplate = journey?.team?.id === 'jfp-team' + const isGlobalTemplate = journey?.team?.id === GLOBAL_TEMPLATE_TEAM_IDapps/journeys-admin/src/components/Editor/Toolbar/Items/CreateTemplateItem/CreateTemplateItem.tsx (1)
57-57: Consider extracting the hardcoded team ID.The
'jfp-team'string is hardcoded here and in the resolver. Consider importing from a shared constants file to ensure consistency:// In a shared constants file: export const GLOBAL_TEMPLATE_TEAM_ID = 'jfp-team'apps/journeys-admin/src/components/TemplateList/ActiveTemplateList/ActiveTemplateList.tsx (1)
42-43: Same hardcoded team ID concern.Consider using a shared constant for
'jfp-team'to maintain consistency with the resolver and CreateTemplateItem component.libs/journeys/ui/src/components/TemplateView/TemplateView.tsx (1)
46-47: Consider extracting the hardcoded team ID into a named constant.The string literal
'jfp-team'is repeated across multiple files in this PR. Extracting it into a shared constant (e.g.,GLOBAL_TEMPLATES_TEAM_ID) would improve maintainability and make the intent clearer.Example:
// In a shared constants file export const GLOBAL_TEMPLATES_TEAM_ID = 'jfp-team' // In this file teamId: GLOBAL_TEMPLATES_TEAM_IDapps/journeys-admin/pages/templates/index.tsx (1)
143-144: Consider extracting magic constants.Both
'529'(English language ID) and'jfp-team'are hardcoded string literals. Consider extracting these to named constants for better maintainability and clarity.Example:
const ENGLISH_LANGUAGE_ID = '529' const GLOBAL_TEMPLATES_TEAM_ID = 'jfp-team' // In the query languageIds: [ENGLISH_LANGUAGE_ID], teamId: GLOBAL_TEMPLATES_TEAM_IDapps/journeys-admin/src/libs/useAdminJourneysQuery/useAdminJourneysQuery.spec.tsx (1)
110-126: Avoidas anycasting in test assertions.The test assertions use
(journey as any)to access the new fields, which bypasses TypeScript's type checking. This can hide type mismatches and make refactoring more difficult.Consider one of these approaches:
Option 1: Update the GraphQL type definitions
expect(hookResult.current.data?.journeys).toBeDefined() expect(hookResult.current.data?.journeys?.length).toBeGreaterThan(0) hookResult.current.data?.journeys?.forEach((journey) => { expect(journey.team?.id).toBe('team-id') expect(journey.journeyCustomizationDescription).toBe( 'Customize this journey' ) expect(journey.journeyCustomizationFields).toEqual([ { id: 'journey-customization-field-id', journeyId: 'journey.id', key: 'key', value: 'value', defaultValue: 'defaultValue' } ]) })Option 2: If the types are intentionally not in the query, add proper type guards or define an explicit test type
type JourneyWithCustomization = GetAdminJourneys['journeys'][0] & { team: { id: string } journeyCustomizationDescription: string journeyCustomizationFields: Array<{...}> } const journey = hookResult.current.data?.journeys?.[0] as JourneyWithCustomizationThis makes the type extension explicit and documents the additional fields being tested.
apps/journeys-admin/pages/publisher/[journeyId].tsx (1)
48-86: Template gating logic looks correct; consider load-state handling as an optional polishThe
isGlobalTemplatecalculation and the new(isPublisher || !isGlobalTemplate)/(isGlobalTemplate && !isPublisher)conditions match the local vs global template behavior you’re introducing and look logically sound.One minor side effect: before the queries resolve, both
isPublisherandisGlobalTemplateareundefined, so the editor branch evaluates truthy (!isGlobalTemplate) and briefly renders until data arrives (at which point global, non‑publisher users will flip to the access‑denied view). If that flicker is undesirable, you could gate rendering on the query loading states or ondata?.publisherTemplate/publisherDatabeing defined.Otherwise, this change aligns with the new global/local template semantics.
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DefaultMenu/DefaultMenu.tsx (1)
8-9: Menu integrations for template actions look good; watch for double Divider when no active teamThe new wiring for template actions makes sense:
- Non‑template journeys: Duplicate/Translate, then a dedicated section for “Make Template” plus “Make Global Template” when
isPublisher === trueviaCreateTemplateItemwithglobalPublishset appropriately.- Template journeys:
TemplateActionButtonrendered as amenu-item, consistent with the TemplateView behavior.One minor UI nit: when
template !== trueandactiveTeam == null, theDividerat line 217 still renders, theDuplicate/Translateblock is skipped, and the non‑template block at line 231 adds anotherDivider, so you end up with two separators back‑to‑back. If there’s a realistic path whereactiveTeamis unset, you may want to conditionally render one of thoseDividers or fold them into the same block.If that edge case doesn’t occur in practice, the current implementation is fine.
Also applies to: 34-35, 218-241
libs/journeys/ui/src/components/TemplateView/TemplateViewHeader/TemplateActionButton/TemplateActionButton.spec.tsx (1)
2-5: Expanded TemplateActionButton tests are solid and map well to behaviorThe router mocking, SnackbarProvider/JourneyProvider wrapping, and the new scenarios (menu-item vs button, signed in/out, customizable vs non-customizable, navigation and dialog flows) give this component excellent coverage and closely match the implementation semantics.
If you find yourself adding more cases later, you might consider a small
renderWithProvidershelper to DRY up the repeated render tree, but that’s purely optional.Also applies to: 16-22, 30-38, 44-143, 145-272
apps/journeys-admin/src/components/JourneyList/JourneyList.tsx (1)
66-73: Consider simplifying nested ternary for readability.The nested ternary expression for
activeTabis hard to follow. Consider using a helper function or a more readable approach.- // Determine active tab from router query (support both old 'tab' and new 'status' params) - const activeTab = - (router?.query?.status as JourneyStatusFilter) ?? - (router?.query?.tab?.toString() === 'archived' - ? 'archived' - : router?.query?.tab?.toString() === 'trashed' - ? 'trashed' - : 'active') + // Determine active tab from router query (support both old 'tab' and new 'status' params) + const getActiveTab = (): JourneyStatusFilter => { + if (router?.query?.status) { + return router.query.status as JourneyStatusFilter + } + const tab = router?.query?.tab?.toString() + if (tab === 'archived') return 'archived' + if (tab === 'trashed') return 'trashed' + return 'active' + } + const activeTab = getActiveTab()apps/journeys-admin/src/components/JourneyList/JourneyListView/JourneyListView.tsx (3)
25-26: Consider renaming localJourneyStatustype to avoid shadowing.The local type
JourneyStatuscould be confused with the GraphQLJourneyStatusenum fromglobalTypes. The index barrel already re-exports it asJourneyStatusFilter, so consider using that name here as well for consistency.export type ContentType = 'journeys' | 'templates' -export type JourneyStatus = 'active' | 'archived' | 'trashed' +export type JourneyStatusFilter = 'active' | 'archived' | 'trashed'Then update all internal usages of
JourneyStatustoJourneyStatusFilterwithin this file.
114-139: MovecontentTypeOptionsoutside component or memoize to prevent unnecessary effect runs.
contentTypeOptionsis recreated on every render and is included in the dependency array, causing theuseEffectto run more frequently than necessary.Move the options outside the component (they only depend on
tfor translations, but the values are static):+const CONTENT_TYPE_OPTIONS: Omit<ContentTypeOption, 'displayValue'>[] = [ + { queryParam: 'journeys', tabIndex: 0 }, + { queryParam: 'templates', tabIndex: 1 } +] + export function JourneyListView({ ... }: JourneyListViewProps): ReactElement { const { t } = useTranslation('apps-journeys-admin') ... - // Content type options (Journeys, Templates) - const contentTypeOptions: ContentTypeOption[] = [ - { - queryParam: 'journeys', - displayValue: t('Team Projects'), - tabIndex: 0 - }, - { - queryParam: 'templates', - displayValue: t('Team Templates'), - tabIndex: 1 - } - ] + const contentTypeOptions = useMemo( + () => + CONTENT_TYPE_OPTIONS.map((opt) => ({ + ...opt, + displayValue: + opt.queryParam === 'journeys' ? t('Team Projects') : t('Team Templates') + })), + [t] + )Then update the
useEffectdependency array to only includecontentTypeOptions(now stable).
227-273: Consider moving controls outside of Tabs for better semantics.Placing
Boxelements withFormControl,JourneySort, andJourneyListMenuas direct children ofTabsis unconventional. While MUI allows this, it may affect accessibility and tab navigation. Consider wrapping the tabs row in aBoxwithdisplay: flexand placing controls alongside theTabscomponent.- <Tabs - value={activeContentTypeTab} - onChange={handleContentTypeChange} - aria-label="journey content type tabs" - ... - > - <Tab ... /> - <Tab ... /> - {/* Status filter dropdown - visible for both tabs */} - <Box ...> - <FormControl ...>...</FormControl> - </Box> - ... - </Tabs> + <Box sx={{ display: 'flex', alignItems: 'center', width: '100%' }}> + <Tabs + value={activeContentTypeTab} + onChange={handleContentTypeChange} + aria-label="journey content type tabs" + ... + > + <Tab ... /> + <Tab ... /> + </Tabs> + <Box sx={{ ml: 'auto', display: 'flex', alignItems: 'center', gap: 2 }}> + <FormControl ...>...</FormControl> + <JourneySort ... /> + <JourneyListMenu ... /> + </Box> + </Box>libs/journeys/ui/src/components/TemplateView/CreateJourneyButton/CreateJourneyButton.tsx (1)
237-263: Consider adding aria-label to MenuItem for accessibility.The
Buttonvariant has accessibility attributes, but theMenuItemcould benefit from an explicitaria-labelfor screen readers, per coding guidelines.<MenuItem onClick={handleCheckSignIn} data-testid="CreateJourneyMenuItem" + aria-label={t('Use This Template')} >apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx (2)
110-143: MemoizegetQueryParamsto avoid recalculation on each render.
getQueryParams()is called in multiple places (line 145, 255) and recalculates the same values. Since it depends only oncontentTypeandstatus, consider usinguseMemo.- // Determine query parameters based on contentType and status - const getQueryParams = () => { + // Determine query parameters based on contentType and status + const queryParams = useMemo(() => { const isTemplate = contentType === 'templates' const baseParams: { status: JourneyStatus[] template?: boolean useLastActiveTeamId?: boolean } = { status: [] } // ... rest of switch logic return baseParams - } + }, [contentType, status]) - const { data, refetch } = useAdminJourneysQuery(getQueryParams()) + const { data, refetch } = useAdminJourneysQuery(queryParams)Then update
getOwnerFilteredIdsto usequeryParams.useLastActiveTeamIdinstead ofgetQueryParams().useLastActiveTeamId.
576-580: Remove redundantkeyprop on JourneyCard.The parent
Gridalready haskey={journey.id}. ThekeyonJourneyCardis redundant.<JourneyCard - key={journey.id} journey={journey} refetch={refetch} />
| <Box | ||
| sx={{ | ||
| display: 'flex', | ||
| flexDirection: 'column', | ||
| pt: status === 'active' ? 30 : 30 | ||
| }} | ||
| > | ||
| <Typography variant="subtitle1" align="center" gutterBottom> | ||
| {getEmptyStateMessage()} | ||
| </Typography> | ||
| </Box> |
There was a problem hiding this comment.
Remove redundant ternary expression.
The ternary status === 'active' ? 30 : 30 always evaluates to 30. Simplify to just 30.
<Box
sx={{
display: 'flex',
flexDirection: 'column',
- pt: status === 'active' ? 30 : 30
+ pt: 30
}}
>📝 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.
| <Box | |
| sx={{ | |
| display: 'flex', | |
| flexDirection: 'column', | |
| pt: status === 'active' ? 30 : 30 | |
| }} | |
| > | |
| <Typography variant="subtitle1" align="center" gutterBottom> | |
| {getEmptyStateMessage()} | |
| </Typography> | |
| </Box> | |
| <Box | |
| sx={{ | |
| display: 'flex', | |
| flexDirection: 'column', | |
| pt: 30 | |
| }} | |
| > | |
| <Typography variant="subtitle1" align="center" gutterBottom> | |
| {getEmptyStateMessage()} | |
| </Typography> | |
| </Box> |
🤖 Prompt for AI Agents
In
apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx
around lines 586 to 596, the Box sx prop uses a redundant ternary pt: status ===
'active' ? 30 : 30; replace the ternary with the constant value by setting pt:
30 directly to simplify the code and remove the unnecessary conditional.
| onClose={() => setOpenAccountDialog(false)} | ||
| /> | ||
| )} | ||
| {openTeamDialog != null && ( |
There was a problem hiding this comment.
Redundant null check on boolean state.
openTeamDialog is a boolean state initialized to false, so openTeamDialog != null is always true. This condition should likely just check if the dialog is open, or the condition may be a remnant from previous code.
- {openTeamDialog != null && (
+ {openTeamDialog && (
<DynamicCopyToTeamDialog📝 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.
| {openTeamDialog != null && ( | |
| {openTeamDialog && ( | |
| <DynamicCopyToTeamDialog |
🤖 Prompt for AI Agents
In
libs/journeys/ui/src/components/TemplateView/CreateJourneyButton/CreateJourneyButton.tsx
around line 275, the condition "openTeamDialog != null" is redundant because
openTeamDialog is a boolean initialized to false; replace the null-check with a
boolean check (e.g., "openTeamDialog && ( ... )") so the dialog is only rendered
when openTeamDialog is true, or remove the conditional entirely if the intent
was to always render the element.
|
This pull request has been marked stale due to 28 days without activity. It will be closed in 14 days unless updated. |
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.