refactor: refactor code #38387
refactor: refactor code #38387
Conversation
## What was done Refactored `ReturnChatQueueModal` to use the shared `GenericModal` component instead of composing the modal structure manually. ## Why This aligns the Omnichannel modal with the standardized modal pattern used across the codebase, reducing duplicated boilerplate and improving consistency and maintainability. ## Changes - Replaced raw Fuselage modal primitives with `GenericModal` - Mapped existing header, icon, content, and footer actions to `GenericModal` props - Removed redundant modal layout code ### Verification - Confirmed the modal renders correctly - Confirmed cancel and confirm actions behave the same as before ## Notes This is a refactor-only change; no visual or functional behavior was modified.
|
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 |
|
|
|
WalkthroughReplaced bespoke modal compositions with Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
amankv1234
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/omnichannel/modals/CloseChatModal.tsx`:
- Line 178: Remove the noisy inline implementation comment inside the
CloseChatModal component's JSX (the comment text like {/* Transcript section
unchanged */}); instead delete that JSX comment altogether so the markup is
cleaner and follows the guideline to avoid implementation comments in
CloseChatModal (look for the JSX block in the CloseChatModal function where the
transcript section is rendered and remove the inline comment).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/omnichannel/modals/ReturnChatQueueModal.tsx`:
- Line 14: Remove the stray development comment "//model to genericmodel" from
the ReturnChatQueueModal component (ReturnChatQueueModal.tsx); delete the line
entirely (do not replace it with another comment) so the codebase contains no
leftover dev notes or typos.
apps/meteor/client/views/omnichannel/modals/ReturnChatQueueModal.tsx
Outdated
Show resolved
Hide resolved
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/views/omnichannel/modals/CloseChatModal.tsx (1)
82-107: Broken validation logic: staleerrorsand inverted condition.Two issues prevent correct validation:
Stale
errorsreference:setErrorschedules a state update buterrorsin this closure still holds the previous render's value. The checks on line 102 will never see the errors just set above.Inverted condition:
!errors.comment || errors.tagsreads "proceed if no comment error OR there are tag errors", which is the opposite of the intended logic. Also,errors.subjectis never checked.Refactor to validate inline and return early, avoiding reliance on stale state:
🐛 Proposed fix
const onSubmit = useCallback( ({ comment, tags, transcriptPDF, transcriptEmail, subject }: CloseChatModalFormData) => { const preferences = { omnichannelTranscriptPDF: !!transcriptPDF, omnichannelTranscriptEmail: alwaysSendTranscript ? true : !!transcriptEmail, }; const requestData = transcriptEmail && visitorEmail ? { email: visitorEmail, subject } : undefined; + let hasError = false; + if (!comment?.trim() && commentRequired) { setError('comment', { type: 'custom', message: t('Required_field', { field: t('Comment') }) }); + hasError = true; } if (transcriptEmail && !subject) { setError('subject', { type: 'custom', message: t('Required_field', { field: t('Subject') }) }); + hasError = true; } if (!tags?.length && tagRequired) { setError('tags', { type: 'custom', message: t('error-tags-must-be-assigned-before-closing-chat') }); + hasError = true; } - if (!errors.comment || errors.tags) { + if (!hasError) { onConfirm(comment, tags, preferences, requestData); } }, - [commentRequired, tagRequired, visitorEmail, errors, setError, t, onConfirm, alwaysSendTranscript], + [commentRequired, tagRequired, visitorEmail, setError, t, onConfirm, alwaysSendTranscript], );
|
Thanks for the review! |
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/views/omnichannel/modals/CloseChatModal.tsx (1)
162-170: Potential issue:subjectin dependency array may cause unintended resets.Including
subjectin the dependency array while also callingsetValue('subject', ...)can create unexpected behavior. If the user clears the subject field, the empty string is falsy, sosubject || customSubject || t(...)resets it to the default value—preventing the user from intentionally clearing it.While subject is required when
transcriptEmailis enabled (so an empty value would fail validation anyway), this pattern is confusing and could mask bugs.Consider removing
subjectfrom the dependency array and only setting the default whentranscriptEmailfirst becomes true:Suggested approach
useEffect(() => { if (transcriptEmail) { if (!visitorEmail) { dispatchToastMessage({ type: 'error', message: t('Customer_without_registered_email') }); return; } - setValue('subject', subject || customSubject || t('Transcript_of_your_livechat_conversation')); + setValue('subject', customSubject || t('Transcript_of_your_livechat_conversation'), { shouldDirty: false }); } -}, [transcriptEmail, setValue, visitorEmail, subject, t, customSubject, dispatchToastMessage]); +}, [transcriptEmail, setValue, visitorEmail, t, customSubject, dispatchToastMessage]);
|
Thanks for pointing this out! |
4 participants
What was done
Refactored
ReturnChatQueueModalto use the sharedGenericModalcomponent instead of composing the modal structure manually.Why
This aligns the Omnichannel modal with the standardized modal pattern used across the codebase, reducing duplicated boilerplate and improving consistency and maintainability.
Changes
GenericModalGenericModalpropsVerification
Notes
This is a refactor-only change; no visual or functional behavior was modified.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.