sync fixes #167
sync fixes #167
Conversation
WalkthroughEdits convert several select handlers to accept single objects, refine template and validator flows, add a feature flag gate for start_session, adjust select component behaviors, remove PATCH from webhook methods and include result_name in webhook payloads, and apply multiple guards, whitespace and minor UI refinements across router/forms. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Template UI
participant Form as SendMsgForm
participant State as Component State
User->>UI: Select template (single object)
UI->>Form: onChange(template)
Note over Form: handleTemplateChanged(template)
Form->>Form: templateTranslation = template.translations[0]
Form->>Form: templateVariables = compute by variable_count (or reinit)
Form->>State: set { template:{ value: template }, templateTranslation, templateVariables }
State-->>UI: Re-render with updated template UI
sequenceDiagram
autonumber
participant Form as Webhook Form
participant Helper as stateToNode()
participant Node as Flow Node
Form->>Helper: Save (state with method, url, resultName)
Note right of Helper: METHOD_OPTIONS excludes PATCH
Helper->>Node: Emit CallWebhook { method, url, result_name }
Node-->>Form: Node persisted/updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
…e in SendMsgForm feat: Add HAS_START_SESSION feature filter in interfaces feat: Update typeConfigs to include filter for StartSessionForm fix: Remove console logs from initializeForm in helpers
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/components/flow/actions/sendbroadcast/SendBroadcastForm.tsx (1)
245-270: Update array argument in handleTemplateChanged call to an object
In src/components/flow/actions/sendbroadcast/SendBroadcastForm.tsx (line 61), you’re still invokingthis.handleTemplateChanged([{ …this.state.template.value, …asset.content }]);but the handler now expects a single template object. Change it to:
this.handleTemplateChanged({ …this.state.template.value, …asset.content });src/components/flow/actions/sendmsg/SendMsgForm.tsx (3)
183-186: Save blocked when attachment is valid (logic inverted)Early return triggers when attachments[0].valid is true; this prevents saving valid forms.
- if (this.state.attachments.length > 0 && this.state.attachments[0].valid) { + if (this.state.attachments.length > 0 && !this.state.attachments[0].valid) { return; }Also update the tab hasErrors predicate below to match validity (see next comment).
515-531: Tab hasErrors predicate invertedhasErrors should reflect invalid state, not valid.
- hasErrors: this.state.attachments.length > 0 && this.state.attachments[0].valid + hasErrors: + this.state.attachments.length > 0 && this.state.attachments[0].valid === falseIf multiple attachments are possible, consider computing any invalid instead of index 0.
Also applies to: 529-529
119-139: Replace.idwith.uuidin template comparisons
- In
src/components/flow/actions/sendmsg/helpers.ts(around line 126), changetoaction.templating.template.uuid === state.template.value.idaction.templating.template.uuid === state.template.value.uuid- In
src/components/flow/actions/sendbroadcast/helpers.ts(around line 115), changetoaction.templating.template.uuid === formState.template.value.idaction.templating.template.uuid === formState.template.value.uuidThis ensures consistent use of
uuidand prevents template-reuse breakage.src/components/flow/routers/webhook/helpers.ts (1)
135-151: Bug: result_name now always set on action, breaking router-level result.Setting result_name unconditionally means createServiceCallSplitNode never receives a router result_name, changing behavior for existing flows.
Apply:
const newAction: CallWebhook = { uuid, headers, type: Types.call_webhook, url: state.url.value, body: state.body.value, - method: state.method.value.value as Methods, - result_name: state.resultName.value + method: state.method.value.value as Methods }; - // if the action had the result name, keep the result on the action rather than the router - if (originalAction && originalAction.result_name) { - newAction.result_name = state.resultName.value; - } + // keep result on action only if the original action had it; otherwise put it on the router + const putResultOnAction = !!(originalAction && originalAction.result_name); + if (putResultOnAction) { + newAction.result_name = state.resultName.value; + } return createServiceCallSplitNode( newAction, settings.originalNode, '@webhook.status', Operators.has_number_between, ['200', '299'], - newAction.result_name ? '' : state.resultName.value // put result on router if not on action + putResultOnAction ? '' : state.resultName.value // put result on router if not on action );
🧹 Nitpick comments (16)
src/components/flow/actions/updategroup/helpers.ts (3)
39-47: Add a null guard before dereferencing field in stateToActionIf stateToAction is ever called with state.field.value unset, this will throw at field.label. Add a defensive check and use getName for a safer fallback.
Apply:
- const field = state.field.value; + const field = state.field.value; + if (!field) { + throw new Error('Field is required before building SetWAGroupField'); + } @@ - field: { name: field.label, key: field.key }, + field: { name: getName(field), key: field.key },
50-52: Tighten typing for getName inputAvoid any to improve safety and editor help.
-export const getName = (asset: any): string => { +export const getName = (asset: { label?: string; name?: string; key?: string }): string => {
20-22: Remove redundant state.type reassignmenttype is already set to Types.set_wa_group_field in the initializer.
- if (settings.originalAction) { - state.type = Types.set_wa_group_field; + if (settings.originalAction) {src/temba/TembaSelectElement.tsx (1)
22-22: Value now always sourced from entry; remove redundant value props upstream.This overrides any value passed via spread. Recommend not passing value from parents (e.g., LLMForm) to avoid confusion.
src/temba/TembaSelect.tsx (2)
116-134: Call handleCreateAsset only when creating a new item; also prefer const.Invoking before the existing-check adds unnecessary side-effects and risks future double-creation if handleCreateAsset starts persisting. Use const and move the call.
- if (input.indexOf('@') === -1) { - var existing = options.find(function(option: any) { + if (input.indexOf('@') === -1) { + const existing = options.find(function(option: any) { const name = select.getName(option); return !!(name.toLowerCase().trim() === input.toLowerCase().trim()); }); - this.handleCreateAsset(input); if (!existing) { + this.handleCreateAsset(input); if (this.props.createArbitraryOption) { return this.props.createArbitraryOption(input); }
188-190: Avoid console noise in production.Gate logs or use a logger.
- public handleCreateAsset = (asset: any): void => { - console.log('Create asset', asset); - }; + public handleCreateAsset = (asset: any): void => { + if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line no-console + console.debug('Create asset', asset); + } + };src/components/flow/routers/llm/LLMForm.tsx (1)
132-133: Redundant value prop; TembaSelectElement overrides it.Safe to remove to reduce confusion.
- onChange={this.handleLLMUpdate} - value={this.state.llm.value ? this.state.llm.value : null} + onChange={this.handleLLMUpdate}src/components/flow/actions/addlabels/AddLabelsForm.tsx (3)
51-52: Remove debug logs.These add noise and may leak context in prod.
- console.log(selected);
58-59: Remove debug logs.- console.log(updated);
86-87: Remove debug logs.- console.log(this.context);src/components/flow/routers/sheet/SheetForm.tsx (3)
25-25: Context access wired correctly; consider typing contextDefining static contextTypes with fakePropType unblocks this.context usage. Optionally add a context type to the class for better TS safety.
-export default class SheetForm extends React.Component<RouterFormProps, SheetFormState> { +export default class SheetForm extends React.Component<RouterFormProps, SheetFormState> { + context!: { config: any }; public static contextTypes = { config: fakePropType };Also applies to: 96-100
177-189: Remove debug logging and make submitting flag useful (or drop it)console.log leaks noise. Also, submitting is never true in current call sites, so shouldRequireIf never fires here.
- private handleSheetChanged(value: any, submitting: boolean = false) { - console.log(value); + private handleSheetChanged(value: any, submitting: boolean = false) { const updates: Partial<SheetFormState> = { sheet: validate(i18n.t('forms.sheet', 'Sheet'), value, [shouldRequireIf(submitting)]) }; - // console.log(sheetValue); const updated = mergeForm(this.state, updates); this.setState(updated); }If you want validation to surface on save, you can keep this as-is; otherwise consider passing submitting=true from an onBlur/onClose path.
242-252: Use a stable key for WRITE sheet select as wellYou added key="sheet_select" for READ; mirror it for WRITE to avoid element identity churn when toggling action_type.
- <TembaSelectElement + <TembaSelectElement + key="sheet_select" name={i18n.t('forms.sheet', 'Sheet')}Also applies to: 299-307
src/components/flow/actions/sendmsg/helpers.ts (1)
74-74: Drop debug logconsole.log(template) is noisy and can leak data in production logs.
- console.log(template);src/components/flow/actions/sendmsg/SendMsgForm.tsx (2)
325-326: Remove console logging in render/onChangeAvoid logging state and change events in production UI.
- console.log(this.state); ... - onChange={e => { - console.log('template changed'); - this.handleTemplateChanged(e); - }} + onChange={this.handleTemplateChanged}Also applies to: 341-345
552-557: Tab order logic is confusingYou splice templates at index 0, then reverse the array, which puts attachments first anyway. Simplify to explicit ordering.
- const tabs = [attachments]; - if (hasFeature(this.context.config, FeatureFilter.HAS_WHATSAPP)) { - const templates: Tab = { ... }; - tabs.splice(0, 0, templates); - } - tabs.reverse(); + const tabs: Tab[] = []; + if (hasFeature(this.context.config, FeatureFilter.HAS_WHATSAPP)) { + const templates: Tab = { ... }; + tabs.push(templates); + } + tabs.push(attachments);Also applies to: 564-564
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
src/components/flow/actions/addlabels/AddLabelsForm.tsx(2 hunks)src/components/flow/actions/sendbroadcast/SendBroadcastForm.tsx(1 hunks)src/components/flow/actions/sendmsg/SendMsgForm.tsx(7 hunks)src/components/flow/actions/sendmsg/helpers.ts(1 hunks)src/components/flow/actions/updategroup/helpers.ts(1 hunks)src/components/flow/routers/llm/LLMForm.tsx(2 hunks)src/components/flow/routers/sheet/SheetForm.tsx(4 hunks)src/components/flow/routers/webhook/helpers.ts(2 hunks)src/config/interfaces.ts(1 hunks)src/config/typeConfigs.ts(1 hunks)src/temba/TembaSelect.tsx(2 hunks)src/temba/TembaSelectElement.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/temba/TembaSelectElement.tsx (1)
src/temba/TembaSelect.tsx (1)
TembaSelect(56-239)
src/components/flow/actions/addlabels/AddLabelsForm.tsx (2)
src/store/validators.ts (2)
validate(118-132)shouldRequireIf(272-280)src/store/nodeEditor.ts (1)
mergeForm(63-156)
src/config/typeConfigs.ts (1)
src/components/flow/actions/startsession/StartSession.tsx (1)
StartSessionComp(11-37)
src/components/flow/actions/sendmsg/SendMsgForm.tsx (5)
src/store/validators.ts (2)
shouldRequireIf(272-280)validate(118-132)src/store/nodeEditor.ts (1)
mergeForm(63-156)src/flowTypes.ts (1)
Template(235-239)src/config/typeConfigs.ts (1)
hasFeature(661-663)src/components/dialog/Dialog.tsx (1)
Tab(20-26)
src/temba/TembaSelect.tsx (1)
lambda/revisions.js (1)
asset(77-86)
src/components/flow/routers/sheet/SheetForm.tsx (3)
src/config/ConfigProvider.tsx (1)
fakePropType(4-4)src/store/validators.ts (1)
shouldRequireIf(272-280)src/store/nodeEditor.ts (1)
mergeForm(63-156)
🪛 GitHub Actions: Build
src/components/flow/actions/sendmsg/SendMsgForm.tsx
[error] 17-17: Line 17:10: 'fetchAsset' is defined but never used. @typescript-eslint/no-unused-vars
🔇 Additional comments (8)
src/components/flow/actions/updategroup/helpers.ts (1)
41-42: No-op formatting change — OKThe added blank line improves readability and has no runtime effect.
src/components/flow/routers/llm/LLMForm.tsx (1)
96-97: Confirm llm is always initialized; otherwise this can throw.Direct access assumes this.state.llm is always defined. If nodeToState ever omits llm, revert to optional chaining here.
src/components/flow/routers/sheet/SheetForm.tsx (1)
11-17: Validator import looks goodBringing in shouldRequireIf aligns with the new conditional validation flow.
src/components/flow/actions/sendmsg/SendMsgForm.tsx (3)
121-124: Conditional message requirement is sensibleRequiring message only when no template and no attachments on submit is correct.
254-259: Usable translation helper is fineClear and localized here for reuse.
336-348: Optional: show “Expression” first-class without hitting endpointSince you inject additionalOption, ensure TembaSelectElement merges options with endpoint results predictably. If it replaces instead of merges, users may only see “Expression”.
Would you confirm TembaSelectElement merges options with fetched items? If not, we should switch to createPrefix or a grouped list approach.
src/config/interfaces.ts (1)
125-127: Verify environment includes ‘start_session’ feature flag
FeatureFilter.HAS_START_SESSION is applied in typeConfigs (src/config/typeConfigs.ts:464) and referenced by related components—ensure your deployment’s feature‐flag filters include ‘start_session’ to avoid regressions.src/config/typeConfigs.ts (1)
463-465: Confirm default config & update release notes
- Enable
FeatureFilter.HAS_START_SESSIONin all default/tenant configurations so the action remains visible where it was before.- Document the new
start_sessionfeature flag in the release notes.
| // { value: Methods.PATCH, name: Methods.PATCH }, | ||
| { value: Methods.FUNCTION, name: Methods.FUNCTION } | ||
| ]; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm intentional removal of PATCH from options (and enum).
If any existing actions used PATCH, they’ll still round-trip but users can’t select it. Make sure no tenants rely on PATCH or plan a migration.
🏁 Script executed:
#!/bin/bash
# Scan repo for PATCH mentions
rg -n '\bPATCH\b'Length of output: 495
Confirm PATCH removal and update snapshots
- Ensure removal of PATCH from
Methodsis intentional and no existing tenants rely on it. - Remove or update the
PATCHreferences insrc/components/flow/routers/webhook/__snapshots__/WebhookRouterForm.test.tsx.snap(lines 181–182) to keep tests in sync.
🤖 Prompt for AI Agents
In src/components/flow/routers/webhook/helpers.ts around lines 41 to 43, the
PATCH method was removed from the Methods list but the change may affect tenants
and tests; confirm PATCH removal is intentional for all tenants and then update
tests — remove or replace the PATCH entries in
src/components/flow/routers/webhook/__snapshots__/WebhookRouterForm.test.tsx.snap
(around lines 181–182) so snapshots match the new Methods list, and run/update
snapshot tests to persist the corrected snapshot file.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/components/flow/actions/updategroup/UpdateGroupForm.tsx (1)
88-92: Fix callsite: pass a single Asset, not an array.
handlePropertyChangenow expects a single selection. Passing[field]stores an array instate.field.value, andonUpdatedthen reads.typefrom an array, causing a runtime error.public handleFieldAdded(field: Asset): void { // update our store with our new group this.props.addAsset('fields', field); - this.handlePropertyChange([field]); + this.handlePropertyChange(field); }src/components/flow/actions/sendmsg/helpers.ts (1)
121-129: uuid/id mismatch prevents templating UUID reuseYou compare action.templating.template.uuid to state.template.value.id. The selected template object in this flow uses uuid, not id, so the reuse branch never triggers.
- action.templating.template.uuid === state.template.value.id + action.templating.template.uuid === state.template.value.uuidsrc/components/flow/actions/sendmsg/SendMsgForm.tsx (2)
181-183: Save guard inverted: prevents saving when attachment is validReturning early when attachments[0].valid is true blocks valid submissions and allows invalid ones through the rest of the flow.
- if (this.state.attachments.length > 0 && this.state.attachments[0].valid) { - return; - } + if (this.state.attachments.length > 0 && !this.state.attachments[0].valid) { + // Block save when the first attachment is invalid + return; + }
517-519: Tab error flag likely reversedhasErrors should be true when the first attachment is invalid, not valid.
- hasErrors: this.state.attachments.length > 0 && this.state.attachments[0].valid + hasErrors: this.state.attachments.length > 0 && !this.state.attachments[0].valid
🧹 Nitpick comments (10)
src/components/flow/actions/updategroup/UpdateGroupForm.tsx (2)
65-72: Tighten types and ensure deterministic boolean return.Use a precise type for
selectedand always return a boolean to match the signature.- private handlePropertyChange(selected: any): boolean { + private handlePropertyChange(selected: Asset | null): boolean { if (selected) { return this.handleUpdate({ type: Types.set_wa_group_field, field: selected }); } + return false; }
39-47: Remove unused parametersubmitting.
submittingis never read; drop it to simplify the API.- private handleUpdate( - keys: { - type?: Types; - field?: Asset; - fieldValue?: string; - }, - submitting = false - ): boolean { + private handleUpdate( + keys: { + type?: Types; + field?: Asset; + fieldValue?: string; + } + ): boolean {src/components/flow/actions/sendinteractivemsg/SendInteractiveMsg.tsx (1)
57-66: Handle rejected fetches to avoid unhandled promise rejectionsWhen the request fails (network/5xx), the promise rejects and nothing sets a fallback body. Add a catch to show the placeholder and avoid console noise.
- if (id) { - getAsset(endpoint, type, id.toString()).then(response => { - if (response.error) { - setBody(PLACEHOLDER); - } else { - addAsset('interactives', response); - setNode(response); - } - }); - } + if (id) { + getAsset(endpoint, type, id.toString()) + .then(response => { + if (response.error) { + setBody(PLACEHOLDER); + return; + } + addAsset('interactives', response); + setNode(response); + }) + .catch(() => setBody(PLACEHOLDER)); + }src/components/flow/routers/contactprofile/helpers.ts (1)
40-41: Remove stray console.log.Leftover debug logging can leak state and clutter consoles.
- console.log(resulNode);src/components/flow/routers/contactprofile/ContactProfileRouterForm.tsx (2)
51-55: Clear dependent profileType when switching away from “Create Profile”.Prevents stale type data if the user changes the method.
private handleTypeChange(selection: any): void { - if (selection && this.state.optionType.value.id !== selection.id) { - this.setState({ - optionType: { value: selection } - }); - } + if (selection && this.state.optionType.value.id !== selection.id) { + const updates: Partial<ContactProfileRouterFormState> = { + optionType: { value: selection } + }; + if (selection.id !== '1') { + updates.profileType = { value: '' }; + } + this.setState(updates as any); + } }
237-246: Use the same stable key for WRITE path too.Keeps the select’s identity consistent across mode switches.
Apply in the WRITE section:
- <TembaSelectElement + <TembaSelectElement + key="sheet_select" name={i18n.t('forms.sheet', 'Sheet')}src/temba/TembaSelect.tsx (1)
121-129: GuardcreateArbitraryOptionor require it when usingcreatePrefix
No<TembaSelect>in the codebase currently setscreatePrefix, but if it’s ever provided withoutcreateArbitraryOption, this line will callundefined(input)and crash:- const item = this.props.createArbitraryOption(input); + const item = this.props.createArbitraryOption + ? this.props.createArbitraryOption(input) + : { [this.props.nameKey || 'name']: input, [this.props.valueKey || 'value']: input };Alternatively, tighten the
TembaSelectPropstype so thatcreateArbitraryOptionis required whenevercreatePrefixis set.src/components/flow/routers/sheet/SheetForm.tsx (1)
237-238: Stable key on READ sheet select — mirror in WRITE.Add the same key in WRITE block for consistency (see suggested diff in ContactProfileRouterForm comment).
src/components/flow/routers/result/ResultRouterForm.tsx (2)
71-73: Consider trimming and a stricter start rule for resultName.If users can enter leading/trailing spaces or start with whitespace, add a trim validator and/or a rule like “must start with a letter” (not just non-numeric) if that’s desired.
Do we already have a
Trimvalidator instore/validators? If yes, add it beforeStartIsNonNumeric. If not, I can draft one.
12-12: Align Alphanumeric validator message with regex and backend
Regex/^[a-z\d\-_\s]+$/i(src/store/validators.ts:370) permits letters, numbers, spaces, hyphens, and underscores, but the error text reads “can only have letters and numbers.” Confirm that server-side accepts spaces, hyphens, and underscores, then update either the UI copy to list those characters or tighten the regex to match “letters and numbers” only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
src/components/flow/actions/addlabels/__snapshots__/AddLabelsForm.test.tsx.snapis excluded by!**/*.snapsrc/components/flow/actions/sendmsg/__snapshots__/SendMsgForm.test.ts.snapis excluded by!**/*.snapsrc/components/flow/actions/startsession/__snapshots__/StartSessionForm.test.tsx.snapis excluded by!**/*.snapsrc/components/flow/routers/webhook/__snapshots__/WebhookRouterForm.test.tsx.snapis excluded by!**/*.snapsrc/config/__snapshots__/typeConfigs.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
src/components/flow/actions/addlabels/AddLabelsForm.tsx(1 hunks)src/components/flow/actions/sendinteractivemsg/SendInteractiveMsg.tsx(1 hunks)src/components/flow/actions/sendinteractivemsg/SendInteractiveMsgForm.tsx(1 hunks)src/components/flow/actions/sendmsg/SendMsgForm.tsx(8 hunks)src/components/flow/actions/sendmsg/helpers.ts(1 hunks)src/components/flow/actions/updategroup/UpdateGroupForm.tsx(1 hunks)src/components/flow/routers/contactprofile/ContactProfileRouterForm.tsx(3 hunks)src/components/flow/routers/contactprofile/helpers.ts(1 hunks)src/components/flow/routers/result/ResultRouterForm.tsx(3 hunks)src/components/flow/routers/result/helpers.ts(1 hunks)src/components/flow/routers/sheet/SheetForm.tsx(4 hunks)src/temba/TembaSelect.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/flow/actions/addlabels/AddLabelsForm.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
src/components/flow/actions/sendinteractivemsg/SendInteractiveMsg.tsx (2)
src/external/index.ts (1)
getAsset(170-183)src/store/thunks.ts (1)
addAsset(458-477)
src/components/flow/routers/contactprofile/ContactProfileRouterForm.tsx (1)
src/components/form/select/SelectElement.tsx (1)
SelectElement(30-54)
src/components/flow/routers/sheet/SheetForm.tsx (3)
src/config/ConfigProvider.tsx (1)
fakePropType(4-4)src/store/validators.ts (1)
shouldRequireIf(272-280)src/store/nodeEditor.ts (1)
mergeForm(63-156)
src/components/flow/routers/result/ResultRouterForm.tsx (2)
src/store/validators.ts (3)
Alphanumeric(370-370)validate(118-132)shouldRequireIf(272-280)src/store/nodeEditor.ts (1)
mergeForm(63-156)
src/components/flow/actions/sendmsg/SendMsgForm.tsx (5)
src/store/validators.ts (1)
shouldRequireIf(272-280)src/utils/index.tsx (1)
range(408-409)src/flowTypes.ts (1)
Template(235-239)src/config/typeConfigs.ts (1)
hasFeature(661-663)src/components/dialog/Dialog.tsx (1)
Tab(20-26)
⏰ 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: Coverage
- GitHub Check: Tests (16.x)
🔇 Additional comments (17)
src/components/flow/actions/updategroup/UpdateGroupForm.tsx (1)
141-152: No change: TembaSelectElement defaults to single-select and onChange emits a single Asset
multiisfalsewhen unspecified, soonChangeuses the single‐value branch and passes oneAssetrather than an array.src/components/flow/actions/sendinteractivemsg/SendInteractiveMsgForm.tsx (1)
342-342: Noise reduction: console log removal is goodRemoving the stray console output from render helps keep logs clean.
src/components/flow/actions/sendmsg/helpers.ts (1)
145-145: Formatting-only changeNo functional impact. LGTM.
src/components/flow/actions/sendmsg/SendMsgForm.tsx (5)
91-99: Template rehydration on mount looks goodUsing fetchAsset and then normalizing via handleTemplateChanged resolves the earlier “unused import” situation and keeps state consistent.
118-121: Confirm message-required ruleYou now require message only when submitting and neither a template nor attachments are present. If a media template without attachments is selected, message stays optional but template gets a validation error below—intentional?
253-258: Translation gating is sensibleAccepting pending/approved translations for usability is fine.
296-313: Label select: stable keys and creation UXAdding key, valueKey="uuid", and allowCreate improves stability and UX. LGTM.
541-549: Template tab checked-stateUsing != null for checked aligns with the single-template model. LGTM.
src/components/flow/routers/contactprofile/helpers.ts (1)
28-31: Storing full option object is correct and aligns with SelectElement.This keeps state/props consistent across form and helpers.
src/components/flow/routers/contactprofile/ContactProfileRouterForm.tsx (2)
15-15: Switching to SelectElement is appropriate.Matches the new state shape (full option object) and stabilizes select behavior.
90-96: LGTM: SelectElement usage with stable key.Key and full-option value wiring look good.
src/temba/TembaSelect.tsx (1)
76-78: LGTM: Added type guard for label.Safer access on arbitrary options.
src/components/flow/routers/sheet/SheetForm.tsx (3)
11-17: Validator imports and shouldRequireIf usage are correct.Enables conditional required checks on submit.
96-100: Context typing looks good.Ensures config is available for endpoints.
177-184: Sheet onChange now defers “required” to submit — good.mergeForm keeps validity in sync.
src/components/flow/routers/result/helpers.ts (1)
129-131: Guard against nullresultand useresultValueconsistently
- Replace
let resultValue = result.id || result.value || result.name;
with
const resultValue = result?.id ?? result?.value ?? result?.name;- Update all
operandandidreferences to useresultValue(lines 129–131, 135, 144).- Confirm the UI disables saving when no result is selected; if not, add an explicit check before building the operand.
src/components/flow/routers/result/ResultRouterForm.tsx (1)
91-103: Always validate/update and return a boolean; make selection optionalFile: src/components/flow/routers/result/ResultRouterForm.tsx (handleResultChanged)
Non‑exhaustive return (no path for cleared selection) — make
selectedoptional, always run validate/mergeForm/setState, and return the form validity. Apply the diff below:- private handleResultChanged(selected: Asset, submitting = false): boolean { - if (selected) { - const updates: Partial<ResultRouterFormState> = { - result: validate(i18n.t('forms.result_to_split_on', 'Result to split on'), selected, [ - shouldRequireIf(submitting) - ]) - }; - - const updated = mergeForm(this.state, updates); - this.setState(updated); - return updated.valid; - } - } + private handleResultChanged(selected: Asset | undefined, submitting = false): boolean { + const updates: Partial<ResultRouterFormState> = { + result: validate(i18n.t('forms.result_to_split_on', 'Result to split on'), selected as any, [ + shouldRequireIf(submitting) + ]) + }; + const updated = mergeForm(this.state, updates); + this.setState(updated); + return updated.valid; + }Confirm TembaSelectElement.onChange payload type — I couldn't locate TembaSelectElement/SelectOption in the repo. If onChange emits a SelectOption, change
selectedto that type (e.g.SelectOption | undefined) and map the option to an Asset before calling validate (e.g., use option.value or similar).
| private handleTemplateChanged(template: any): void { | ||
| const templateTranslation = template.translations[0]; | ||
| const templateVariables = | ||
| this.state.templateVariables.length === 0 || | ||
| (this.state.template.value && this.state.template.value.uuid !== template.uuid) | ||
| ? range(0, templateTranslation.variable_count).map(() => { | ||
| return { | ||
| value: '' | ||
| }; | ||
| }) | ||
| : this.state.templateVariables; | ||
|
|
||
| this.setState({ | ||
| expression: null, | ||
| template: { value: template }, | ||
| templateTranslation, | ||
| templateVariables | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle null clear, ‘Expression’ option, and missing variable_count
Current code assumes template and template.translations[0] always exist; selecting “Expression” (custom option) or clearing the select will crash or compute range(0, undefined). Also, keeping Expression in template causes stateToAction to emit a template with undefined uuid. Treat Expression as a separate mode and clear template/variables accordingly.
- private handleTemplateChanged(template: any): void {
- const templateTranslation = template.translations[0];
- const templateVariables =
- this.state.templateVariables.length === 0 ||
- (this.state.template.value && this.state.template.value.uuid !== template.uuid)
- ? range(0, templateTranslation.variable_count).map(() => {
- return {
- value: ''
- };
- })
- : this.state.templateVariables;
-
- this.setState({
- expression: null,
- template: { value: template },
- templateTranslation,
- templateVariables
- });
- }
+ private handleTemplateChanged(template: any | null): void {
+ // Clear selection
+ if (!template) {
+ this.setState({
+ expression: null,
+ template: { value: null },
+ templateTranslation: undefined,
+ templateVariables: []
+ });
+ return;
+ }
+ // Expression mode
+ if (template.name === 'Expression') {
+ this.setState({
+ expression: { value: '' },
+ template: { value: null },
+ templateTranslation: undefined,
+ templateVariables: []
+ });
+ return;
+ }
+ const tr = template.translations?.[0];
+ const varCount = (tr && typeof tr.variable_count === 'number') ? tr.variable_count : 0;
+ const resetVars =
+ this.state.templateVariables.length === 0 ||
+ (this.state.template.value && this.state.template.value.uuid !== template.uuid);
+ const templateVariables = resetVars
+ ? range(0, varCount).map(() => ({ value: '' }))
+ : this.state.templateVariables;
+
+ this.setState({
+ expression: null,
+ template: { value: template },
+ templateTranslation: tr,
+ templateVariables
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private handleTemplateChanged(template: any): void { | |
| const templateTranslation = template.translations[0]; | |
| const templateVariables = | |
| this.state.templateVariables.length === 0 || | |
| (this.state.template.value && this.state.template.value.uuid !== template.uuid) | |
| ? range(0, templateTranslation.variable_count).map(() => { | |
| return { | |
| value: '' | |
| }; | |
| }) | |
| : this.state.templateVariables; | |
| this.setState({ | |
| expression: null, | |
| template: { value: template }, | |
| templateTranslation, | |
| templateVariables | |
| }); | |
| } | |
| private handleTemplateChanged(template: any | null): void { | |
| // Clear selection | |
| if (!template) { | |
| this.setState({ | |
| expression: null, | |
| template: { value: null }, | |
| templateTranslation: undefined, | |
| templateVariables: [] | |
| }); | |
| return; | |
| } | |
| // Expression mode | |
| if (template.name === 'Expression') { | |
| this.setState({ | |
| expression: { value: '' }, | |
| template: { value: null }, | |
| templateTranslation: undefined, | |
| templateVariables: [] | |
| }); | |
| return; | |
| } | |
| // Safe translation lookup and variable_count default | |
| const tr = template.translations?.[0]; | |
| const varCount = (tr && typeof tr.variable_count === 'number') ? tr.variable_count : 0; | |
| // Determine whether to reset variables | |
| const resetVars = | |
| this.state.templateVariables.length === 0 || | |
| (this.state.template.value && this.state.template.value.uuid !== template.uuid); | |
| const templateVariables = resetVars | |
| ? range(0, varCount).map(() => ({ value: '' })) | |
| : this.state.templateVariables; | |
| this.setState({ | |
| expression: null, | |
| template: { value: template }, | |
| templateTranslation: tr, | |
| templateVariables | |
| }); | |
| } |
🤖 Prompt for AI Agents
src/components/flow/actions/sendmsg/SendMsgForm.tsx around lines 225 to 243, the
handler assumes template and template.translations[0] always exist which crashes
when the select is cleared or the special "Expression" option is chosen and can
leave a template with undefined uuid; update the method to treat a falsy
template or an explicit Expression sentinel as the expression mode by clearing
template, templateTranslation and templateVariables (set template.value to null
or clear the template object) so stateToAction won't emit an undefined uuid, and
when a real template is provided safely read template.translations[0] only if
present and use a default variable_count of 0 (e.g., Number(...)||0) before
calling range to build templateVariables.
| if (!name && this.props.nameKey && this.props.nameKey in option) { | ||
| name = option[this.props.nameKey]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard ‘in’ check with an object type check.
Avoids runtime errors when option is not an object.
- if (!name && this.props.nameKey && this.props.nameKey in option) {
+ if (
+ !name &&
+ this.props.nameKey &&
+ option &&
+ typeof option === 'object' &&
+ this.props.nameKey in option
+ ) {
name = option[this.props.nameKey];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!name && this.props.nameKey && this.props.nameKey in option) { | |
| name = option[this.props.nameKey]; | |
| } | |
| if ( | |
| !name && | |
| this.props.nameKey && | |
| option && | |
| typeof option === 'object' && | |
| this.props.nameKey in option | |
| ) { | |
| name = option[this.props.nameKey]; | |
| } |
🤖 Prompt for AI Agents
In src/temba/TembaSelect.tsx around lines 72 to 74, the code uses the "in"
operator on option without confirming option is an object, which can throw if
option is null or a primitive; guard the check by ensuring option is non-null
and typeof option === 'object' (or use option && typeof option === 'object')
before evaluating this.props.nameKey in option, so update the if condition to
include that object-type check.
Testing sheet
Summary by CodeRabbit
New Features
Improvements
Bug Fixes