refactor: extract wrapper for shared screen layout#8792
Open
refactor: extract wrapper for shared screen layout#8792
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>
Contributor
WalkthroughThis refactoring removes the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate 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 f3b8f24
☁️ Nx Cloud last updated this comment at |
Contributor
|
The latest updates on your projects.
|
edmonday
changed the title
Eliminate the headerSx prop from DoneScreen, LanguageScreen, LinksScreen, and ScreenWrapper components to streamline the code. Update TextScreen to maintain consistent mobileSubtitle formatting.
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.tsx (1)
138-153:⚠️ Potential issue | 🟠 MajorKeep a manual-editor escape hatch in the flow.
This block now renders only the stepper and active screen. Since
DoneScreenonly exposes Preview, Share, and Dashboard, the previous manual-editor route disappears entirely, which is a behavior regression rather than a layout refactor.🤖 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/MultiStepForm.tsx` around lines 138 - 153, The change removed the manual-editor "escape hatch" by wrapping the stepper and screen rendering in a feature-check conditional, which hides the manual-editor route when hasEditableText/hasCustomizableLinks/hasCustomizableMedia are false; update MultiStepForm so the manual-editor escape hatch (the link/button that navigates to the manual editor) is rendered even when those feature flags are false—either render the escape hatch unconditionally alongside renderScreen(activeScreen, handleNext) or explicitly render it when activeScreen corresponds to DoneScreen; modify MultiStepForm to add that unconditional/DoneScreen-specific render (referencing MultiStepForm, renderScreen, activeScreen, handleNext, screens, DoneScreen and ProgressStepper) so the manual editor route is preserved.
🧹 Nitpick comments (6)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.spec.tsx (1)
134-141: Trim the stale navigation button from theDoneScreenmock.
DoneScreenis prop-less now, sodone-screen-go-to-languageno longer represents a real interaction. Dropping that dead control will keep the test harness aligned with the simplified flow and avoid future specs depending on removed behavior.🤖 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/MultiStepForm.spec.tsx` around lines 134 - 141, The test mock for DoneScreen contains a stale navigation button (data-testid "done-screen-go-to-language") that no longer matches the component's prop-less API; remove the button element from the DoneScreen mock so the JSX for DoneScreen only renders the container (data-testid "done-screen") and its static content (e.g., the <h2>), ensuring tests no longer reference the obsolete "done-screen-go-to-language" control.apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/ScreenWrapper/ScreenWrapper.tsx (1)
41-73: Render a single responsive heading/subtitle pair.Right now both breakpoint variants stay in the DOM, which is already leaking into downstream specs—
MediaScreen.spec.tsxhad to switch togetAllByText(...)[0]to cope with the duplicates. A single responsive text node, or explicitdata-testid/aria-hiddenon the hidden copy, would keep the wrapper easier to test and less implementation-coupled.🤖 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/ScreenWrapper/ScreenWrapper.tsx` around lines 41 - 73, The two heading/subtitle Typography nodes render simultaneously (one hidden via CSS) causing duplicate DOM; in ScreenWrapper replace the duplicated Typography blocks with a single responsive render (use useMediaQuery or MUI Hidden) so only one text node exists: determine isSmUp = useMediaQuery(theme.breakpoints.up('sm')) and render one Typography for the title where variant = isSmUp ? 'h3' : 'h6' and content = isSmUp ? title : (mobileTitle ?? title), and likewise render one Typography for the subtitle with display and content chosen from subtitle/mobileSubtitle; alternatively, if you must keep both nodes, add explicit data-testid or aria-hidden on the hidden copy to avoid duplicate-test fallout.apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.tsx (1)
48-57: Narrow the newbuttonPropssurface before exporting it.
ItemspreadsButtonPropsafter its own props, so callers can overrideonClick,href,component, orvariantand bypasshandleShowMenu. Since the current use case is styling fromDoneScreen, wire upbuttonSx(currently unused) or whitelist non-behavioral button props instead of forwarding the fullButtoncontract.Also applies to: 70-78, 127-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.tsx` around lines 48 - 57, The ShareItem currently exposes a full buttonProps prop which allows callers to override behavioral props (onClick, href, component, variant) on the underlying Item and bypass handleShowMenu; instead narrow the surface by either wiring up the unused buttonSx into the Item/Button for styling (e.g., forward Sx only) or replace buttonProps with a whitelist of non-behavioral props (e.g., sx, size, color, disableElevation) and drop forwarding of onClick/href/component/variant; update the ShareItemProps type and all places passing buttonProps (including DoneScreen and the call sites around lines 70–78 and 127–132) to use the new narrowed prop name or buttonSx so behavioral handlers remain controlled by handleShowMenu and handleKeepMounted.apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx (3)
235-246: Consider using a single Typography with responsive styling.The pattern of two Typography components with opposite
displaybreakpoints is repeated for both "Select a language" and "Select a team" labels. A single component with responsivevariantortypographysx prop could reduce duplication.Example consolidation
- <Typography - variant="h6" - display={{ xs: 'none', sm: 'block' }} - > - {t('Select a language')} - </Typography> - <Typography - variant="body2" - display={{ xs: 'block', sm: 'none' }} - > - {t('Select a language')} - </Typography> + <Typography + sx={{ + typography: { xs: 'body2', sm: 'h6' } + }} + > + {t('Select a language')} + </Typography>Same pattern applies to "Select a team" labels at lines 260-274.
🤖 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 235 - 246, Replace the duplicated twin Typography components in LanguageScreen that render t('Select a language') (and the similar pair for t('Select a team')) with a single Typography that uses responsive props (variant or sx.typography/display) to adjust presentation per breakpoint; locate the two paired blocks that currently toggle display with display={{ xs:'none', sm:'block' }} and display={{ xs:'block', sm:'none' }} and consolidate them into one Typography that applies responsive styling so the text and visual weight change correctly across breakpoints while removing the duplicate element.
28-28: Import ordering: MoveBoximport to group with other MUI imports.The
Boximport is placed after local imports, breaking the convention of grouping external library imports before relative imports.Suggested fix
Move the import to the MUI import group at the top:
import FormControl from '@mui/material/FormControl' import Stack from '@mui/material/Stack' import Typography from '@mui/material/Typography' +import Box from '@mui/material/Box' import { Form, Formik, FormikValues } from 'formik'And remove line 28.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx` at line 28, Move the stray "Box" import into the existing MUI import group at the top of LanguageScreen.tsx so all Material-UI imports are grouped together (e.g., alongside other `@mui/`* imports used by the LanguageScreen component); remove the current import statement at line 28 so there is no duplicate or out-of-order import for Box.
227-227: Avoid inlinestyleattribute; use MUI'ssxprop instead.Per coding guidelines, avoid CSS/style tags. Formik's
Formdoesn't supportsx, so wrap it in aBoxor apply the width to an inner MUI component.Suggested fix
- <Form style={{ width: '100%' }}> + <Form> <FormControl sx={{ - width: { xs: '100%' }, + width: '100%', alignSelf: 'center' }} >Alternatively, wrap in a Box:
- <Form style={{ width: '100%' }}> + <Box component={Form} sx={{ width: '100%' }}> ... - </Form> + </Box>As per coding guidelines: "Always use MUI over HTML elements; avoid using CSS or tags."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx` at line 227, The Formik Form in LanguageScreen currently uses an inline style (Form style={{ width: '100%' }}); remove this inline style and instead wrap the Form in a MUI Box (import Box from `@mui/material`) and apply the layout via Box's sx (e.g., <Box sx={{ width: '100%' }}>), or move the width into an inner MUI component (Box/Stack/Container) so Form remains plain Formik's Form without style props; update imports in LanguageScreen accordingly and delete the inline style attribute.
🤖 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/LinksScreen/LinksScreen.tsx`:
- Around line 218-234: The phone validators currently allow a calling code alone
because valid-local returns true for empty strings; update both validators to
reference sibling fields (use a non-arrow function so you can access
this.parent) and enforce "both blank or both filled": in the `${link.id}__cc`
test (id `valid-cc`) allow empty only when the corresponding `${link.id}__local`
is also empty, otherwise validate the normalized calling code against
`countries`; in the `${link.id}__local` test (id `valid-local`) allow empty only
when the corresponding `${link.id}__cc` is also empty, but if `${link.id}__cc`
is populated require the local value to contain digits (e.g., the existing
/^[0-9\s\-()]+$/ check) and not be blank. Ensure you switch arrow tests to
function() to access this.parent and use `${link.id}__cc`/`${link.id}__local`
keys to read sibling values.
---
Outside diff comments:
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.tsx`:
- Around line 138-153: The change removed the manual-editor "escape hatch" by
wrapping the stepper and screen rendering in a feature-check conditional, which
hides the manual-editor route when
hasEditableText/hasCustomizableLinks/hasCustomizableMedia are false; update
MultiStepForm so the manual-editor escape hatch (the link/button that navigates
to the manual editor) is rendered even when those feature flags are false—either
render the escape hatch unconditionally alongside renderScreen(activeScreen,
handleNext) or explicitly render it when activeScreen corresponds to DoneScreen;
modify MultiStepForm to add that unconditional/DoneScreen-specific render
(referencing MultiStepForm, renderScreen, activeScreen, handleNext, screens,
DoneScreen and ProgressStepper) so the manual editor route is preserved.
---
Nitpick comments:
In
`@apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.tsx`:
- Around line 48-57: The ShareItem currently exposes a full buttonProps prop
which allows callers to override behavioral props (onClick, href, component,
variant) on the underlying Item and bypass handleShowMenu; instead narrow the
surface by either wiring up the unused buttonSx into the Item/Button for styling
(e.g., forward Sx only) or replace buttonProps with a whitelist of
non-behavioral props (e.g., sx, size, color, disableElevation) and drop
forwarding of onClick/href/component/variant; update the ShareItemProps type and
all places passing buttonProps (including DoneScreen and the call sites around
lines 70–78 and 127–132) to use the new narrowed prop name or buttonSx so
behavioral handlers remain controlled by handleShowMenu and handleKeepMounted.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.spec.tsx`:
- Around line 134-141: The test mock for DoneScreen contains a stale navigation
button (data-testid "done-screen-go-to-language") that no longer matches the
component's prop-less API; remove the button element from the DoneScreen mock so
the JSX for DoneScreen only renders the container (data-testid "done-screen")
and its static content (e.g., the <h2>), ensuring tests no longer reference the
obsolete "done-screen-go-to-language" control.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx`:
- Around line 235-246: Replace the duplicated twin Typography components in
LanguageScreen that render t('Select a language') (and the similar pair for
t('Select a team')) with a single Typography that uses responsive props (variant
or sx.typography/display) to adjust presentation per breakpoint; locate the two
paired blocks that currently toggle display with display={{ xs:'none',
sm:'block' }} and display={{ xs:'block', sm:'none' }} and consolidate them into
one Typography that applies responsive styling so the text and visual weight
change correctly across breakpoints while removing the duplicate element.
- Line 28: Move the stray "Box" import into the existing MUI import group at the
top of LanguageScreen.tsx so all Material-UI imports are grouped together (e.g.,
alongside other `@mui/`* imports used by the LanguageScreen component); remove the
current import statement at line 28 so there is no duplicate or out-of-order
import for Box.
- Line 227: The Formik Form in LanguageScreen currently uses an inline style
(Form style={{ width: '100%' }}); remove this inline style and instead wrap the
Form in a MUI Box (import Box from `@mui/material`) and apply the layout via Box's
sx (e.g., <Box sx={{ width: '100%' }}>), or move the width into an inner MUI
component (Box/Stack/Container) so Form remains plain Formik's Form without
style props; update imports in LanguageScreen accordingly and delete the inline
style attribute.
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/ScreenWrapper/ScreenWrapper.tsx`:
- Around line 41-73: The two heading/subtitle Typography nodes render
simultaneously (one hidden via CSS) causing duplicate DOM; in ScreenWrapper
replace the duplicated Typography blocks with a single responsive render (use
useMediaQuery or MUI Hidden) so only one text node exists: determine isSmUp =
useMediaQuery(theme.breakpoints.up('sm')) and render one Typography for the
title where variant = isSmUp ? 'h3' : 'h6' and content = isSmUp ? title :
(mobileTitle ?? title), and likewise render one Typography for the subtitle with
display and content chosen from subtitle/mobileSubtitle; alternatively, if you
must keep both nodes, add explicit data-testid or aria-hidden on the hidden copy
to avoid duplicate-test fallout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 62fdfb46-bd49-4c84-9a95-075e9c07b3f9
📒 Files selected for processing (22)
apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.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.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LinksScreen/CardsPreview/CardsPreview.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LinksScreen/CardsPreview/index.tsapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LinksScreen/LinksScreen.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LinksScreen/LinksScreen.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/MediaScreen/MediaScreen.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/MediaScreen/MediaScreen.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/MediaScreen/Sections/CardsSection/CardsSection.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/ScreenWrapper/ScreenWrapper.spec.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.tsxlibs/journeys/ui/src/components/TemplateView/TemplatePreviewTabs/TemplateCardPreview/TemplateCardPreview.tsxlibs/journeys/ui/src/components/TemplateView/TemplatePreviewTabs/TemplateCardPreview/index.tslibs/journeys/ui/src/components/TemplateView/TemplatePreviewTabs/TemplateCardPreview/templateCardPreviewConfig.ts
💤 Files with no reviewable changes (2)
- apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.spec.tsx
- apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LinksScreen/LinksScreen.spec.tsx
...admin/src/components/TemplateCustomization/MultiStepForm/Screens/LinksScreen/LinksScreen.tsx
Outdated
Show resolved
Hide resolved
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/TextScreen/TextScreen.spec.tsx (1)
109-133:⚠️ Potential issue | 🟡 MinorAssert
handleNextin the save path.Lines 129-130 say navigation should happen after the mutation, but the test only checks that the mutation mock ran. Since
handleNextis now the only navigation callback onTextScreen, a regression there would still pass this spec.🧪 Suggested assertion
await waitFor(() => - expect(journeyCustomizationFieldUpdate.result).toHaveBeenCalled() + expect(journeyCustomizationFieldUpdate.result).toHaveBeenCalled() ) + expect(handleNext).toHaveBeenCalled()🤖 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/TextScreen/TextScreen.spec.tsx` around lines 109 - 133, The test currently only asserts the mutation mock (journeyCustomizationFieldUpdate) ran but not that the component navigated; update the spec in TextScreen.spec.tsx to also assert handleNext was invoked after the mutation resolves by adding an await waitFor(() => expect(handleNext).toHaveBeenCalled()) (or equivalent) after the existing waitFor for journeyCustomizationFieldUpdate; ensure you reference the existing TextScreen render with the mocked provider and the handleNext jest.fn so the assertion targets that function.
♻️ Duplicate comments (1)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LinksScreen/LinksScreen.tsx (1)
217-233:⚠️ Potential issue | 🟠 MajorPhone validation still allows partial submissions and blocks clears.
Line 221 rejects a blank calling code, but Lines 229-232 treat a blank local number as valid. That still lets users save just the country code (
+1,+64, etc.), and it also makes “clear both fields” impossible even thoughhandleFormSubmitnormalizes both blank parts to''. These two inputs need paired validation so they are either both empty or both populated.🤖 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/LinksScreen/LinksScreen.tsx` around lines 217 - 233, The two validators for acc[`${link.id}__cc`] and acc[`${link.id}__local`] must be paired so both fields are either empty or both populated: update the string().test for each field to consult the sibling value (via the validation context/parent) using the same keys `${link.id}__cc` and `${link.id}__local` and enforce: if both are blank -> valid; if one is blank and the other populated -> invalid; if both populated -> validate the calling code format (normalize with leading '+') and the local number regex. Ensure the tests mirror each other so clearing both passes and partial submissions are rejected.
🧹 Nitpick comments (2)
apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx (2)
228-234: Simplify single-breakpoint responsive values.The responsive objects
{ xs: '100%' }and{ xs: 0 }can be simplified since no other breakpoints override them.♻️ Suggested simplification
<FormControl sx={{ - width: { xs: '100%' }, + width: '100%', alignSelf: 'center' }} > - <Stack gap={2} sx={{ px: { xs: 0 } }}> + <Stack gap={2}>🤖 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 228 - 234, The sx props on FormControl and Stack use single-breakpoint objects that can be simplified: in the FormControl component replace width: { xs: '100%' } with width: '100%', and in the Stack replace px: { xs: 0 } with px: 0; update the JSX in LanguageScreen where the FormControl and Stack components are defined to use these simplified values to remove unnecessary responsive objects.
146-177: Consider adding an early return or explicit handling for the unsigned-in branch.When
shouldSkipDuplicatereturns false and!isSignedIn, the function sets loading but performs no action. While the button is disabled for unsigned users (line 199), adding an explicit early return improves clarity and guards against future changes that might enable the button.♻️ Suggested improvement
async function handleSubmit(values: FormikValues) { if (journey == null) return + if (!isSignedIn) return const { teamSelect: teamId } = values🤖 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 146 - 177, handleSubmit currently sets loading then does nothing when shouldSkipDuplicate is false and isSignedIn is false; add an explicit branch after the shouldSkipDuplicate check to handle the unsigned case (e.g. if (!isSignedIn) { setLoading(false); return; }) so the function returns early and clears the loading state instead of leaving the spinner active—update the logic in handleSubmit, referencing shouldSkipDuplicate, isSignedIn, setLoading and handleNext.
🤖 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/LinksScreen/LinksScreen.tsx`:
- Around line 240-243: The Formik props include an unsupported validateOnSubmit
usage in the Formik wrapper inside LinksScreen.tsx; remove
validateOnSubmit={false} and instead set validateOnChange={false} and
validateOnBlur={false} on the same Formik component so validation is disabled on
change/blur but still runs on submit (leave onSubmit={handleFormSubmit} and
validateOnMount as-is).
---
Outside diff comments:
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/TextScreen/TextScreen.spec.tsx`:
- Around line 109-133: The test currently only asserts the mutation mock
(journeyCustomizationFieldUpdate) ran but not that the component navigated;
update the spec in TextScreen.spec.tsx to also assert handleNext was invoked
after the mutation resolves by adding an await waitFor(() =>
expect(handleNext).toHaveBeenCalled()) (or equivalent) after the existing
waitFor for journeyCustomizationFieldUpdate; ensure you reference the existing
TextScreen render with the mocked provider and the handleNext jest.fn so the
assertion targets that function.
---
Duplicate comments:
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LinksScreen/LinksScreen.tsx`:
- Around line 217-233: The two validators for acc[`${link.id}__cc`] and
acc[`${link.id}__local`] must be paired so both fields are either empty or both
populated: update the string().test for each field to consult the sibling value
(via the validation context/parent) using the same keys `${link.id}__cc` and
`${link.id}__local` and enforce: if both are blank -> valid; if one is blank and
the other populated -> invalid; if both populated -> validate the calling code
format (normalize with leading '+') and the local number regex. Ensure the tests
mirror each other so clearing both passes and partial submissions are rejected.
---
Nitpick comments:
In
`@apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsx`:
- Around line 228-234: The sx props on FormControl and Stack use
single-breakpoint objects that can be simplified: in the FormControl component
replace width: { xs: '100%' } with width: '100%', and in the Stack replace px: {
xs: 0 } with px: 0; update the JSX in LanguageScreen where the FormControl and
Stack components are defined to use these simplified values to remove
unnecessary responsive objects.
- Around line 146-177: handleSubmit currently sets loading then does nothing
when shouldSkipDuplicate is false and isSignedIn is false; add an explicit
branch after the shouldSkipDuplicate check to handle the unsigned case (e.g. if
(!isSignedIn) { setLoading(false); return; }) so the function returns early and
clears the loading state instead of leaving the spinner active—update the logic
in handleSubmit, referencing shouldSkipDuplicate, isSignedIn, setLoading and
handleNext.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d9949fc5-bf2d-4169-9e20-00c2fca0cd56
📒 Files selected for processing (16)
apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/MultiStepForm.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LinksScreen/LinksScreen.spec.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/MediaScreen/Sections/CardsSection/CardsSection.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/SocialScreen/SocialScreen.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/SocialScreen/SocialScreen.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/TextScreen/TextScreen.spec.tsxapps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/TextScreen/TextScreen.tsxlibs/journeys/ui/src/components/TemplateView/TemplatePreviewTabs/TemplateCardPreview/TemplateCardPreview.tsxlibs/locales/en/apps-journeys-admin.jsonlibs/locales/en/journeys-ui.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/MediaScreen/Sections/CardsSection/CardsSection.tsx
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.tsx
- libs/journeys/ui/src/components/TemplateView/TemplatePreviewTabs/TemplateCardPreview/TemplateCardPreview.tsx
- apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LanguageScreen/LanguageScreen.spec.tsx
Comment on lines
+240
to
+243
| validateOnSubmit={false} | ||
| onSubmit={handleFormSubmit} | ||
| validateOnMount | ||
| > |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/journeys-admin/src/components/TemplateCustomization/MultiStepForm/Screens/LinksScreen/LinksScreen.tsx | sed -n '235,250p'Repository: JesusFilm/core
Length of output: 652
🏁 Script executed:
find . -name "package.json" -type f | head -5 | xargs grep -l "formik" 2>/dev/null | head -3Repository: JesusFilm/core
Length of output: 72
🏁 Script executed:
cat package.json | grep -A 2 '"formik"'Repository: JesusFilm/core
Length of output: 132
🏁 Script executed:
find node_modules/formik -name "*.d.ts" -type f | head -5Repository: JesusFilm/core
Length of output: 112
🏁 Script executed:
grep -r "validateOnSubmit" node_modules/formik --include="*.d.ts" --include="*.js" 2>/dev/null | head -20Repository: JesusFilm/core
Length of output: 40
🏁 Script executed:
web_search
Formik 2.4.4 validateOnSubmit prop documentationRepository: JesusFilm/core
Length of output: 153
🌐 Web query:
Formik 2.4.4 validateOnSubmit prop API documentation
💡 Result:
Formik 2.4.4 does not have a validateOnSubmit prop. The form-level validation timing knobs in the FormikSharedConfig/FormikConfig API are:
validateOnChange?: booleanvalidateOnBlur?: booleanvalidateOnMount?: boolean
[1][2]
A search of the Formik API/type docs shows no validateOnSubmit entry (i.e., it’s not a supported config prop) [3]. If your goal is “validate only when the user submits”, the typical approach is:
<Formik validateOnChange={false} validateOnBlur={false} ... />(Submission-time validation still occurs when you call/trigger submitForm/handleSubmit.) [1][2]
Sources: [1] [2] [3]
🏁 Script executed:
grep -r "validateOnSubmit" . --include="*.tsx" --include="*.ts" --include="*.jsx" --include="*.js" 2>/dev/null | head -20Repository: JesusFilm/core
Length of output: 197
Remove unsupported validateOnSubmit config.
validateOnSubmit is not a documented Formik prop in version 2.4.4. Formik's <Formik /> component only supports validateOnChange, validateOnBlur, and validateOnMount. The validateOnSubmit={false} at line 240 will be ignored. To disable validation on change/blur while keeping submit-time validation, use validateOnChange={false} and validateOnBlur={false} instead.
🤖 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/LinksScreen/LinksScreen.tsx`
around lines 240 - 243, The Formik props include an unsupported validateOnSubmit
usage in the Formik wrapper inside LinksScreen.tsx; remove
validateOnSubmit={false} and instead set validateOnChange={false} and
validateOnBlur={false} on the same Formik component so validation is disabled on
change/blur but still runs on submit (leave onSubmit={handleFormSubmit} and
validateOnMount as-is).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements