Open
Conversation
Create ScreenWrapper component with responsive title/subtitle and migrate all 6 customization screens to use it. Fixes MediaScreen using wrong Typography variants and broken color prop. Refs: NES-1364 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Eliminate the headerSx prop from DoneScreen, LanguageScreen, LinksScreen, and ScreenWrapper components to streamline the code. Update TextScreen to maintain consistent mobileSubtitle formatting.
Refs: NES-1295 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Refs: NES-1297 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ScreenWrapper and migrates multiple MultiStepForm screens to use it; updates NotificationSwitch to render without a Tooltip when Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DoneScreen as "DoneScreen"
participant SyncDialog as "GoogleSheetsSyncDialog"
participant Backend as "GraphQL Backend"
User->>DoneScreen: Open DoneScreen
DoneScreen->>Backend: Query GET_GOOGLE_SHEETS_SYNCS_FOR_DONE_SCREEN (journeyId)
Backend-->>DoneScreen: Return syncs
DoneScreen->>DoneScreen: Compute hasActiveSyncs
alt has active syncs
DoneScreen->>User: Show "Edit" button
else no active syncs
DoneScreen->>User: Show "Sync" button
end
User->>DoneScreen: Click Sync/Edit
DoneScreen->>DoneScreen: set syncDialogOpen = true
DoneScreen->>SyncDialog: Render with journeyId
User->>SyncDialog: Complete OAuth (authCode) or confirm
SyncDialog->>Backend: Mutation INTEGRATION_GOOGLE_CREATE (code, redirectUri)
Backend-->>SyncDialog: Return integration result
SyncDialog->>SyncDialog: refetchIntegrations
SyncDialog->>DoneScreen: Close dialog (trigger refetch)
DoneScreen->>Backend: Re-query GET_GOOGLE_SHEETS_SYNCS_FOR_DONE_SCREEN
Backend-->>DoneScreen: Return updated syncs
DoneScreen->>User: Update UI (Sync/Edit state)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 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 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
edmonday
changed the title
edmonday
changed the base branch from
edmondshen/nes-1295-toggle-email-notifications-on-for-a-journey
to
main
![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: 1
🧹 Nitpick comments (4)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/SocialScreen/SocialScreen.tsx (1)
40-43: Prefer a namedhandle*function for the Done button click.Use a named handler instead of an inline lambda for consistency with event-handler naming conventions.
As per coding guidelines, event functions should be named with a “handle” prefix, like “handleClick” for onClick and “handleKeyDown” for onKeyDown.♻️ Suggested refactor
export function SocialScreen({ handleNext, handleScreenNavigation }: SocialScreenProps): ReactElement { const { t } = useTranslation('apps-journeys-admin') + function handleDoneClick(): void { + handleNext() + } return ( @@ footer={ <CustomizeFlowNextButton label={t('Done')} - onClick={() => handleNext()} + onClick={handleDoneClick} ariaLabel={t('Done')} /> }🤖 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/SocialScreen/SocialScreen.tsx` around lines 40 - 43, Replace the inline arrow used for the Done button with a named handler to follow event-handler naming conventions: create a new function (e.g., handleDoneClick or handleDone) in the SocialScreen component and call that from the CustomizeFlowNextButton's onClick prop instead of () => handleNext(); keep the existing behavior by invoking handleNext() (or delegating to it) inside the new handler and update ariaLabel/label usage unchanged.apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/MediaScreen/MediaScreen.tsx (1)
89-93: Use a namedhandle*callback for the footer click handler.Please replace the inline
onClicklambda with a named handler to keep event callbacks consistent and easier to trace.As per coding guidelines, event functions should be named with a “handle” prefix, like “handleClick” for onClick and “handleKeyDown” for onKeyDown.♻️ Suggested refactor
export function MediaScreen({ handleNext }: MediaScreenProps): ReactElement { const { t } = useTranslation('apps-journeys-admin') @@ + function handleNextClick(): void { + handleNext() + } + return ( @@ footer={ <CustomizeFlowNextButton label={t('Next')} - onClick={() => handleNext()} + onClick={handleNextClick} ariaLabel={t('Next')} loading={hasActiveUploads} /> }🤖 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/MediaScreen/MediaScreen.tsx` around lines 89 - 93, The inline lambda on the CustomizeFlowNextButton (onClick={() => handleNext()}) should be replaced with a named handler to follow the handle* convention; add a new function like handleNextClick that calls the existing handleNext (optionally wrap with useCallback if in a React component), then pass handleNextClick as the onClick prop and keep loading={hasActiveUploads}, label and ariaLabel unchanged so the footer click handler is traceable and consistent.apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx (1)
224-249: Use namedhandle*event callbacks instead of inline lambdas.Line 224 and Line 248 should use named handlers (
handleLanguageChange,handleNextClick) to match project event-handler conventions and improve readability.✍️ Suggested refactor
- <LanguageAutocomplete + <LanguageAutocomplete value={values.languageSelect} languages={languages.map((language) => ({ id: language?.id, name: language?.name, slug: language?.slug }))} - onChange={(value) => setFieldValue('languageSelect', value)} + onChange={handleLanguageChange} /> ... <CustomizeFlowNextButton label={t('Next')} - onClick={() => handleSubmit()} + onClick={handleNextClick} disabled={ (templateCustomizationGuestFlow && !isSignedIn) || loading } ariaLabel={t('Next')} />const handleLanguageChange = (value): void => { setFieldValue('languageSelect', value) } function handleNextClick(): void { void handleSubmit() }As per coding guidelines: “event functions should be named with a ‘handle’ prefix, like ‘handleClick’ for onClick and ‘handleKeyDown’ for onKeyDown.”
🤖 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 224 - 249, Replace the inline event lambdas with named handler functions: create a handleLanguageChange that accepts the value and calls setFieldValue('languageSelect', value) and create handleNextClick that calls/awaits handleSubmit (or void handleSubmit() to preserve current behavior); then update the onChange prop (currently calling setFieldValue inline) to onChange={handleLanguageChange} and the CustomizeFlowNextButton onClick (currently () => handleSubmit()) to onClick={handleNextClick} so handlers follow the project’s handle* naming convention and improve readability.apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.spec.tsx (1)
237-396: Extract a shared render helper for the repeated provider tree.Line 239, Line 256, Line 274, Line 352, Line 371, and Line 389 repeat nearly identical setup. A helper will reduce drift and make future test updates safer.
♻️ Suggested refactor
+function renderDoneScreen({ + mocks = [getCustomDomainsMock, googleSheetsSyncsNoActiveMock], + journeyOverride = journey +}: { + mocks?: MockedResponse[] + journeyOverride?: typeof journey +} = {}): void { + render( + <SnackbarProvider> + <MockedProvider mocks={mocks}> + <JourneyProvider value={{ journey: journeyOverride, variant: 'admin' }}> + <DoneScreen /> + </JourneyProvider> + </MockedProvider> + </SnackbarProvider> + ) +}🤖 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/DoneScreen/DoneScreen.spec.tsx` around lines 237 - 396, Extract a shared test render helper (e.g., renderDoneScreen) to replace the repeated provider tree around DoneScreen: create a function that accepts optional args for mocks array and a journey override and calls render(<SnackbarProvider><MockedProvider mocks={mocks}><JourneyProvider value={{ journey: journeyOverride, variant: 'admin' }}><DoneScreen /></JourneyProvider></MockedProvider></SnackbarProvider>), then update each test to call renderDoneScreen with the appropriate mocks and/or journeyWithTeam; reference DoneScreen, MockedProvider, SnackbarProvider, JourneyProvider and the new render helper to locate and replace duplicated render blocks.
🤖 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/DoneScreen/DoneScreen.tsx`:
- Around line 153-157: The NotificationSwitch rendered in DoneScreen currently
lacks an explicit accessible name, causing assistive tech to treat the switch as
unnamed; update NotificationSwitch to accept a new prop (e.g., ariaLabel or
label) and forward it into the underlying MUI Switch via inputProps (set
'aria-label' and keep 'aria-checked'), then pass a meaningful label from
DoneScreen (e.g., t('Send to my email')) when rendering <NotificationSwitch
journeyId={journey?.id} ariaLabel={t('Send to my email')} /> so the switch has
an explicit accessible name.
---
Nitpick comments:
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.spec.tsx`:
- Around line 237-396: Extract a shared test render helper (e.g.,
renderDoneScreen) to replace the repeated provider tree around DoneScreen:
create a function that accepts optional args for mocks array and a journey
override and calls render(<SnackbarProvider><MockedProvider
mocks={mocks}><JourneyProvider value={{ journey: journeyOverride, variant:
'admin' }}><DoneScreen
/></JourneyProvider></MockedProvider></SnackbarProvider>), then update each test
to call renderDoneScreen with the appropriate mocks and/or journeyWithTeam;
reference DoneScreen, MockedProvider, SnackbarProvider, JourneyProvider and the
new render helper to locate and replace duplicated render blocks.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx`:
- Around line 224-249: Replace the inline event lambdas with named handler
functions: create a handleLanguageChange that accepts the value and calls
setFieldValue('languageSelect', value) and create handleNextClick that
calls/awaits handleSubmit (or void handleSubmit() to preserve current behavior);
then update the onChange prop (currently calling setFieldValue inline) to
onChange={handleLanguageChange} and the CustomizeFlowNextButton onClick
(currently () => handleSubmit()) to onClick={handleNextClick} so handlers follow
the project’s handle* naming convention and improve readability.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/MediaScreen/MediaScreen.tsx`:
- Around line 89-93: The inline lambda on the CustomizeFlowNextButton
(onClick={() => handleNext()}) should be replaced with a named handler to follow
the handle* convention; add a new function like handleNextClick that calls the
existing handleNext (optionally wrap with useCallback if in a React component),
then pass handleNextClick as the onClick prop and keep
loading={hasActiveUploads}, label and ariaLabel unchanged so the footer click
handler is traceable and consistent.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/SocialScreen/SocialScreen.tsx`:
- Around line 40-43: Replace the inline arrow used for the Done button with a
named handler to follow event-handler naming conventions: create a new function
(e.g., handleDoneClick or handleDone) in the SocialScreen component and call
that from the CustomizeFlowNextButton's onClick prop instead of () =>
handleNext(); keep the existing behavior by invoking handleNext() (or delegating
to it) inside the new handler and update ariaLabel/label usage unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/journeys-admin/src/components/AccessDialog/NotificationSwitch/NotificationSwitch.spec.tsxapps/journeys-admin/src/components/AccessDialog/NotificationSwitch/NotificationSwitch.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LinksScreen/LinksScreen.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/MediaScreen/MediaScreen.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/ScreenWrapper/ScreenWrapper.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/ScreenWrapper/index.tsapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/SocialScreen/SocialScreen.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/TextScreen/TextScreen.tsx
Comment on lines
+153
to
+157
| <Typography variant="body2"> | ||
| {t('Send to my email')} | ||
| </Typography> | ||
| <NotificationSwitch journeyId={journey?.id} /> | ||
| </Stack> |
Contributor
There was a problem hiding this comment.
Add an explicit accessible name to the email notification switch.
Line 156 renders a switch without a guaranteed accessible label association; assistive tech can announce this as an unnamed checkbox. Please pass an explicit label prop through NotificationSwitch to the underlying <Switch inputProps>.
♿ Suggested fix
- <NotificationSwitch journeyId={journey?.id} />
+ <NotificationSwitch
+ journeyId={journey?.id}
+ ariaLabel={t('Send to my email')}
+ />// In NotificationSwitch props/component (separate file):
// add `ariaLabel?: string` and forward it:
<Switch
inputProps={{
'aria-checked': checked,
'aria-label': ariaLabel
}}
...
/>As per coding guidelines: “Implement accessibility features on elements… [including] aria-label…”.
🤖 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/DoneScreen/DoneScreen.tsx`
around lines 153 - 157, The NotificationSwitch rendered in DoneScreen currently
lacks an explicit accessible name, causing assistive tech to treat the switch as
unnamed; update NotificationSwitch to accept a new prop (e.g., ariaLabel or
label) and forward it into the underlying MUI Switch via inputProps (set
'aria-label' and keep 'aria-checked'), then pass a meaningful label from
DoneScreen (e.g., t('Send to my email')) when rendering <NotificationSwitch
journeyId={journey?.id} ariaLabel={t('Send to my email')} /> so the switch has
an explicit accessible name.
…ialog - Added mutation for creating Google integration. - Implemented OAuth code handling directly in the sync dialog. - Updated routing logic to support new integration flow. - Enhanced user feedback with success/error notifications. Refs: NES-1298
|
View your CI Pipeline Execution ↗ for commit 4702e8a
☁️ Nx Cloud last updated this comment at |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.tsx (1)
175-188: Consider a more semantically appropriate button color.Using
color="error"for a "Sync" or "Edit" action is semantically misleading—error color (typically red) suggests a destructive or warning state. Consider usingcolor="primary"orcolor="secondary"unless the red styling is an intentional brand decision.♻️ Suggested change
<Button data-testid="GoogleSheetsSyncButton" onClick={handleSyncDialogOpen} - color="error" + color="primary" aria-label={ hasActiveSyncs ? t('Edit Google Sheets sync') : t('Sync to Google Sheets') } >🤖 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/DoneScreen/DoneScreen.tsx` around lines 175 - 188, The Button rendering the Google Sheets sync action uses color="error", which is semantically misleading for a non-destructive "Sync"/"Edit" action; update the Button (data-testid "GoogleSheetsSyncButton", onClick handleSyncDialogOpen, conditional label using hasActiveSyncs and t) to use a non-destructive color such as color="primary" (or color="secondary" if that matches your design system) instead of "error".apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx (1)
120-127: Consider extracting shared GraphQL mutation.This
INTEGRATION_GOOGLE_CREATEmutation is duplicated fromGoogleCreateIntegration.tsx. Extracting it to a shared GraphQL operations file would improve maintainability and ensure consistency if the mutation schema changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx` around lines 120 - 127, The INTEGRATION_GOOGLE_CREATE GraphQL mutation defined in GoogleSheetsSyncDialog (symbol: INTEGRATION_GOOGLE_CREATE) is duplicated in GoogleCreateIntegration.tsx; extract this shared mutation into a common operations file (e.g., a new shared GraphQLOperations or integrations/operations module), export INTEGRATION_GOOGLE_CREATE from there, and replace the local definitions in both GoogleSheetsSyncDialog and GoogleCreateIntegration to import the single exported mutation so schema changes remain consistent and maintainable.
🤖 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/Google/GoogleCreateIntegration/GoogleCreateIntegration.tsx`:
- Around line 115-119: The fallbackPath logic currently uses the
query-controlled returnTo directly which can cause open-redirects; update the
code around fallbackPath/router.push to validate returnTo before using it:
ensure returnTo is a same-origin internal path (e.g., starts with a single '/'
and does not start with '//' or contain a protocol like 'http:'), otherwise fall
back to `/teams/${teamId}/integrations`; perform this check where fallbackPath
is computed (referencing returnTo, fallbackPath, teamId) and only pass the
validated path to router.push.
In
`@apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx`:
- Around line 252-257: The effect that reads router.query.code and calls
handleIntegrationCreate can miss the OAuth code because
journeyData?.journey?.team?.id isn’t a dependency; update the effect so it
re-runs when the team id becomes available (e.g., add
journeyData?.journey?.team?.id to the dependency array) or memoize
handleIntegrationCreate with useCallback (including team id) and include it in
the dependencies; also ensure you guard so the authCode (from router.query.code)
is processed exactly once when openSyncDialog is true and team id exists before
calling handleIntegrationCreate.
- Around line 236-240: The catch in GoogleSheetsSyncDialog currently only
handles instances of Error and silently ignores other thrown values; update the
catch block in the async handler inside GoogleSheetsSyncDialog to always surface
errors by calling enqueueSnackbar for non-Error throws as well (e.g.,
enqueueSnackbar(String(error), { variant: 'error' })) and log the original value
(console.error(error)) so strings/custom objects are not swallowed; ensure the
existing Error branch remains to preserve error.message usage.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.tsx`:
- Around line 54-60: The effect reads router.query.openSyncDialog and opens the
dialog via setSyncDialogOpen but never clears the query param; update the
useEffect (watching journey?.id and router.query.openSyncDialog) so that after
you setSyncDialogOpen(true) you remove openSyncDialog from the URL using
router.replace (preserve other query params, use shallow routing or replace to
avoid full navigation and scroll), e.g., build a new query object without
router.query.openSyncDialog and call router.replace with that query; ensure this
runs only when openSyncDialog is true so you don't remove params unnecessarily.
---
Nitpick comments:
In
`@apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx`:
- Around line 120-127: The INTEGRATION_GOOGLE_CREATE GraphQL mutation defined in
GoogleSheetsSyncDialog (symbol: INTEGRATION_GOOGLE_CREATE) is duplicated in
GoogleCreateIntegration.tsx; extract this shared mutation into a common
operations file (e.g., a new shared GraphQLOperations or integrations/operations
module), export INTEGRATION_GOOGLE_CREATE from there, and replace the local
definitions in both GoogleSheetsSyncDialog and GoogleCreateIntegration to import
the single exported mutation so schema changes remain consistent and
maintainable.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.tsx`:
- Around line 175-188: The Button rendering the Google Sheets sync action uses
color="error", which is semantically misleading for a non-destructive
"Sync"/"Edit" action; update the Button (data-testid "GoogleSheetsSyncButton",
onClick handleSyncDialogOpen, conditional label using hasActiveSyncs and t) to
use a non-destructive color such as color="primary" (or color="secondary" if
that matches your design system) instead of "error".
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/journeys-admin/src/components/Google/GoogleCreateIntegration/GoogleCreateIntegration.tsxapps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.tsx
Comment on lines
+115
to
+119
| const fallbackPath = | ||
| returnTo != null && returnTo !== '' | ||
| ? returnTo | ||
| : `/teams/${teamId}/integrations` | ||
| await router.push(fallbackPath) |
Contributor
There was a problem hiding this comment.
Validate returnTo before navigation to avoid an open-redirect path.
Line 115–119 pushes returnTo (query-param-controlled) directly. A crafted absolute URL can redirect users off-site in the error path. Please constrain redirects to same-origin internal paths before calling router.push.
🔒 Proposed fix
- const fallbackPath =
- returnTo != null && returnTo !== ''
- ? returnTo
- : `/teams/${teamId}/integrations`
- await router.push(fallbackPath)
+ const defaultPath = `/teams/${teamId}/integrations`
+ const fallbackPath =
+ returnTo != null && returnTo !== '' ? returnTo : defaultPath
+
+ const safePath = (() => {
+ try {
+ const parsed = new URL(fallbackPath, window.location.origin)
+ return parsed.origin === window.location.origin
+ ? `${parsed.pathname}${parsed.search}${parsed.hash}`
+ : defaultPath
+ } catch {
+ return defaultPath
+ }
+ })()
+
+ await router.push(safePath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/journeys-admin/src/components/Google/GoogleCreateIntegration/GoogleCreateIntegration.tsx`
around lines 115 - 119, The fallbackPath logic currently uses the
query-controlled returnTo directly which can cause open-redirects; update the
code around fallbackPath/router.push to validate returnTo before using it:
ensure returnTo is a same-origin internal path (e.g., starts with a single '/'
and does not start with '//' or contain a protocol like 'http:'), otherwise fall
back to `/teams/${teamId}/integrations`; perform this check where fallbackPath
is computed (referencing returnTo, fallbackPath, teamId) and only pass the
validated path to router.push.
Comment on lines
+236
to
+240
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| enqueueSnackbar(error.message, { variant: 'error' }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Silently swallows non-Error exceptions.
The catch block only handles Error instances. If something else is thrown (e.g., a string or custom object), it will be silently ignored and the user won't see any feedback.
🛡️ Proposed fix
} catch (error) {
- if (error instanceof Error) {
- enqueueSnackbar(error.message, { variant: 'error' })
- }
+ const message =
+ error instanceof Error
+ ? error.message
+ : t('Google settings failed. Reload the page or try again.')
+ enqueueSnackbar(message, { variant: 'error' })
}📝 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
| } catch (error) { | |
| if (error instanceof Error) { | |
| enqueueSnackbar(error.message, { variant: 'error' }) | |
| } | |
| } | |
| } catch (error) { | |
| const message = | |
| error instanceof Error | |
| ? error.message | |
| : t('Google settings failed. Reload the page or try again.') | |
| enqueueSnackbar(message, { variant: 'error' }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx`
around lines 236 - 240, The catch in GoogleSheetsSyncDialog currently only
handles instances of Error and silently ignores other thrown values; update the
catch block in the async handler inside GoogleSheetsSyncDialog to always surface
errors by calling enqueueSnackbar for non-Error throws as well (e.g.,
enqueueSnackbar(String(error), { variant: 'error' })) and log the original value
(console.error(error)) so strings/custom objects are not swallowed; ensure the
existing Error branch remains to preserve error.message usage.
Comment on lines
+252
to
+257
| // New flow: handle OAuth code exchange directly | ||
| const authCode = router.query.code as string | undefined | ||
| if (authCode != null && openSyncDialog) { | ||
| void handleIntegrationCreate(authCode) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Potential race condition: OAuth code may not be processed if journeyData hasn't loaded.
The handleIntegrationCreate function depends on journeyData?.journey?.team?.id (line 202-203), but journeyData is not in the useEffect's dependency array. If the dialog opens before journeyData loads (e.g., on a fresh page load after OAuth redirect), the function returns early and the auth code remains unprocessed. The effect won't re-run when journeyData becomes available.
🔧 Suggested approach
Add journeyData?.journey?.team?.id to the dependency array and ensure the auth code is only processed once:
useEffect(() => {
if (!open) return
+ const teamId = journeyData?.journey?.team?.id
void loadSyncs({
variables: { filter: { journeyId } },
fetchPolicy: 'network-only'
})
const openSyncDialog = router.query.openSyncDialog === 'true'
// New flow: handle OAuth code exchange directly
const authCode = router.query.code as string | undefined
- if (authCode != null && openSyncDialog) {
+ if (authCode != null && openSyncDialog && teamId != null) {
void handleIntegrationCreate(authCode)
return
}
// ... rest of effect
- }, [open, journeyId, loadSyncs, router, enqueueSnackbar, t])
+ }, [open, journeyId, loadSyncs, router, enqueueSnackbar, t, journeyData?.journey?.team?.id])Alternatively, wrap handleIntegrationCreate in useCallback with proper dependencies and include it in the effect's dependency array.
Also applies to: 275-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx`
around lines 252 - 257, The effect that reads router.query.code and calls
handleIntegrationCreate can miss the OAuth code because
journeyData?.journey?.team?.id isn’t a dependency; update the effect so it
re-runs when the team id becomes available (e.g., add
journeyData?.journey?.team?.id to the dependency array) or memoize
handleIntegrationCreate with useCallback (including team id) and include it in
the dependencies; also ensure you guard so the authCode (from router.query.code)
is processed exactly once when openSyncDialog is true and team id exists before
calling handleIntegrationCreate.
Comment on lines
+54
to
+60
| useEffect(() => { | ||
| if (journey?.id == null) return | ||
| const openSyncDialog = router.query.openSyncDialog === 'true' | ||
| if (openSyncDialog) { | ||
| setSyncDialogOpen(true) | ||
| } | ||
| }, [journey?.id, router.query.openSyncDialog]) |
Contributor
There was a problem hiding this comment.
Clear the query parameter after opening the dialog to prevent re-triggering.
The openSyncDialog query parameter is read but never removed from the URL. This can cause the dialog to reopen unexpectedly if the component re-renders or the journey changes, and leaves a "dirty" URL.
🔧 Suggested fix
useEffect(() => {
if (journey?.id == null) return
const openSyncDialog = router.query.openSyncDialog === 'true'
if (openSyncDialog) {
setSyncDialogOpen(true)
+ // Remove query param to prevent re-triggering
+ const { openSyncDialog: _, ...restQuery } = router.query
+ void router.replace({ pathname: router.pathname, query: restQuery }, undefined, { shallow: true })
}
}, [journey?.id, router.query.openSyncDialog])📝 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
| useEffect(() => { | |
| if (journey?.id == null) return | |
| const openSyncDialog = router.query.openSyncDialog === 'true' | |
| if (openSyncDialog) { | |
| setSyncDialogOpen(true) | |
| } | |
| }, [journey?.id, router.query.openSyncDialog]) | |
| useEffect(() => { | |
| if (journey?.id == null) return | |
| const openSyncDialog = router.query.openSyncDialog === 'true' | |
| if (openSyncDialog) { | |
| setSyncDialogOpen(true) | |
| // Remove query param to prevent re-triggering | |
| const { openSyncDialog: _, ...restQuery } = router.query | |
| void router.replace({ pathname: router.pathname, query: restQuery }, undefined, { shallow: true }) | |
| } | |
| }, [journey?.id, router.query.openSyncDialog]) |
🤖 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/DoneScreen/DoneScreen.tsx`
around lines 54 - 60, The effect reads router.query.openSyncDialog and opens the
dialog via setSyncDialogOpen but never clears the query param; update the
useEffect (watching journey?.id and router.query.openSyncDialog) so that after
you setSyncDialogOpen(true) you remove openSyncDialog from the URL using
router.replace (preserve other query params, use shallow routing or replace to
avoid full navigation and scroll), e.g., build a new query object without
router.query.openSyncDialog and call router.replace with that query; ensure this
runs only when openSyncDialog is true so you don't remove params unnecessarily.
Contributor
|
The latest updates on your projects.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx (2)
253-258:⚠️ Potential issue | 🟠 MajorPotential race condition: OAuth code may not be processed if
journeyDatahasn't loaded.The
handleIntegrationCreatefunction depends onjourneyData?.journey?.team?.id(line 203-204), butjourneyDatais not in the useEffect's dependency array. If the dialog opens beforejourneyDataloads (e.g., on a fresh page load after OAuth redirect), the function returns early and the auth code remains unprocessed. The effect won't re-run whenjourneyDatabecomes available.🔧 Suggested fix
Add
journeyData?.journey?.team?.idto the dependency array:useEffect(() => { if (!open) return + const teamId = journeyData?.journey?.team?.id void loadSyncs({ variables: { filter: { journeyId } }, fetchPolicy: 'network-only' }) const openSyncDialog = router.query.openSyncDialog === 'true' // New flow: handle OAuth code exchange directly const authCode = router.query.code as string | undefined - if (authCode != null && openSyncDialog) { + if (authCode != null && openSyncDialog && teamId != null) { void handleIntegrationCreate(authCode) return } // ... rest of effect - }, [open, journeyId, loadSyncs, router, enqueueSnackbar, t]) + }, [open, journeyId, loadSyncs, router, enqueueSnackbar, t, journeyData?.journey?.team?.id])Also applies to: 276-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx` around lines 253 - 258, The effect that checks router.query.code and calls handleIntegrationCreate can miss the OAuth code because journeyData (used inside handleIntegrationCreate for journeyData?.journey?.team?.id) is not watched; update the useEffect dependency array to include the team id expression (journeyData?.journey?.team?.id) along with router.query.code and openSyncDialog so the effect re-runs when journeyData arrives and handleIntegrationCreate (which uses journeyData) gets the authCode to process.
237-241:⚠️ Potential issue | 🟡 MinorSilently swallows non-Error exceptions.
The catch block only handles
Errorinstances. If something else is thrown (e.g., a string or custom object), it will be silently ignored and the user won't see any feedback.🛡️ Proposed fix
} catch (error) { - if (error instanceof Error) { - enqueueSnackbar(error.message, { variant: 'error' }) - } + const message = + error instanceof Error + ? error.message + : t('Google settings failed. Reload the page or try again.') + enqueueSnackbar(message, { variant: 'error' }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx` around lines 237 - 241, The catch in GoogleSheetsSyncDialog currently only handles Error instances and ignores other thrown values; update the error handling in the async try/catch (inside the GoogleSheetsSyncDialog component) to accept error as unknown, derive a user message with const message = error instanceof Error ? error.message : String(error), call enqueueSnackbar(message, { variant: 'error' }), and also console.error(error) (or processLogger) so non-Error exceptions (strings, objects, etc.) are surfaced and logged.apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.tsx (2)
163-163:⚠️ Potential issue | 🟡 MinorAdd an explicit accessible name to the email notification switch.
The NotificationSwitch is rendered without a guaranteed accessible label association. Assistive technology may announce this as an unnamed checkbox. Consider passing an explicit
ariaLabelprop.♿ Suggested fix
- <NotificationSwitch journeyId={journey?.id} /> + <NotificationSwitch + journeyId={journey?.id} + ariaLabel={t('Send to my email')} + />This requires updating
NotificationSwitchto accept and forward theariaLabelprop to the underlying Switch component.As per coding guidelines: "Implement accessibility features on elements… [including] aria-label…".
🤖 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/DoneScreen/DoneScreen.tsx` at line 163, The NotificationSwitch rendered in DoneScreen (NotificationSwitch) lacks an explicit accessible name; update the usage and component to accept an ariaLabel prop and pass it through to the underlying Switch/input so assistive tech can announce it. Modify NotificationSwitch's props to include ariaLabel: string (or optional), forward that prop to the inner Switch/Input element (e.g., <Switch aria-label={ariaLabel} /> or equivalent), and update the DoneScreen invocation to supply a descriptive label such as ariaLabel="Email notifications for this journey". Ensure prop forwarding is correctly typed in NotificationSwitch's function/component signature and passed unchanged to the DOM control.
54-60:⚠️ Potential issue | 🟡 MinorClear the query parameter after opening the dialog to prevent re-triggering.
The
openSyncDialogquery parameter is read but never removed from the URL. This can cause the dialog to reopen unexpectedly if the component re-renders, and leaves a "dirty" URL.🔧 Suggested fix
useEffect(() => { if (journey?.id == null) return const openSyncDialog = router.query.openSyncDialog === 'true' if (openSyncDialog) { setSyncDialogOpen(true) + // Remove query param to prevent re-triggering + const { openSyncDialog: _, ...restQuery } = router.query + void router.replace({ pathname: router.pathname, query: restQuery }, undefined, { shallow: true }) } - }, [journey?.id, router.query.openSyncDialog]) + }, [journey?.id, router.query.openSyncDialog, router])🤖 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/DoneScreen/DoneScreen.tsx` around lines 54 - 60, The effect reads router.query.openSyncDialog and opens the dialog but never removes the query param; update the effect (useEffect) so that after calling setSyncDialogOpen(true) you remove openSyncDialog from the URL using router.replace (or router.push) with the current pathname and a new query object that omits openSyncDialog, passing { shallow: true } to avoid a full reload; reference the useEffect, setSyncDialogOpen, router.query.openSyncDialog and router.replace symbols when implementing this change.
🧹 Nitpick comments (2)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.tsx (1)
173-186: Reconsider the button color for Google Sheets sync action.Using
color="error"for the Sync/Edit button is semantically misleading—error color typically indicates destructive or warning actions, not neutral sync operations. Consider usingcolor="primary"orcolor="secondary"instead.🎨 Suggested fix
<Button data-testid="GoogleSheetsSyncButton" onClick={handleSyncDialogOpen} - color="error" + color="primary" aria-label={ hasActiveSyncs ? t('Edit Google Sheets sync') : t('Sync to Google Sheets') } >🤖 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/DoneScreen/DoneScreen.tsx` around lines 173 - 186, The Button in DoneScreen (the element with data-testid "GoogleSheetsSyncButton" inside the DoneScreen component that calls handleSyncDialogOpen) uses color="error", which is misleading for a non-destructive sync/edit action; update the Button's color prop to a neutral action color such as "primary" or "secondary" (e.g., color="primary") so the Google Sheets Sync/Edit action is properly styled and semantically correct.apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx (1)
120-126: Consider consolidating duplicate mutation definition.
INTEGRATION_GOOGLE_CREATEis also defined inGoogleCreateIntegration.tsx. Consider exporting and reusing it from a shared location to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx` around lines 120 - 126, Duplicate GraphQL mutation INTEGRATION_GOOGLE_CREATE appears here and in GoogleCreateIntegration.tsx; remove the local definition and import the shared constant instead: extract the gql mutation into a single exported constant (e.g., INTEGRATION_GOOGLE_CREATE) in a shared module or the existing GoogleCreateIntegration.tsx, export it, then update GoogleSheetsSyncDialog.tsx to import and use that exported mutation constant rather than redefining it; ensure any tests or usages still reference the exported symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx`:
- Around line 253-258: The effect that checks router.query.code and calls
handleIntegrationCreate can miss the OAuth code because journeyData (used inside
handleIntegrationCreate for journeyData?.journey?.team?.id) is not watched;
update the useEffect dependency array to include the team id expression
(journeyData?.journey?.team?.id) along with router.query.code and openSyncDialog
so the effect re-runs when journeyData arrives and handleIntegrationCreate
(which uses journeyData) gets the authCode to process.
- Around line 237-241: The catch in GoogleSheetsSyncDialog currently only
handles Error instances and ignores other thrown values; update the error
handling in the async try/catch (inside the GoogleSheetsSyncDialog component) to
accept error as unknown, derive a user message with const message = error
instanceof Error ? error.message : String(error), call enqueueSnackbar(message,
{ variant: 'error' }), and also console.error(error) (or processLogger) so
non-Error exceptions (strings, objects, etc.) are surfaced and logged.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.tsx`:
- Line 163: The NotificationSwitch rendered in DoneScreen (NotificationSwitch)
lacks an explicit accessible name; update the usage and component to accept an
ariaLabel prop and pass it through to the underlying Switch/input so assistive
tech can announce it. Modify NotificationSwitch's props to include ariaLabel:
string (or optional), forward that prop to the inner Switch/Input element (e.g.,
<Switch aria-label={ariaLabel} /> or equivalent), and update the DoneScreen
invocation to supply a descriptive label such as ariaLabel="Email notifications
for this journey". Ensure prop forwarding is correctly typed in
NotificationSwitch's function/component signature and passed unchanged to the
DOM control.
- Around line 54-60: The effect reads router.query.openSyncDialog and opens the
dialog but never removes the query param; update the effect (useEffect) so that
after calling setSyncDialogOpen(true) you remove openSyncDialog from the URL
using router.replace (or router.push) with the current pathname and a new query
object that omits openSyncDialog, passing { shallow: true } to avoid a full
reload; reference the useEffect, setSyncDialogOpen, router.query.openSyncDialog
and router.replace symbols when implementing this change.
---
Nitpick comments:
In
`@apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsx`:
- Around line 120-126: Duplicate GraphQL mutation INTEGRATION_GOOGLE_CREATE
appears here and in GoogleCreateIntegration.tsx; remove the local definition and
import the shared constant instead: extract the gql mutation into a single
exported constant (e.g., INTEGRATION_GOOGLE_CREATE) in a shared module or the
existing GoogleCreateIntegration.tsx, export it, then update
GoogleSheetsSyncDialog.tsx to import and use that exported mutation constant
rather than redefining it; ensure any tests or usages still reference the
exported symbol.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.tsx`:
- Around line 173-186: The Button in DoneScreen (the element with data-testid
"GoogleSheetsSyncButton" inside the DoneScreen component that calls
handleSyncDialogOpen) uses color="error", which is misleading for a
non-destructive sync/edit action; update the Button's color prop to a neutral
action color such as "primary" or "secondary" (e.g., color="primary") so the
Google Sheets Sync/Edit action is properly styled and semantically correct.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/journeys-admin/src/components/JourneyVisitorsList/FilterDrawer/GoogleSheetsSyncDialog/GoogleSheetsSyncDialog.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/DoneScreen/DoneScreen.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/TextScreen/TextScreen.tsxlibs/locales/en/apps-journeys-admin.json
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
edmonday
changed the base branch from
main
to
edmondshen/nes-1295-toggle-email-notifications-on-for-a-journey
edmonday
changed the base branch from
edmondshen/nes-1295-toggle-email-notifications-on-for-a-journey
to
main
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
GoogleSheetsSyncDialogon click; refetches syncs on dialog closeMerge Order
Verification
pnpm run build: passedDoneScreen.tsx,DoneScreen.spec.tsxLinear Issue
NES-1297
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests