feat(outbound): Added closing confirmation to the modal #36937
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughIntroduces a two-stage close-confirmation flow for OutboundMessageModal with Escape/Close handling and accessibility ids, adds OutboundMessageCloseConfirmationModal component, adds unit tests for the close flow, and adds English and pt-BR i18n keys. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Modal as OutboundMessageModal
participant Wizard as OutboundMessageWizard
participant Confirm as OutboundMessageCloseConfirmationModal
rect #E6F0FF
note over Modal: Initial — wizard visible
Modal->>Wizard: render
end
User->>Modal: Click Close / press Esc
Modal->>Modal: setClosingConfirmation(true)
Wizard-->>Modal: hidden (display: none)
rect #FFF5E6
Modal->>Confirm: render confirmation
note over Confirm: "Keep editing" / "Discard"
end
alt User cancels
User->>Confirm: Click Keep editing / press Esc
Confirm-->>Modal: onCancel
Modal->>Modal: setClosingConfirmation(false)
Modal->>Wizard: show
else User confirms
User->>Confirm: Click Discard
Confirm-->>Modal: onConfirm
Modal-->>User: onClose()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmation.tsx (2)
5-12: Avoid % padding for vertical spacing; it’s width-relative and can look off.Percentage padding in the block axis resolves against container width, not height. Prefer fixed/scale tokens or drop it.
const containerStyle = css` display: flex; align-items: center; justify-content: center; - padding-block-end: 10%; width: 100%; height: 100%; `;
22-36: Fix a11y: remove aria-live, add labelling, and autofocus Cancel.This isn’t a live region; use aria-labelledby/aria-describedby and focus the safe action.
- <Box bg='surface-light' className={containerStyle} aria-live='assertive'> + <Box + bg='surface-light' + className={containerStyle} + role='group' + aria-labelledby='outbound-close-title' + aria-describedby='outbound-close-desc' + > <States> <StatesIcon name='warning' variation='danger' /> - <StatesTitle>{t('All_changes_will_be_lost')}</StatesTitle> - <StatesSubtitle>{t('Are_you_sure_you_want_to_discard_this_outbound_message')}</StatesSubtitle> + <StatesTitle id='outbound-close-title'>{t('All_changes_will_be_lost')}</StatesTitle> + <StatesSubtitle id='outbound-close-desc'>{t('Are_you_sure_you_want_to_discard_this_outbound_message')}</StatesSubtitle> <StatesActions> - <Button secondary onClick={onCancel}> + <Button type='button' secondary onClick={onCancel} autoFocus> {t('Cancel')} </Button> - <Button danger onClick={onConfirm}> + <Button type='button' danger onClick={onConfirm}> {t('Confirm')} </Button> </StatesActions> </States> </Box>Also applies to: 28-31
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (2)
18-18: Name clarity.Consider renaming to isCloseConfirmationOpen/setIsCloseConfirmationOpen for readability.
45-45: Double-check closing on error.Closing the modal on onError can discard user input after failures. Confirm this is intentional for this flow.
I can help gate close-on-error behind a flag or show an inline error while keeping the modal open.
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx (2)
12-18: Add missing i18n keys to avoid brittle fallback behavior.Explicitly include Confirm and Outbound_Message to decouple from global defaults.
const appRoot = mockAppRoot() .withTranslations('en', 'core', { Close: 'Close', Cancel: 'Cancel', + Confirm: 'Confirm', + Outbound_Message: 'Outbound Message', Are_you_sure_you_want_to_discard_this_outbound_message: 'Are you sure you want to discard this outbound message?', }) .build();
35-49: Add keyboard and backdrop interaction tests.Cover Esc key and ensure Cancel receives focus (with the proposed autoFocus). Optionally assert clicking the backdrop doesn’t close.
it('should close confirmation and leave modal open when cancel is clicked', async () => { const onClose = jest.fn(); render(<OutboundMessageModal onClose={onClose} />, { wrapper: appRoot }); await userEvent.click(screen.getByRole('button', { name: 'Close' })); expect(screen.getByText('Are you sure you want to discard this outbound message?')).toBeInTheDocument(); expect(screen.queryByRole('button', { name: 'Close' })).not.toBeInTheDocument(); await userEvent.click(screen.getByRole('button', { name: 'Cancel' })); expect(screen.queryByText('Are you sure you want to discard this outbound message?')).not.toBeInTheDocument(); expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); expect(onClose).not.toHaveBeenCalled(); }); +it('should open confirmation on Escape and focus Cancel', async () => { + const onClose = jest.fn(); + render(<OutboundMessageModal onClose={onClose} />, { wrapper: appRoot }); + await userEvent.keyboard('{Escape}'); + expect(screen.getByText('Are you sure you want to discard this outbound message?')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Cancel' })).toHaveFocus(); +}); + +it('should not close when clicking the backdrop', async () => { + const onClose = jest.fn(); + const { container } = render(<OutboundMessageModal onClose={onClose} />, { wrapper: appRoot }); + // Click outside the dialog; adjust selector if needed depending on Fuselage markup + await userEvent.click(container); + expect(onClose).not.toHaveBeenCalled(); + expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmation.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx(3 hunks)packages/i18n/src/locales/en.i18n.json(2 hunks)packages/i18n/src/locales/pt-BR.i18n.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
apps/meteor/tests/e2e/page-objects/modal.ts (1)
Modal(3-25)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
packages/i18n/src/locales/pt-BR.i18n.json (2)
425-425: LGTM: translation and placement look correct."All_changes_will_be_lost" → "Todas as alterações serão perdidas" is accurate and follows existing style, positioned near related "All_*" keys.
672-672: LGTM: consistent terminology with “Mensagem ativa”."Are_you_sure_you_want_to_discard_this_outbound_message" → "Tem certeza de que deseja descartar esta mensagem ativa?" matches prior usage of “Mensagem ativa” in this locale and mirrors existing confirmation phrasing.
packages/i18n/src/locales/en.i18n.json (2)
426-426: LGTM: new key for secondary warning copy is clear and consistent.
672-672: LGTM: confirmation prompt text fits existing “Are_you_sure_you_want_to_…” patterns.Please confirm:
- pt-BR locale includes matching keys.
- Tests/assertions cover both keys (including “All_changes_will_be_lost”).
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
44-46: Good call keeping the wizard mounted to preserve state.Hiding instead of unmounting avoids losing form progress when users cancel the confirmation.
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx (1)
20-33: Nice coverage of the confirmation happy path.Test clearly asserts the two‑stage flow and callback.
.../components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/outbound-msg-ui #36937 +/- ##
========================================================
+ Coverage 70.21% 70.25% +0.04%
========================================================
Files 3086 3087 +1
Lines 106092 106192 +100
Branches 18832 18848 +16
========================================================
+ Hits 74491 74607 +116
+ Misses 29632 29614 -18
- Partials 1969 1971 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
49-51: Also stop mousedown/pointerdown on the backdrop to prevent bypass via early-phase handlers.Click is covered, but handlers listening on mousedown/pointerdown can still see the event and close the modal without confirmation.
-<ModalBackdrop bg='transparent' onClick={(e) => e.stopPropagation()} onKeyDown={handleKeyDown}> +<ModalBackdrop + bg='transparent' + onClick={(e) => e.stopPropagation()} + onMouseDown={(e) => e.stopPropagation()} + onPointerDown={(e) => e.stopPropagation()} + onKeyDown={handleKeyDown} +>
🧹 Nitpick comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
22-32: Optional hardening: capture Esc at document level to guarantee interception.If the Modal library listens on document for Esc, React bubbling may not block it. Consider a capture listener with cleanup:
useEffect(() => { const onKeyDown = (e: KeyboardEvent) => { if (e.key !== 'Escape') return; e.preventDefault(); e.stopPropagation(); setClosingConfirmation((v) => !v); // toggle confirm; aligns with handler above }; document.addEventListener('keydown', onKeyDown, true); return () => document.removeEventListener('keydown', onKeyDown, true); }, []);Add/extend tests to cover: Esc opens confirmation; Esc while confirming cancels; backdrop mousedown does not close.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
apps/meteor/tests/e2e/page-objects/modal.ts (1)
Modal(3-25)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
20-20: State-driven hide of the wizard looks good.Keeping the wizard mounted (display: none) preserves unsaved state while showing confirmation. LGTM.
Please confirm the wizard has no polling/side-effects that should pause while hidden.
Also applies to: 56-58
.../components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx
Outdated
Show resolved
Hide resolved
.../components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx
Outdated
Show resolved
Hide resolved
aleksandernsilva
force-pushed
the
feat/outbound-close-confirmation
branch
from
3eddb84 to
1c385d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
49-49: Also stop mousedown on the backdrop to prevent bypasses.Covers handlers listening on mousedown instead of click; avoids accidental close.
- <ModalBackdrop bg='transparent' onClick={(e) => e.stopPropagation()} onKeyDown={handleKeyDown}> + <ModalBackdrop + bg='transparent' + onClick={(e) => e.stopPropagation()} + onMouseDown={(e) => e.stopPropagation()} + onKeyDown={handleKeyDown} + >
🧹 Nitpick comments (6)
packages/i18n/src/locales/en.i18n.json (1)
671-671: New confirm string looks good; consider keeping alphabetical order within the “Are_you_sure_you_want_to_…” group.Minor nit: most keys in this block are sorted; placing “discard … outbound message” after “reset … priorities” breaks the ordering slightly. Not required, but helps diff hygiene.
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx (3)
25-31: Use alertdialog semantics; remove aria-live; add aria-describedby.Improve a11y by using role="alertdialog", referencing the description, and dropping aria-live on static text.
const { t } = useTranslation(); - const modalId = useId(); + const modalId = useId(); + const descriptionId = useId(); return ( - <Modal aria-labelledby={modalId} onKeyDown={onKeyDown}> + <Modal role='alertdialog' aria-labelledby={modalId} aria-describedby={descriptionId} onKeyDown={onKeyDown}> <ModalHeader> <ModalTitle id={modalId}>{t('Discard_message')}</ModalTitle> </ModalHeader> <ModalContent> - <p aria-live='assertive'>{t('Are_you_sure_you_want_to_discard_this_outbound_message')}</p> + <p id={descriptionId}>{t('Are_you_sure_you_want_to_discard_this_outbound_message')}</p> </ModalContent>Also applies to: 21-23
35-37: Prefer autoFocus over imperative ref focusing.Avoid focusing on every render; let the platform handle initial focus.
- <Button ref={(el) => el?.focus()} secondary onClick={onCancel}> + <Button autoFocus secondary onClick={onCancel}> {t('Keep_editing')} </Button>
17-18: Loosen keydown handler type to HTMLElement.The underlying element may not be a div; use HTMLElement to future‑proof.
- onKeyDown?(e: KeyboardEvent<HTMLDivElement>): void; + onKeyDown?(e: KeyboardEvent<HTMLElement>): void;apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (2)
35-46: Use functional state update when toggling.Avoid stale reads; this is the canonical pattern for toggles.
- // If the confirmation is already being shown, close it. - // Otherwise, show the confirmation. - setClosingConfirmation(!isClosing); + // Toggle confirmation visibility on Esc. + setClosingConfirmation((v) => !v);
50-50: Optional: centralize key handling on the backdrop.The same handler on both Modal and Backdrop is redundant; keeping it only on the Backdrop reduces duplicate bindings.
- <Modal aria-labelledby={modalId} display={isClosing ? 'none' : 'block'} onKeyDown={handleKeyDown}> + <Modal aria-labelledby={modalId} display={isClosing ? 'none' : 'block'}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx(3 hunks)packages/i18n/src/locales/en.i18n.json(3 hunks)packages/i18n/src/locales/pt-BR.i18n.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx (1)
apps/meteor/tests/e2e/page-objects/modal.ts (1)
Modal(3-25)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
apps/meteor/tests/e2e/page-objects/modal.ts (1)
Modal(3-25)
⏰ 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). (4)
- GitHub Check: ⚙️ Variables Setup
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
packages/i18n/src/locales/pt-BR.i18n.json (2)
671-671: LGTM — consistent terminology with “Mensagem ativa”.Translation aligns with existing “Outbound_message” strings in this locale.
2665-2665: LGTM — natural button text.“Continuar editando” reads well for a CTA.
packages/i18n/src/locales/en.i18n.json (2)
1714-1714: “Discard_message” may duplicate the generic “Discard”; validate intent and casing.Automated search failed ("No files were searched"); unable to verify usages. Confirm where each key is used and decide:
- Reuse generic "Discard" to avoid translation churn.
- Or keep "Discard_message" and change label to title case ("Discard Message") for consistency.
Location: packages/i18n/src/locales/en.i18n.json — "Discard_message": "Discard message"
Run locally: git grep -n 'Discard_message' || rg -n --hidden -S 'Discard_message'
2685-2685: Confirm locales & tests for "Keep_editing"
- Found in packages/i18n/src/locales/en.i18n.json (line 2685) and packages/i18n/src/locales/pt-BR.i18n.json (line 2665).
- Absent from other locale files — add translations or confirm fallback behavior is acceptable.
- Repo search did not locate usages of "Keep_editing" or the literal "Keep editing" and no tests covering primary/fallback flows were detected; confirm the key is referenced by the UI and add unit/integration tests for both primary and fallback locales if missing.
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx (1)
24-44: LGTM overall.Clear props, proper labeling with aria-labelledby, and clean footer actions.
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
61-67: LGTM: close-confirmation flow is solid.Esc toggles confirm/cancel, Close button gates via confirmation, and wizard state is preserved via display:none.
Please confirm preserving wizard state across the confirmation (via display:none) is intentional for this UX.
aleksandernsilva
force-pushed
the
feat/outbound-close-confirmation
branch
from
3e1632e to
5735995
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (3)
5-5: TS import fix looks correct.Using a single
import type { KeyboardEvent, ComponentProps }resolves the earlier compile error.
35-46: Handle Esc auto‑repeat; use functional state update to avoid stale reads.Current toggle can flicker when Esc is held due to
e.repeat, and!isClosingrelies on closure state.Apply this diff:
-const handleKeyDown = useEffectEvent((e: KeyboardEvent<HTMLDivElement>): void => { - if (e.key !== 'Escape') { - return; - } - - e.stopPropagation(); - e.preventDefault(); - - // If the confirmation is already being shown, close it. - // Otherwise, show the confirmation. - setClosingConfirmation(!isClosing); -}); +const handleKeyDown = useEffectEvent((e: KeyboardEvent<HTMLDivElement>): void => { + if (e.key !== 'Escape') { + return; + } + e.preventDefault(); + e.stopPropagation(); + // Avoid rapid toggle when key is held down + if (e.repeat) { + return; + } + // Functional update avoids stale closure reads + setClosingConfirmation((prev) => !prev); +});
49-49: Also stop mousedown on the backdrop to prevent outside‑click close paths.Click is covered; mousedown can still trigger upstream handlers.
Apply this diff:
-<ModalBackdrop bg='transparent' onClick={(e) => e.stopPropagation()} onKeyDown={handleKeyDown}> +<ModalBackdrop + bg='transparent' + onClick={(e) => e.stopPropagation()} + onMouseDown={(e) => e.stopPropagation()} + onKeyDown={handleKeyDown} +>
🧹 Nitpick comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
50-60: Prefer unmounting the wizard modal instead ofdisplay="none"while confirming.Unmounting avoids focus‑trap/a11y quirks and reduces hidden DOM.
Apply this diff:
- <Modal aria-labelledby={modalId} display={isClosing ? 'none' : 'block'} onKeyDown={handleKeyDown}> - <ModalHeader> - <ModalTitle id={modalId}>{t('Outbound_Message')}</ModalTitle> - <ModalClose onClick={() => setClosingConfirmation(true)} /> - </ModalHeader> - - <ModalContent pbe={16}> - <OutboundMessageWizard defaultValues={defaultValues} onSuccess={onClose} onError={onClose} /> - </ModalContent> - </Modal> + {!isClosing && ( + <Modal aria-labelledby={modalId} onKeyDown={handleKeyDown}> + <ModalHeader> + <ModalTitle id={modalId}>{t('Outbound_Message')}</ModalTitle> + <ModalClose onClick={() => setClosingConfirmation(true)} /> + </ModalHeader> + <ModalContent pbe={16}> + <OutboundMessageWizard defaultValues={defaultValues} onSuccess={onClose} onError={onClose} /> + </ModalContent> + </Modal> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx(3 hunks)packages/i18n/src/locales/en.i18n.json(3 hunks)packages/i18n/src/locales/pt-BR.i18n.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/i18n/src/locales/en.i18n.json
- apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx
- packages/i18n/src/locales/pt-BR.i18n.json
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
apps/meteor/tests/e2e/page-objects/modal.ts (1)
Modal(3-25)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (2)
56-58: Confirm UX: should errors close the modal?Both
onSuccessandonErrorcallonClose. If errors should keep context for retry, consider not closing on error or showing inline feedback.
61-68: Avoid double backdrops — confirm the confirmation modal doesn't mount its own ModalBackdroprg produced no matches for apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx; manually check that OutboundMessageCloseConfirmationModal does not render/import ModalBackdrop — if it does, remove the inner backdrop or render the confirmation modal outside the outer ModalBackdrop to avoid nested backdrops and focus/stacking issues.
aleksandernsilva
force-pushed
the
feat/outbound-close-confirmation
branch
from
c977c29 to
31c44da
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx (5)
7-10: Hoist mock above the SUT import for clarity.Jest hoists mocks, but placing the mock before importing the component under test avoids surprises and reads clearer.
-import OutboundMessageModal from './OutboundMessageModal'; - -jest.mock('../../components/OutboundMessageWizard', () => ({ +jest.mock('../../components/OutboundMessageWizard', () => ({ __esModule: true, default: () => <div>Outbound Message Wizard</div>, })); + +import OutboundMessageModal from './OutboundMessageModal';
2-2: Import wait utilities to harden async assertions.Using waitForElementToBeRemoved will de-flake the dismissal path.
-import { render, screen } from '@testing-library/react'; +import { render, screen, waitForElementToBeRemoved } from '@testing-library/react';
24-41: Harden the confirmation-flow test: use userEvent.setup, findByRole, and a relaxed accessible description.Prevents race conditions and brittleness if the description includes additional text.
it('should display confirmation before closing the modal', async () => { const onClose = jest.fn(); render(<OutboundMessageModal onClose={onClose} />, { wrapper: appRoot }); expect(screen.getByRole('dialog', { name: 'Outbound Message' })).toBeInTheDocument(); expect(screen.queryByRole('dialog', { name: 'Discard message' })).not.toBeInTheDocument(); - await userEvent.click(screen.getByRole('button', { name: 'Close' })); + const user = userEvent.setup(); + await user.click(screen.getByRole('button', { name: 'Close' })); - expect(screen.getByRole('dialog', { name: 'Discard message' })).toBeInTheDocument(); - expect(screen.getByRole('dialog', { name: 'Discard message' })).toHaveAccessibleDescription( - 'Are you sure you want to discard this outbound message?', - ); + const confirmDialog = await screen.findByRole('dialog', { name: 'Discard message' }); + expect(confirmDialog).toBeInTheDocument(); + expect(confirmDialog).toHaveAccessibleDescription( + expect.stringContaining('Are you sure you want to discard this outbound message?'), + ); expect(screen.queryByRole('button', { name: 'Close' })).not.toBeInTheDocument(); - await userEvent.click(screen.getByRole('button', { name: 'Discard' })); - expect(onClose).toHaveBeenCalled(); + await user.click(screen.getByRole('button', { name: 'Discard' })); + expect(onClose).toHaveBeenCalledTimes(1); });
43-61: De-flake the cancel path: wait for the confirm dialog to appear and then to be removed.Also switches to userEvent.setup for consistency.
it('should close confirmation and leave modal open when cancel is clicked', async () => { const onClose = jest.fn(); render(<OutboundMessageModal onClose={onClose} />, { wrapper: appRoot }); - await userEvent.click(screen.getByRole('button', { name: 'Close' })); + const user = userEvent.setup(); + await user.click(screen.getByRole('button', { name: 'Close' })); - expect(screen.getByRole('dialog', { name: 'Discard message' })).toBeInTheDocument(); - expect(screen.getByRole('dialog', { name: 'Discard message' })).toHaveAccessibleDescription( - 'Are you sure you want to discard this outbound message?', - ); + const confirmDialog = await screen.findByRole('dialog', { name: 'Discard message' }); + expect(confirmDialog).toBeInTheDocument(); + expect(confirmDialog).toHaveAccessibleDescription( + expect.stringContaining('Are you sure you want to discard this outbound message?'), + ); expect(screen.queryByRole('button', { name: 'Close' })).not.toBeInTheDocument(); - await userEvent.click(screen.getByRole('button', { name: 'Keep editing' })); + await user.click(screen.getByRole('button', { name: 'Keep editing' })); - expect(screen.queryByRole('dialog', { name: 'Discard message' })).not.toBeInTheDocument(); - expect(screen.getByRole('dialog', { name: 'Outbound Message' })).toBeInTheDocument(); - expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); + await waitForElementToBeRemoved(confirmDialog); + expect(await screen.findByRole('dialog', { name: 'Outbound Message' })).toBeInTheDocument(); + expect(await screen.findByRole('button', { name: 'Close' })).toBeInTheDocument(); expect(onClose).not.toHaveBeenCalled(); });
24-61: Add coverage for Escape/backdrop behavior introduced by the PR.Test that overlay clicks don’t close and that Escape follows the confirm flow (or is disabled) per spec.
Example tests to add:
it('should not close when clicking the backdrop', async () => { const onClose = jest.fn(); render(<OutboundMessageModal onClose={onClose} />, { wrapper: appRoot }); const user = userEvent.setup(); // Click outside the dialog; adjust selector if your Modal renders a dedicated backdrop element. await user.click(document.body); expect(screen.getByRole('dialog', { name: 'Outbound Message' })).toBeInTheDocument(); expect(onClose).not.toHaveBeenCalled(); }); it('should show confirmation when pressing Escape', async () => { const onClose = jest.fn(); render(<OutboundMessageModal onClose={onClose} />, { wrapper: appRoot }); const user = userEvent.setup(); await user.keyboard('{Escape}'); expect(await screen.findByRole('dialog', { name: 'Discard message' })).toBeInTheDocument(); });Please confirm whether Escape is intended to open the confirmation or be disabled; adjust expectations accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx (1)
12-22: Good use of mockAppRoot with i18n keys.This makes the assertions stable and representative of runtime strings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx (1)
29-31: Avoid double SR announcements; removearia-live.The content is already referenced by aria-describedby; assertive live region can cause duplicate/interrupted reads.
- <p aria-live='assertive' id={`${modalId}-description`}> + <p id={`${modalId}-description`}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.spec.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx (1)
apps/meteor/tests/e2e/page-objects/modal.ts (1)
Modal(3-25)
⏰ 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). (6)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx (3)
20-22: LGTM on structure and i18n usage.Clean separation of concerns, proper labeling via useId, and good copy choices.
Also applies to: 25-27
36-41: Use autoFocus; don’t add type (Fuselage Button defaults to "button")Replace the ref-based focus with autoFocus on the "Keep editing" Button. Do not add type='button' to either Button — @rocket.chat/fuselage already defaults to type="button". File: apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx (lines 36–41)
Likely an incorrect or invalid review comment.
24-24: Keep theopenprop; add the footer annotation id to aria-describedby.Repo uses widely — dropping
openis inconsistent. In apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageCloseConfirmationModal.tsx, append the annotation id to the modal's aria-describedby (aria-describedby={${modalId}-description ${modalId}-annotation}) and add id={${modalId}-annotation} to ModalFooterAnnotation so the “This action cannot be undone” note is included in screen-reader announcements.Likely an incorrect or invalid review comment.
aleksandernsilva
force-pushed
the
feat/outbound-close-confirmation
branch
from
31c44da to
da496b8
Compare
aleksandernsilva
force-pushed
the
feat/outbound-msg-ui
branch
from
9afa3e3 to
669fa47
Compare
aleksandernsilva
force-pushed
the
feat/outbound-close-confirmation
branch
from
da496b8 to
151b126
Compare
aleksandernsilva
merged commit 1eb5ff1
into
feat/outbound-msg-ui
Proposed changes (including videos or screenshots)
Issue(s)
CTZ-325
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Localization
Tests