feat: Improved Outbound message's modal scrolling experience #36974
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 54b2026 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughUpdates layout and scrolling for the Outbound Message modal and wizard: replaces many divs with Box, adds a shared OutboundMessageForm and Scrollable wrappers, forwards Box props across several components, and extends OutboundMessagePreview with providerType and Box prop forwarding. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #36974 +/- ##
===========================================
+ Coverage 65.88% 67.32% +1.44%
===========================================
Files 3314 3339 +25
Lines 114127 113204 -923
Branches 20832 20531 -301
===========================================
+ Hits 75187 76211 +1024
+ Misses 36391 34383 -2008
- Partials 2549 2610 +61
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (6)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm.tsx (2)
123-196: Make Scrollable a flexible child to avoid layout edge cases.To prevent double scrollbars or clipped content in some flex parents, let Scrollable grow and set minHeight=0.
- <Scrollable vertical> + <Scrollable vertical flexGrow={1} minHeight={0}>
167-176: Inconsistent error prop type vs department field.Department passes a message string (errors.departmentId?.message) while Agent passes a boolean (!!errors.agentId). Aligning helps UX/types unless AutoCompleteAgent intentionally differs.
If Agent also accepts a message, consider:
- error={!!errors.agentId} + error={errors.agentId?.message}apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx (1)
181-209: Let the Scrollable grow to fill vertical space.Prevents rare clipping in nested flex layouts.
- <Scrollable vertical> + <Scrollable vertical flexGrow={1} minHeight={0}>apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx (1)
102-144: Ensure Scrollable participates in flex sizing.Same rationale as other forms.
- <Scrollable vertical> + <Scrollable vertical flexGrow={1} minHeight={0}>apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/ReviewStep.tsx (1)
19-23: Avoid hard-coded preview height.Consider a viewport-relative cap for better small/large screen behavior.
- <Scrollable vertical> - <OutboundMessagePreview {...props} maxHeight={500} /> + <Scrollable vertical maxHeight='60vh'> + <OutboundMessagePreview {...props} />apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessagePreview/OutboundMessagePreview.tsx (1)
73-74: Optional a11y: give the section an accessible name.Adding aria-label or aria-labelledby helps screen readers.
- <Box {...props} is='section'> + <Box {...props} is='section' aria-label='Outbound message preview'>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/__snapshots__/MessageStep.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/__snapshots__/RecipientStep.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/__snapshots__/RepliesStep.spec.tsx.snapis excluded by!**/*.snappackages/ui-client/src/components/Wizard/__snapshots__/Wizard.spec.tsx.snapis excluded by!**/*.snappackages/ui-client/src/components/Wizard/__snapshots__/WizardActions.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
.changeset/witty-impalas-flow.md(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessagePreview/OutboundMessagePreview.tsx(5 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageForm.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx(4 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm.tsx(4 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/MessageStep.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RecipientStep.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RepliesStep.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/ReviewStep.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx(1 hunks)packages/ui-client/src/components/Wizard/Wizard.tsx(1 hunks)packages/ui-client/src/components/Wizard/WizardContent.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
apps/meteor/tests/e2e/page-objects/modal.ts (1)
Modal(3-25)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx (2)
apps/meteor/app/utils/lib/i18n.ts (1)
t(6-6)apps/meteor/client/components/Omnichannel/OutboundMessage/constants.ts (1)
OUTBOUND_DOCS_LINK(1-1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageForm.tsx (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/hooks/useFormKeyboardSubmit.tsx (1)
useFormKeyboardSubmit(14-24)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm.tsx (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/utils/cx.ts (1)
cxp(29-34)
packages/ui-client/src/components/Wizard/Wizard.tsx (1)
packages/ui-client/src/components/Wizard/WizardContext.tsx (2)
WizardAPI(7-15)WizardContext(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (13)
.changeset/witty-impalas-flow.md (1)
1-6: Changeset reads well and matches the PR scope.Scope and messaging look accurate.
packages/ui-client/src/components/Wizard/WizardContent.tsx (1)
13-18: Wrapper removal: double-check styling/ARIA assumptions.Returning children directly removes a DOM node; verify no selectors or layout rules relied on a wrapper element.
Run quick checks:
- Inspect step containers to ensure margins/paddings still apply.
- Verify focus trapping/tab order unchanged.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/MessageStep.tsx (1)
21-32: Wrapper removal LGTM.Lean DOM and submit flow with WizardNextButton manual looks correct.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RecipientStep.tsx (1)
22-31: LGTM: direct form return aligns with new layout.Actions rendering and submit progression remain intact.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RepliesStep.tsx (1)
21-30: LGTM: flatter structure, unchanged behavior.Back/Next and manual submit wiring look good.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.tsx (1)
150-154: Flex container + minHeight=0 is the right combo for scrollables.Good fix for overflow on small viewports.
Manual checks:
- 320x568 and 375x667: confirm inner Scrollable receives focus and wheel/touch scroll works.
- Long template on Step 2: Next/Back visible after scroll; keyboard Cmd/Ctrl+Enter submits.
packages/ui-client/src/components/Wizard/Wizard.tsx (1)
12-15: Forward ref through Wizard to the root Box.Repo search found no occurrences or existing ref usage; forwarding the ref is safe for internal code — confirm external consumers won't rely on the current signature.
-import { ComponentProps, memo, type ReactNode } from 'react'; +import { ComponentProps, memo, forwardRef, type ReactNode } from 'react'; @@ -const Wizard = ({ children, api, ...props }: WizardProps) => ( - <WizardContext.Provider value={api}> - <Box {...props}>{children}</Box> - </WizardContext.Provider> -); - -export default memo(Wizard); +const Wizard = forwardRef<HTMLDivElement, WizardProps>(({ children, api, ...props }, ref) => ( + <WizardContext.Provider value={api}> + <Box ref={ref} {...props}>{children}</Box> + </WizardContext.Provider> +)); + +export default memo(Wizard);apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (2)
49-49: Confirm focus/ARIA behavior when confirmation is open.Hiding the underlying Modal via display='none' is fine; please also ensure it’s removed from the a11y tree and doesn’t trap focus. If needed, add aria-hidden when isClosing is true.
Apply if required:
- <Modal aria-labelledby={modalId} display={isClosing ? 'none' : undefined}> + <Modal aria-labelledby={modalId} display={isClosing ? 'none' : undefined} aria-hidden={isClosing || undefined}>
55-57: Height=100% on content helps Scrollable sizing.Looks good. Please sanity-check on small viewports that inner Scrollable areas actually scroll and the modal itself doesn’t get a second scrollbar.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx (1)
179-181: Shared Form wrapper adoption looks good.Consistent handling across steps.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessagePreview/OutboundMessagePreview.tsx (1)
18-29: LGTM: Box props forwarding increases composability.Accepting ComponentProps is a nice touch for layout control.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm.tsx (1)
and forwards noValidate/onSubmit.
121-126: Approve — OutboundMessageForm renders a native
Box is='form'; onSubmit is passed explicitly and {...props} is spread (so noValidate will be forwarded).apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx (1)
130-134: Update OUTBOUND_DOCS_LINK to the canonical Rocket.Chat WhatsApp template docs.Set OUTBOUND_DOCS_LINK to https://developer.rocket.chat/docs/whatsapp-business-template-messages, remove the TODO, and confirm the change in apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx (lines 130–134).
...ichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageForm.tsx
Outdated
Show resolved
Hide resolved
...nichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageForm.tsx (1)
2-6: Fix onSubmit typing and wire Cmd/Ctrl+Enter to native form submission.
- onSubmit: () => void is incompatible with standard FormEvent handlers used by RHF and native forms.
- Add a keyboard shortcut that calls requestSubmit() so validations fire consistently.
- Prevent overriding is; this component should always render a form.
Apply:
-import { Box } from '@rocket.chat/fuselage'; -import type { ComponentProps } from 'react'; +import { Box } from '@rocket.chat/fuselage'; +import { useCallback, type ComponentProps, type FormEventHandler, type KeyboardEvent } from 'react'; -type OutboundMessageFormProps = ComponentProps<typeof Box> & { - onSubmit?: () => void; -}; +type OutboundMessageFormProps = Omit<ComponentProps<typeof Box>, 'is' | 'onSubmit' | 'onKeyDown'> & { + onSubmit?: FormEventHandler<HTMLFormElement>; + onKeyDown?: (e: KeyboardEvent<HTMLFormElement>) => void; +}; -const OutboundMessageForm = ({ onSubmit, ...props }: OutboundMessageFormProps) => { - return <Box is='form' display='flex' flexDirection='column' height='100%' flexGrow={1} flexShrink={0} onSubmit={onSubmit} {...props} />; -}; +const OutboundMessageForm = ({ onSubmit, onKeyDown, ...props }: OutboundMessageFormProps) => { + const handleKeyDown = useCallback( + (e: KeyboardEvent<HTMLFormElement>) => { + onKeyDown?.(e); + if (!e.isDefaultPrevented() && (e.metaKey || e.ctrlKey) && e.key === 'Enter') { + e.preventDefault(); + e.currentTarget.requestSubmit(); + } + }, + [onKeyDown], + ); + + return ( + <Box + is='form' + display='flex' + flexDirection='column' + height='100%' + flexGrow={1} + flexShrink={0} + onKeyDown={handleKeyDown} + onSubmit={onSubmit} + {...props} + /> + ); +};Run to confirm there are no remaining custom keyboard-submit handlers in the wizard forms (to avoid double submissions):
#!/bin/bash rg -nP --type=tsx -C2 '\bonKeyDown\b|\buseFormKeyboardSubmit\b' apps/meteor/client/components/Omnichannel/OutboundMessageAlso applies to: 8-10
🧹 Nitpick comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageForm.tsx (1)
9-9: Allow nested Scrollable to size correctly inside flex containers.Add minHeight={0} to avoid flex children preventing overflow-based scrolling in some layouts.
- return <Box is='form' display='flex' flexDirection='column' height='100%' flexGrow={1} flexShrink={0} onSubmit={onSubmit} {...props} />; + return <Box is='form' display='flex' flexDirection='column' height='100%' minHeight={0} flexGrow={1} flexShrink={0} onSubmit={onSubmit} {...props} />;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageForm.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
tassoevan
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
kodiakhq
bot
removed
the
stat: ready to merge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
aleksandernsilva
force-pushed
the
feat/outbound-scroll-improvements
branch
from
dbe5f4d to
fd5e18f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessagePreview/OutboundMessagePreview.tsx (3)
18-29: Avoid acceptingchildrenhere to prevent shadowing/ignoring passed childrenBecause you also provide explicit children to
<Box>, any user-suppliedchildrenin...propswould be ignored. Tighten the prop type to omitchildrenand rename the spread to clarify intent.-type OutboundMessagePreviewProps = ComponentProps<typeof Box> & { +type OutboundMessagePreviewProps = Omit<ComponentProps<typeof Box>, 'children'> & { @@ - templateParameters, - ...props + templateParameters, + ...boxProps }: OutboundMessagePreviewProps) => { @@ - <Box {...props} is='section'> + <Box {...boxProps} is='section'>Also applies to: 54-55
31-38: Phone formatting condition likely too narrow
providerType === 'phone'may miss SMS/WhatsApp-like providers that also use phone numbers. Consider a small allowlist to trigger phone formatting.- if (providerType === 'phone') { + if (providerType === 'phone' || providerType === 'sms' || providerType === 'whatsapp') { return formatPhoneNumber(rawValue); }If the enum differs, align with the actual provider types used in
IOutboundProviderMetadata.
73-74: Minor a11y: label the sectionAdd an accessible name to the
<section>so screen readers announce it meaningfully in the Review step.- <Box {...boxProps} is='section'> + <Box {...boxProps} is='section' aria-label='Outbound message preview'>apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RepliesStep.tsx (2)
15-18: Remove unnecessaryasync
onSubmitis typed as synchronous. Droppingasyncavoids confusion and micro overhead.- const handleSubmit = useEffectEvent(async (values: RepliesFormSubmitPayload) => { + const handleSubmit = useEffectEvent((values: RepliesFormSubmitPayload) => { onSubmit(values); next(); });
25-28: Disable Back while submittingPrevents navigation during an active submit.
- <WizardBackButton /> + <WizardBackButton disabled={isSubmitting} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/__snapshots__/MessageStep.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/__snapshots__/RecipientStep.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/__snapshots__/RepliesStep.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/__snapshots__/ReviewStep.spec.tsx.snapis excluded by!**/*.snappackages/ui-client/src/components/Wizard/__snapshots__/Wizard.spec.tsx.snapis excluded by!**/*.snappackages/ui-client/src/components/Wizard/__snapshots__/WizardActions.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
.changeset/witty-impalas-flow.md(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessagePreview/OutboundMessagePreview.tsx(5 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageForm.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx(4 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx(4 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/MessageStep.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RecipientStep.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RepliesStep.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/ReviewStep.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx(1 hunks)packages/ui-client/src/components/Wizard/Wizard.tsx(1 hunks)packages/ui-client/src/components/Wizard/WizardContent.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageForm.tsx
- packages/ui-client/src/components/Wizard/WizardContent.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx
- .changeset/witty-impalas-flow.md
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/MessageStep.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/ReviewStep.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx
- packages/ui-client/src/components/Wizard/Wizard.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RecipientStep.tsx (1)
22-31: LGTM — wrapper removal + manual submit flow look rightQuick sanity-check: ensure RecipientForm renders
renderActionsinside the actual<form>so thetype='submit'Next button submits the form. If not, passformattr pointing to the form id (pattern used in RepliesForm).apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx (1)
119-140: Scrollable placement is good; verify container allows it to actually scrollThis looks correct (fields inside Scrollable; actions outside). Ensure the parent
Formsets a flex column layout withminHeight: 0andoverflow: hidden(or equivalent) so the inner<Scrollable vertical>can gain height and scroll on small viewports. If not, addflexGrow={1}to a wrapper aroundScrollableor set these styles onForm.Also applies to: 148-148
aleksandernsilva
force-pushed
the
feat/outbound-scroll-improvements
branch
from
fd5e18f to
e937bb5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/DepartmentField.tsx (1)
14-21: Avoid prop-type collision on onChange.
ComponentProps<typeof Field> & { onChange: () => void }risks an incompatible intersection ifFieldexposesonChange. MakeonChangeyour own prop by omitting it fromField’s props.Apply:
-type DepartmentFieldProps = ComponentProps<typeof Field> & { +type DepartmentFieldProps = Omit<ComponentProps<typeof Field>, 'onChange'> & { control: Control<RepliesFormData>; onlyMyDepartments?: boolean; isError: boolean; isFetching: boolean; onRefetch: () => void; onChange: () => void; };Also applies to: 30-31
♻️ Duplicate comments (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageForm.tsx (2)
4-6: Fix onSubmit typing (conflicts with Box’s native signature).
ComponentProps<typeof Box> & { onSubmit?: () => void }creates an unsatisfiable intersection foronSubmitand will reject standardFormEventhandlers.Apply:
-import type { ComponentProps } from 'react'; +import { useCallback, useRef, type ComponentProps } from 'react'; -type OutboundMessageFormProps = ComponentProps<typeof Box> & { - onSubmit?: () => void; -}; +type OutboundMessageFormProps = Omit<ComponentProps<typeof Box>, 'onSubmit' | 'is' | 'ref'> & { + onSubmit?: ComponentProps<typeof Box>['onSubmit']; +};
8-10: Add Cmd/Ctrl+Enter submit and harden invariants (always a form).The PR description says this component manages the submit shortcut; current code doesn’t. Also prevent consumers from overriding
is='form'.Apply:
-const OutboundMessageForm = ({ onSubmit, ...props }: OutboundMessageFormProps) => ( - <Box is='form' display='flex' flexDirection='column' height='100%' flexGrow={1} flexShrink={0} onSubmit={onSubmit} {...props} /> -); +// NOTE: adjust the import path of useFormKeyboardSubmit if different in this repo +// import { useFormKeyboardSubmit } from '.../useFormKeyboardSubmit'; +const OutboundMessageForm = ({ onSubmit, ...props }: OutboundMessageFormProps) => { + const nodeRef = useRef<HTMLFormElement | null>(null); + const kbRef = useFormKeyboardSubmit(() => nodeRef.current?.requestSubmit(), []); + const combinedRef = useCallback((el: HTMLFormElement | null) => { + nodeRef.current = el; + kbRef(el); + }, [kbRef]); + + return ( + <Box + {...props} + is='form' + display='flex' + flexDirection='column' + height='100%' + flexGrow={1} + flexShrink={0} + ref={combinedRef} + onSubmit={onSubmit} + /> + ); +};Run to locate the hook and confirm the import path:
#!/bin/bash rg -nC2 '\buseFormKeyboardSubmit\b'
🧹 Nitpick comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RecipientStep.tsx (1)
26-29: Option: Disable Next when the form is invalid.If RecipientForm exposes validity (e.g., isValid or canSubmit) to renderActions, disable the submit to prevent no-op submissions and improve UX.
Apply if supported by renderActions’ signature:
- renderActions={({ isSubmitting }) => ( + renderActions={({ isSubmitting, isValid }) => ( <WizardActions> - <WizardNextButton manual type='submit' loading={isSubmitting} /> + <WizardNextButton manual type='submit' loading={isSubmitting} disabled={!isValid} /> </WizardActions> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/__snapshots__/MessageStep.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/__snapshots__/RecipientStep.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/__snapshots__/RepliesStep.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/__snapshots__/ReviewStep.spec.tsx.snapis excluded by!**/*.snappackages/ui-client/src/components/Wizard/__snapshots__/Wizard.spec.tsx.snapis excluded by!**/*.snappackages/ui-client/src/components/Wizard/__snapshots__/WizardActions.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
.changeset/witty-impalas-flow.md(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessagePreview/OutboundMessagePreview.tsx(5 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageForm.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx(4 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/components/TemplateField.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx(4 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/DepartmentField.tsx(4 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/MessageStep.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RecipientStep.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RepliesStep.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/ReviewStep.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- .changeset/witty-impalas-flow.md
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/MessageStep.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RepliesStep.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/ReviewStep.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessagePreview/OutboundMessagePreview.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T17:32:33.969Z
Learnt from: aleksandernsilva
PR: RocketChat/Rocket.Chat#36974
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx:124-129
Timestamp: 2025-09-18T17:32:33.969Z
Learning: The ARIA mismatch issue in MessageForm's template field (where FieldError id used templateId instead of messageFormId) was addressed in PR #36972 through refactoring the template field into a separate TemplateField component, which uses consistent templateFieldId for both aria-describedby and FieldError id.
Applied to files:
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/components/TemplateField.tsx
🧬 Code graph analysis (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx (1)
RepliesFormData(19-22)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/components/TemplateField.tsx (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx (1)
MessageFormData(18-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/components/TemplateField.tsx (1)
15-19: Prop-forwarding is correct and a11y wiring looks good.Extending
ComponentProps<typeof Field>and forwarding via<Field {...props}>is consistent; label/aria IDs are coherent withTemplateSelect.Please sanity-check that
cxp(templateFieldId, { error, hint: true })renders “-error -hint” when error exists. If it doesn’t,aria-describedbymay miss the error node.Also applies to: 21-21, 50-50
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/DepartmentField.tsx (1)
54-79: Good a11y and retry UX.
aria-labelledby,aria-describedby, and inline retry control are wired cleanly; error/hint IDs follow a consistent convention.apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx (1)
13-19: Approve — AutoCompleteDepartmentAgent default export confirmedDefault export AutoCompleteDepartmentAgent found in apps/meteor/client/components/Omnichannel/OutboundMessage/components/AutoCompleteDepartmentAgent.tsx (line 64); import name matches — no changes required.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/steps/RecipientStep.tsx (2)
22-31: Good cleanup: render the form directly (fewer wrappers).Returning RecipientForm at the top level aligns with the new scrollable Form layout and avoids unnecessary nesting.
22-31: No change required — submit button is inside the form.RecipientForm renders renderActions inside its
(OutboundMessageForm is a Box with is='form'), so the WizardNextButton (type='submit') will be inside the form and will submit as expected.
aleksandernsilva
added
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
This PR improves how the Outbound Message modal handles scrolling on smaller screens and with larger templates. The structure was simplified by removing extra wrappers, section heights were adjusted for consistency, and Scrollable was added where needed. A new OutboundMessageForm component was also introduced to manage the submit shortcut and shared styling across all outbound message forms.
Issue(s)
CTZ-340
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Chores