refactor: Outbound message forms #36972
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughReplaces inline template selection with a new TemplateField, removes the keyboard-submit hook, switches forms to standard handleSubmit, restructures RepliesForm into a new module with Agent/Department fields and utilities, updates tests (removes tinykeys mocks and adjusts import paths), and adds unit tests and helper hooks for agent derivation and allowed-agents logic. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MessageForm
participant TemplateField
participant TemplatePreview
participant Caller
User->>MessageForm: Open form
MessageForm->>TemplateField: Render(control, templates, onChange)
User->>TemplateField: Select template
TemplateField-->>MessageForm: update templateId (form state)
MessageForm->>TemplatePreview: Render preview for selected template
User->>MessageForm: Submit
MessageForm->>MessageForm: handleSubmit -> validate template exists
alt template found
MessageForm-->>Caller: onSubmit({ templateId, templateParameters, template })
else template missing
MessageForm-->>User: show toast error
end
sequenceDiagram
actor User
participant RepliesForm
participant DeptAPI as Department API
participant AgentsAPI as Agents API
participant Caller
User->>RepliesForm: Open form
RepliesForm->>DeptAPI: fetch department (by departmentId)
DeptAPI-->>RepliesForm: department data / error
RepliesForm->>AgentsAPI: fetch agents (permission-gated)
AgentsAPI-->>RepliesForm: agents list
User->>RepliesForm: Select Department / Agent
User->>RepliesForm: Submit
RepliesForm->>RepliesForm: validate department & agent exist
alt Valid
RepliesForm-->>Caller: onSubmit({ departmentId/department, agentId/agent })
else Not found
RepliesForm-->>User: set field error (retry available for dept)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #36972 +/- ##
===========================================
+ Coverage 66.21% 66.24% +0.03%
===========================================
Files 3384 3389 +5
Lines 115027 115187 +160
Branches 21069 21009 -60
===========================================
+ Hits 76161 76304 +143
- Misses 36261 36277 +16
- Partials 2605 2606 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
aleksandernsilva
force-pushed
the
refactor/outbound-forms
branch
from
4494147 to
ea04a45
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx (4)
11-11: Import type from the module barrel to avoid brittle pathsPrefer importing
RepliesFormDatafrom the RepliesForm barrel to prevent coupling to an implementation file and reduce churn if files move.Apply:
-import type { RepliesFormData } from '../RepliesForm'; +import type { RepliesFormData } from '..';
27-30: Guard self‑only assignment when userId is nullIf
canAssignSelfOnlyis true butuseUserId()returns null, the field is enabled with an empty list. Disable in that case.Apply:
-const canAssignAgent = canAssignSelfOnly || canAssignAny; +const canAssignAgent = Boolean(canAssignAny) || (Boolean(canAssignSelfOnly) && !!userId);
31-37: Propagate RHF onBlur/ref to ensure touched state and focus workWire
onBlur(andrefif the component forwards it).Apply:
<AutoCompleteAgent name={agentField.name} + onBlur={agentField.onBlur} aria-busy={isLoading} aria-invalid={!!agentFieldError} @@ - value={agentField.value} - onChange={agentField.onChange} + value={agentField.value} + onChange={agentField.onChange} + ref={agentField.ref} // if AutoCompleteAgent forwards refs; otherwise switch to inputRef prop if available />Please confirm whether
AutoCompleteDepartmentAgentforwardsrefor exposes aninputRefprop.Also applies to: 56-58
4-4: Avoid recomputing filtered agents on every renderMemoize
allowedAgentsto prevent unnecessary filtering whenagentsis large.Apply:
-import { useId } from 'react'; +import { useId, useMemo } from 'react'; @@ -const allowedAgents = canAssignAny ? agents : agents.filter((agent) => agent.agentId === userId); +const allowedAgents = useMemo( + () => (canAssignAny ? agents : agents.filter((agent) => agent.agentId === userId)), + [agents, canAssignAny, userId], +);Also applies to: 29-30
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.spec.tsx (1)
138-150: Re‑enable or document skipped tests.Both “default values” and “submit with correct values” tests are skipped. Either fix and enable them or add a note pointing to a tracking issue.
Apply to re‑enable:
-xit('should render with default values', async () => { +it('should render with default values', async () => {-xit('should call submit with correct values when form is submitted', async () => { +it('should call submit with correct values when form is submitted', async () => {Also applies to: 185-204
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/index.ts (1)
1-2: Barrel export looks good.Optional: if you only intend to re‑export types, prefer
export type *to avoid re‑exporting runtime values. Non‑blocking.-export * from './RepliesForm'; +export type * from './RepliesForm';apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/DepartmentField.tsx (2)
13-20: AvoidonChangenaming collision; make external handler optional.Rename to reduce confusion with RHF’s
onChange, and default to a no‑op.type DepartmentFieldProps = { control: Control<RepliesFormData>; onlyMyDepartments?: boolean; isError: boolean; isFetching: boolean; onRefetch: () => void; - onChange: () => void; + onDepartmentChange?: () => void; }; const DepartmentField = ({ control, onlyMyDepartments, isError, isFetching, onRefetch, - onChange: onChangeExternal, + onDepartmentChange: onChangeExternal = () => {}, }: DepartmentFieldProps) => {Also applies to: 28-29
55-68: Consider exposing loading state to AT.Expose
aria-busywhile fetching to communicate loading to assistive tech. Minor.<AutoCompleteDepartment name={departmentField.name} aria-invalid={!!departmentFieldError} aria-labelledby={departmentFieldId} + aria-busy={isFetching} aria-describedby={cxp(departmentFieldId, { error: !!departmentFieldError, hint: true, })}apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx (4)
70-73: Stabilize/queryKey typing when departmentId is unset.
omnichannelQueryKeys.departmentexpects a string;departmentIdmay be undefined. Coerce to an empty string to satisfy typing and keep a stable key shape.- queryKey: omnichannelQueryKeys.department(departmentId), + queryKey: omnichannelQueryKeys.department(departmentId ?? ''),
112-117: Clear agent errors when department changes.You reset
agentId; also clear potential validation errors to avoid a stale error state.<DepartmentField control={control} onlyMyDepartments={!canAssignAllDepartments} isError={isErrorDepartment} isFetching={isFetchingDepartment} onRefetch={refetchDepartment} - onChange={() => setValue('agentId', '')} + onChange={() => { + setValue('agentId', ''); + clearErrors('agentId'); + }} />
96-104: Async onSubmit handler support (optional).If callers pass an async
onSubmit, you may want to await it to surface errors consistently.- onSubmit({ departmentId, department: updatedDepartment, agentId, agent }); + await Promise.resolve(onSubmit({ departmentId, department: updatedDepartment, agentId, agent }));
29-31: Remove unused exported RepliesFormRef OR implement an imperative ref APIRepliesFormRef is declared and exported in apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx but not referenced anywhere else in the repo — either remove the exported type or implement forwardRef + useImperativeHandle to expose submit.
Suggested removal diff (still valid):
-export type RepliesFormRef = { - submit: () => Promise<RepliesFormSubmitPayload>; -};apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx (1)
83-84: Resetting templateParameters should mark dirty and re‑validate.When the template changes, also trigger dirty/validation to clear any stale errors for placeholders.
Apply this diff:
- <TemplateField control={control} templates={templates} onChange={() => setValue('templateParameters', {})} /> + <TemplateField + control={control} + templates={templates} + onChange={() => setValue('templateParameters', {}, { shouldDirty: true, shouldValidate: true })} + />apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/components/TemplateField.tsx (2)
55-66: Disable the select when no templates are available.Prevents users from interacting with an empty selector and matches the “No_templates_available” validation.
<TemplateSelect aria-labelledby={templateFieldId} aria-invalid={!!templateFieldError} aria-describedby={cxp(templateFieldId, { error: !!templateFieldError, hint: true, })} + disabled={!templates?.length} placeholder={t('Select_template')} error={templateFieldError?.message} templates={templates || []} value={templateField.value} onChange={handleTemplateChange} />
55-77: Prevent duplicate screen-reader error announcementsTemplateSelectProps = Omit<ComponentProps, 'value' | 'onChange' | 'options'> — TemplateSelect forwards SelectFiltered props, so passing templateFieldError?.message into TemplateSelect will forward the message into SelectFiltered and can cause the error to be announced twice. Two options: (A) pass a boolean to TemplateSelect (styling only) and keep FieldError as the single SR message, or (B) keep the message but set aria-describedby to the hint only and wire aria-errormessage to the FieldError id.
<TemplateSelect aria-labelledby={templateFieldId} aria-invalid={!!templateFieldError} - aria-describedby={cxp(templateFieldId, { - error: !!templateFieldError, - hint: true, - })} + aria-describedby={`${templateFieldId}-hint`} + aria-errormessage={templateFieldError ? `${templateFieldId}-error` : undefined} placeholder={t('Select_template')} - error={templateFieldError?.message} + # If TemplateSelect supports boolean `error`, prefer boolean to avoid duplicate SR text: + error={!!templateFieldError} templates={templates || []} value={templateField.value} onChange={handleTemplateChange} />
📜 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 (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.spec.tsx(0 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/components/TemplateField.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.spec.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm.tsx(0 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.spec.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/DepartmentField.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/index.ts(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/hooks/useFormKeyboardSubmit.tsx(0 hunks)
💤 Files with no reviewable changes (3)
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.spec.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/hooks/useFormKeyboardSubmit.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.951Z
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.
📚 Learning: 2025-09-18T17:32:33.951Z
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.951Z
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.tsxapps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx
🧬 Code graph analysis (4)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx (3)
packages/core-typings/src/ILivechatDepartment.ts (1)
ILivechatDepartment(1-24)apps/meteor/client/lib/queryKeys.ts (1)
omnichannelQueryKeys(30-82)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/utils/errors.ts (1)
FormFetchError(7-11)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/DepartmentField.tsx (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx (1)
RepliesFormData(17-20)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/utils/cx.ts (1)
cxp(29-34)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/components/TemplateField.tsx (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx (1)
MessageFormData(17-20)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/utils/cx.ts (1)
cxp(29-34)apps/meteor/client/components/Omnichannel/OutboundMessage/constants.ts (1)
OUTBOUND_DOCS_LINK(1-1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx (1)
RepliesFormData(17-20)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/utils/cx.ts (1)
cxp(29-34)
⏰ 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: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build - coverage
🔇 Additional comments (11)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx (3)
47-50: A11y wiring looks solid
aria-describedbycorrectly references both error and hint IDs;FieldErrorusesaria-live='assertive'. Nice alignment with the recent ARIA fix pattern.If you want, I can run a quick pass to confirm no duplicate IDs are generated across the form.
Also applies to: 61-67
56-58: Resolved — AutoCompleteDepartmentAgent value/onChange match RHF (agentId: string)AutoCompleteDepartmentAgentProps declares value: string and onChange(value: string); option values are set to agent.agentId — passing agentField.value and agentField.onChange is correct.
54-54: Do not change the i18n keys — the current keys exist and are correct.
AgentField.tsx (line 54) uses t('Loading...') and t('Select_agent'); packages/i18n/src/locales/en.i18n.json contains "Loading..." and "Select_agent", so the suggested replacement is incorrect.Likely an incorrect or invalid review comment.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.spec.tsx (2)
229-238: Good a11y assertion on permission messaging.Verifies the accessible description when lacking assign permissions; this protects screen reader UX.
10-10: Import path update LGTM; unable to verify tinykeys removal — run local searchRipgrep in the sandbox skipped files ("No files were searched"); I couldn't confirm absence of tinykeys/useFormKeyboardSubmit. Run locally:
rg -n --hidden -S -g '!/node_modules/' -g '!/dist/' -g '!/build/' "tinykeys|useFormKeyboardSubmit" -C1
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.spec.tsx
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/DepartmentField.tsx (2)
53-77: A11y wiring is solid and consistent.Label id, describedby tokens, and error/hint ids are coherent; this fixes common ARIA mismatches.
40-41: i18n keys verified — no change needed.Both keys exist: "department" and "Outbound_message_department_hint" are defined in packages/i18n/src/locales/en.i18n.json (Outbound_message_department_hint also present in packages/i18n/src/locales/pt-BR.i18n.json).
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx (1)
81-99: Switch to native form submit: LGTM.Standardizing on
<form onSubmit={handleSubmit(submit)}>removes custom keyboard wiring and is clearer.apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/components/TemplateField.tsx (1)
24-41: Validation order and messages: LGTM.Good sequencing: availability → selected‑id existence → required. Reads well and avoids confusing “required” when nothing is available.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx (1)
178-214: Switch to native form submit — leftover tinykeys usages foundSearch shows tinykeys still imported/used and listed as a dependency; tests also mock it. Key locations: apps/meteor/package.json, yarn.lock, apps/meteor/client/views/admin/subscription/SubscriptionPage.tsx, apps/meteor/client/sidebar/hooks/useShortcutOpenMenu.ts, apps/meteor/client/sidebarv2/hooks/useShortcutOpenMenu.ts, apps/meteor/client/sidebar/search/SearchList.tsx, apps/meteor/client/sidebar/header/actions/Search.tsx, apps/meteor/client/NavBarV2/NavBarSearch/NavBarSearch.tsx, and tests: apps/meteor/client/components/Omnichannel/OutboundMessage/.../RecipientStep.spec.tsx, MessageStep.spec.tsx, OutboundMessageWizard.spec.tsx. Either remove these imports/mocks and the dependency if tinykeys is no longer needed, or confirm they’re used for other shortcuts and keep them.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.spec.tsx (1)
8-9: Import path updates: LGTM — verify no remaining old mocks imports
Automated sandbox search didn't scan files; run locally to confirm no remaining imports:
rg -n --no-ignore -S -F "tests/mocks/data"
…36989) Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
tassoevan
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx (2)
24-31: Propagate onBlur to RHF to set touched/trigger validationsuseController provides onBlur; not wiring it can break onBlur validation modes and touched state.
value={agentField.value} onChange={agentField.onChange} + onBlur={agentField.onBlur}
8-8: Naming nit: align import with module nameThe file suggests “AutoCompleteDepartmentAgent” but is imported as AutoCompleteAgent. Aligning names avoids confusion in future refactors.
-import AutoCompleteAgent from '../../../../AutoCompleteDepartmentAgent'; +import AutoCompleteDepartmentAgent from '../../../../AutoCompleteDepartmentAgent';And update usage:
- <AutoCompleteAgent + <AutoCompleteDepartmentAgentapps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (1)
3-3: Consider a more descriptive type guard function name.The type guard
isOmnichannelAgentchecks if a user has the 'livechat-agent' role, but the function name could be more explicit about what it's checking.-const isOmnichannelAgent = (user: IUser | null): user is IUser => (user ? user.roles.includes('livechat-agent') : false); +const hasLivechatAgentRole = (user: IUser | null): user is IUser => (user ? user.roles.includes('livechat-agent') : false);apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.spec.ts (1)
4-4: Consider using a relative import for better maintainability.The import path for
createFakeUseris quite long and brittle to directory structure changes.-import { createFakeUser } from '../../../../../../../../../tests/mocks/data'; +import { createFakeUser } from '@meteor-tests/mocks/data';Note: This assumes a path alias is configured for test mocks. If not available, the current import is acceptable.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.spec.ts (1)
20-41: Consider extracting mock data to reduce duplication.The mock data definitions are quite verbose and could be simplified or extracted to reduce test file size.
const createMockAgent = (overrides: Partial<Serialized<ILivechatDepartmentAgents>> = {}) => ({ agentId: 'agent1', username: 'agent.one', count: 1, order: 1, _id: 'agent1', _updatedAt: new Date().toISOString(), departmentEnabled: true, departmentId: 'dept1', ...overrides, }); const mockQueryAgents = [ createMockAgent(), createMockAgent({ agentId: 'agent2', username: 'agent.two', _id: 'agent2', count: 2, order: 2 }), ];Also applies to: 43-52
📜 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 (7)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.spec.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.spec.ts(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.ts(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.spec.ts(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.spec.tsx
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(186-252)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.spec.ts (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (1)
getAgentDerivedFromUser(5-20)apps/meteor/tests/mocks/data.ts (1)
createFakeUser(32-44)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.spec.ts (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (1)
getAgentDerivedFromUser(5-20)apps/meteor/tests/mocks/data.ts (1)
createFakeUser(32-44)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.ts (1)
useAllowedAgents(14-37)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx (1)
RepliesFormData(18-21)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/utils/cx.ts (1)
cxp(29-34)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-252)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (1)
getAgentDerivedFromUser(5-20)
⏰ 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 (8)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (2)
5-20: LGTM! Clean implementation with proper error handling.The function correctly validates the user's livechat agent role and constructs a well-structured agent object. The error message is clear and the return type matches the expected interface.
14-14: Verify _updatedAt timestamp format consistencyapps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts uses:
_updatedAt: new Date().toISOString(),
Confirm the rest of the codebase and DB expect ISO strings for _updatedAt (not Date objects or epoch millis). If not consistent, standardize or add conversions at read/write boundaries. Automated repo search in the sandbox returned "No files were searched" — manual verification required.apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.spec.ts (1)
6-39: Comprehensive test coverage with clear test cases.The tests cover all the essential scenarios:
- Null user validation
- Non-livechat agent validation
- Successful agent derivation with proper object structure verification
The use of
expect.any(String)for the timestamp is appropriate since the exact value is non-deterministic.apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.spec.ts (2)
8-12: LGTM! Proper mocking setup for isolated unit testing.The mock implementation correctly isolates the
getAgentDerivedFromUserdependency and provides a typed mock for better test control.
54-169: Excellent test coverage of all hook scenarios.The test suite comprehensively covers:
- Empty department ID handling
- Permission-based filtering logic
- Query agents vs derived agents logic
- Error handling with safe fallback
Each test case is well-structured with clear setup, execution, and assertions. The use of
renderHookis appropriate for testing this custom hook.apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.ts (3)
6-12: Well-defined TypeScript interface.The
UseAllowedAgentsPropstype clearly defines all required parameters with appropriate nullability and optional markers.
14-37: Clean and logical hook implementation.The hook efficiently handles all business logic scenarios:
- Early returns for invalid states
- Priority-based agent selection logic
- Safe error handling with fallback
The dependency array is complete and the memoization will prevent unnecessary recalculations.
26-29: Verify agent selection priority & constraintsFile: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.ts (lines 26–29)
// can assign any agent, return all agents from query (if any) if (canAssignAnyAgent && queryAgents?.length) { return queryAgents; }
- Ensure queryAgents is filtered before return: exclude offline agents, agents in disabled departments, and agents not in the target department; enforce agent concurrent-chat / global limits.
- Confirm the check uses the correct transfer permission (Transfer Livechat Guests) so only authorized users can manually assign.
- Confirm derived-agent logic (contact manager / last-chatted / routing / auto-assignment) is a fallback when queryAgents is empty — unless workspace routing/contact-manager settings explicitly require derived agents to take priority; adjust priority accordingly.
...OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx
Show resolved
Hide resolved
...OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx
Show resolved
Hide resolved
aleksandernsilva
removed
the
stat: QA assured
dionisio-bot
bot
removed
the
stat: ready to merge
aleksandernsilva
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
This PR:
RepliesFormandMessageFormto improve readability and separation of concernsRecipientForm.spec.tsxto the RecipientForm folderIssue(s)
CTZ-186
Steps to test or reproduce
N/A
Further comments
Summary by CodeRabbit