feat: conditional logic v1 #23
Conversation
* Added 'display_depends_on' field to the form field JSON and Python definitions. * Updated field order to include 'reqd' and 'fieldtype' for better organization.
* Moved FieldPropertiesForm.vue to the field-editor directory for better organization. * Updated import paths in FieldEditorSidebar.vue to reflect the new location.
📝 WalkthroughWalkthroughAdds conditional field logic: a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor as Field Properties Editor
participant Conditional as ConditionalLogicSection
participant Store as Edit Form Store
participant Renderer as FormRenderer
participant Utils as conditionals.ts
User->>Editor: Select field to edit
Editor->>Store: read selectedField
Editor->>Conditional: render conditional UI
User->>Conditional: add/modify conditions
Conditional->>Store: update selectedField.conditional_logic
Store-->>Conditional: emit updated selectedField
User->>Renderer: open form / change values
Renderer->>Utils: getVisibleFields(formFields, formValues)
Utils->>Utils: evaluate conditions
Utils-->>Renderer: visibleFields
Renderer->>Utils: shouldFieldBeRequired(field, formValues, allFields)
Utils-->>Renderer: required boolean
Renderer->>Renderer: render & validate visible fields
sequenceDiagram
participant Conditional as ConditionalLogicSection
participant Store as Edit Form Store
participant Local as Component State
Conditional->>Store: watch selectedField.conditional_logic
Store-->>Conditional: emit changes
Conditional->>Local: parse into local state
Local->>Conditional: user edits conditions
Conditional->>Store: stringify and set selectedField.conditional_logic
Conditional->>Local: reset update guard (microtask)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)frontend/src/utils/conditionals.ts (2)
🪛 Ruff (0.14.11)forms_pro/forms_pro/doctype/form_field/form_field.py36-36: Undefined name (F821) 37-37: Undefined name (F821) ⏰ 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)
🔇 Additional comments (10)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forms_pro/forms_pro/doctype/form_field/form_field.py (1)
65-73: Adddisplay_depends_onto theto_frappe_fieldproperty output.The
display_depends_onfield is defined in the type hints but is missing from theto_frappe_fieldproperty (lines 65-73). Since this property is used inform.pyto serialize field data when synchronizing with DocType, thedisplay_depends_onvalue will not be available for field updates or creation. Include it in the returned dictionary:Updated to_frappe_field method
return { "fieldname": self.fieldname, "fieldtype": _fieldtype, "label": self.label, "reqd": self.reqd, "options": self.options, "description": self.description, "default": self.default, "display_depends_on": self.display_depends_on, }
🤖 Fix all issues with AI agents
In `@frontend/src/components/builder/field-editor/ConditionalLogicSection.vue`:
- Around line 1-12: The template uses the Button component but it isn't
imported; update the top import from "frappe-ui" to include Button alongside
FormControl and Popover (so import Button, FormControl, Popover from
"frappe-ui") so the Button symbol used in the ConditionalLogicSection component
is defined.
- Around line 196-215: The action and target_field selectors are rendered inside
the v-for loop over conditions causing duplicates; move the FormControl blocks
bound to conditionalLogic.action and conditionalLogic.target_field (the selects
using actionOptions and availableFieldOptions) out of the loop that iterates
conditions so they render once per conditionalLogic instead of once per
condition; locate the v-for that iterates conditions and remove the "Then" div
plus the two FormControl elements from inside it and place them after (or
before) the conditions loop so the controls still bind to
conditionalLogic.action and conditionalLogic.target_field but are not repeated.
In `@frontend/src/types/conditional-render.types.ts`:
- Around line 21-24: The LogicOperators enum is declared but unused; add a
logic_operator field to the ConditionalLogic type so callers can express how the
conditions array should be combined (AND vs OR). Update the ConditionalLogic
definition to include a new optional or required property (e.g., logic_operator:
LogicOperators) and ensure any construction/validation of ConditionalLogic
(places that build or consume ConditionalLogic) are updated to set or handle
this field; use the existing LogicOperators enum values (And, Or) as the allowed
options.
- Around line 26-30: The Condition type's value currently only allows a single
primitive (string | number | boolean) but ConditionalOperators includes range
operators IsBetween/IsNotBetween which need two values; update the Condition
definition (and any consumers) so value can represent ranges—either change value
to accept a tuple/array like [string|number, string|number] or union it with a
{minValue: string|number; maxValue: string|number} shape, and ensure code
handling Condition (e.g., evaluation/validation logic) checks the operator and
reads the tuple or min/max fields for IsBetween/IsNotBetween while preserving
existing single-value behavior for other operators.
In `@frontend/src/types/formfield.ts`:
- Around line 1-18: The FormFieldTypes enum includes unsupported values
'Checkbox' and 'Rating' that don't exist in the backend form_field.json; remove
'Checkbox' and 'Rating' from the export in the FormFieldTypes enum so the
front-end only exposes backend-supported types (Attach, Data, Number, Email,
Date, Date Time, Date Range, Time Picker, Password, Select, Switch, Textarea,
Text Editor, Link), and update any code that references FormFieldTypes.Checkbox
or FormFieldTypes.Rating (forms, validators, tests) to use a supported type or a
clear fallback/mapping to avoid sending invalid field types to the backend.
In `@frontend/src/utils/conditionals.ts`:
- Around line 256-311: The early return that checks the current field's own
display_depends_on (the block that returns true when field.display_depends_on is
missing or empty) prevents any targeting rules on other fields from being
considered; remove that initial check so visibility is determined solely by
scanning allFields for ConditionalLogic entries targeting this field (keep the
existing allFields scan, the targetingRules length check, and the rest of the
evaluation logic), and update the comment above shouldFieldBeVisible to reflect
that visibility is driven by rules on other fields rather than a property on the
field itself.
🧹 Nitpick comments (9)
frontend/src/components/FormBuilderContent.vue (1)
81-86: Remove duplicate selector.Lines 83 and 85 are functionally identical selectors (attribute order doesn't matter in CSS selectors). Remove one to avoid redundancy.
Suggested fix
const hasOpenDropdown = !!( document.querySelector('[role="listbox"]:not([hidden]):not([style*="display: none"])') || document.querySelector('[role="combobox"][aria-expanded="true"]') || - document.querySelector('[data-headlessui-state="open"]') || - document.querySelector('[aria-expanded="true"][role="combobox"]') + document.querySelector('[data-headlessui-state="open"]') );frontend/src/components/submission/FormRenderer.vue (1)
38-51: Consider memoizing required status computation.Calling
shouldFieldBeRequiredinline in the template means it executes on every render for each field. For typical form sizes this is acceptable, but for forms with many fields and complex conditional rules, consider computing required statuses in a memoized computed property alongsidevisibleFields.Optional optimization
const visibleFields = computed(() => { const fields = submissionFormStore.formResource.data?.fields || []; return fields.filter((field: FormField) => shouldFieldBeVisible(field, submissionFormStore.fields, fields) - ); + ).map((field: FormField) => ({ + ...field, + reqd: shouldFieldBeRequired(field, submissionFormStore.fields, fields), + })); });Then simplify the template:
<FieldRenderer :disabled="disabled" v-model="submissionFormStore.fields[field.fieldname]" - :field="{ - ...field, - reqd: shouldFieldBeRequired( - field, - submissionFormStore.fields, - submissionFormStore.formResource.data?.fields || [] - ), - }" + :field="field" :inEditMode="false" />frontend/src/components/builder/field-editor/FieldPropertiesForm.vue (1)
91-95: ConditionalLogicSection ignores the v-model binding.The
ConditionalLogicSectioncomponent manages state internally via the store rather than through thev-modelprop passed here. While this works because both components share the same store, this creates an inconsistency in the data flow pattern compared to other field properties.Consider either:
- Making
ConditionalLogicSectionaccept and use thev-modelprop for consistency- Or separating this component from the
fieldPropertiesarray since it uses a different data flow patternfrontend/src/components/builder/field-editor/ConditionalLogicSection.vue (2)
70-78: Potential race condition withsetTimeoutfor flag reset.Using
setTimeout(..., 0)to resetisUpdatingFromStorecould cause race conditions if the store updates rapidly. Consider usingqueueMicrotaskor Vue'snextTickfor more predictable timing within Vue's reactivity cycle.♻️ Proposed fix
+import { ref, watch, computed, nextTick } from "vue"; ... } finally { - // Reset flag after a microtask to allow the other watcher to process - setTimeout(() => { - isUpdatingFromStore.value = false; - }, 0); + // Reset flag after Vue's next tick to allow the other watcher to process + nextTick(() => { + isUpdatingFromStore.value = false; + }); }
80-94: Defaulttarget_fieldset tonullmay cause validation issues.When adding a condition,
target_fieldis initialized tonull. If the user saves without selecting a target field, this could result in invalid conditional logic. Consider validating before save or providing a more explicit default.frontend/src/utils/conditionals.ts (3)
38-51: Type comparison uses string literals instead of enum values.
field.fieldtypeis now typed asFormFieldTypesenum, but the comparisons use string literals. While this works due to TypeScript's structural typing with string enums, using the enum values would be more type-safe and maintainable.♻️ Proposed fix
+import { FormField, FormFieldTypes } from "@/types/formfield"; ... // Handle boolean/switch fields - if (fieldType === "Switch" || fieldType === "Checkbox") { + if (fieldType === FormFieldTypes.Switch || fieldType === FormFieldTypes.Checkbox) { return Boolean(fieldValue); } // Handle number fields - if (fieldType === "Number") { + if (fieldType === FormFieldTypes.Number) { const num = Number(fieldValue); return isNaN(num) ? null : num; }
97-109: Redundantundefinedcheck afternormalizeValue.
normalizeValuenever returnsundefined(onlynull, boolean, number, or string), so checking forundefinedon lines 101-102 and 108 is unnecessary.♻️ Proposed fix
case ConditionalOperators.IsEmpty: return ( - normalizedFieldValue === null || - normalizedFieldValue === "" || - normalizedFieldValue === undefined + normalizedFieldValue === null || + normalizedFieldValue === "" ); case ConditionalOperators.IsNotEmpty: return ( normalizedFieldValue !== null && - normalizedFieldValue !== "" && - normalizedFieldValue !== undefined + normalizedFieldValue !== "" );
188-218: Document expected format forIsBetween/IsNotBetweenoperators.The "min,max" format expected for
IsBetweenandIsNotBetweenoperators is parsed inline but not documented in theConditiontype. Consider adding a JSDoc comment or updating the type definition to clarify this requirement.frontend/src/types/conditional-render.types.ts (1)
38-42: Clarify whentarget_fieldcan benull.If
target_fieldrepresents the field affected by this conditional logic, when would it legitimately benull? If it's always required at runtime, consider making it non-nullable to avoid unnecessary null checks in consuming code. If there's a valid use case fornull, a comment documenting it would be helpful.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
forms_pro/forms_pro/doctype/form_field/form_field.jsonforms_pro/forms_pro/doctype/form_field/form_field.pyfrontend/src/components/FieldEditorSidebar.vuefrontend/src/components/FormBuilderContent.vuefrontend/src/components/builder/FieldPropertiesForm.vuefrontend/src/components/builder/field-editor/ConditionalLogicSection.vuefrontend/src/components/builder/field-editor/FieldPropertiesForm.vuefrontend/src/components/submission/FormRenderer.vuefrontend/src/stores/submissionForm.tsfrontend/src/types/conditional-render.types.tsfrontend/src/types/formfield.tsfrontend/src/utils/conditionals.ts
💤 Files with no reviewable changes (1)
- frontend/src/components/builder/FieldPropertiesForm.vue
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/utils/conditionals.ts (1)
frontend/src/types/conditional-render.types.ts (2)
ConditionalLogic(38-42)Condition(26-30)
frontend/src/stores/submissionForm.ts (1)
frontend/src/utils/conditionals.ts (2)
shouldFieldBeVisible(256-311)shouldFieldBeRequired(316-349)
⏰ 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)
- GitHub Check: Server
🔇 Additional comments (9)
frontend/src/components/FormBuilderContent.vue (2)
17-63: Good defensive approach for portal-based dropdown detection.The DOM traversal approach handles various UI library patterns well. This ensures dropdown interactions don't accidentally deselect the currently edited field.
99-108: LGTM!The multi-condition check properly prevents field deselection during dropdown interactions, which is essential for the new conditional logic editing workflow.
frontend/src/components/FieldEditorSidebar.vue (1)
3-3: LGTM!Import path updated to reflect the new field-editor folder structure for the conditional logic feature.
frontend/src/components/submission/FormRenderer.vue (1)
20-27: LGTM!The computed property correctly filters fields based on conditional visibility and will reactively update when form values change.
frontend/src/stores/submissionForm.ts (1)
168-186: The integration of conditional logic in this function is correct, but the underlying utility design needs clarification.The
validateValues()function correctly usesshouldFieldBeVisibleandshouldFieldBeRequired. However, there is a logical inconsistency in theshouldFieldBeVisibleutility (line 262 ofconditionals.ts): the early return comment suggests a field's owndisplay_depends_oncontrols its visibility, but the actual implementation below searches for OTHER fields' rules that target this field. The early check at line 262–263 returnstruefor fields withoutdisplay_depends_on, yet this comment contradicts the actual behavior where visibility is determined by targeting rules from other fields. This design pattern (storing visibility rules on the controlling field rather than the controlled field) should be verified as intentional and properly documented to avoid confusion.frontend/src/components/builder/field-editor/FieldPropertiesForm.vue (1)
106-113: LGTM!The conditional rendering with
v-ifand dynamic component iteration pattern is clean and correctly guards against nullselectedField.frontend/src/types/formfield.ts (1)
20-30: LGTM!The
FormFieldtype is well-structured with appropriate optional properties. The addition ofdisplay_depends_onas an optional string aligns with the backend schema.forms_pro/forms_pro/doctype/form_field/form_field.json (1)
67-71: LGTM!The
display_depends_onfield is appropriately defined as aCodefieldtype, which is suitable for storing JSON-formatted conditional logic data.frontend/src/utils/conditionals.ts (1)
12-25: LGTM!The
parseConditionalLogicandgetVisibleFieldsfunctions are clean utilities with proper error handling for JSON parsing failures.Also applies to: 354-361
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <script setup lang="ts"> | ||
| import { useEditForm } from "@/stores/editForm"; | ||
| import { ref, watch, computed } from "vue"; | ||
| import { | ||
| ConditionalLogic, | ||
| ConditionalOperators, | ||
| type Condition, | ||
| Actions, | ||
| } from "@/types/conditional-render.types"; | ||
| import { EllipsisVertical, Trash, Workflow, Zap } from "lucide-vue-next"; | ||
| import { FormControl, Popover } from "frappe-ui"; | ||
| import type { FormField } from "@/types/formfield"; |
There was a problem hiding this comment.
Missing Button import.
The Button component is used in the template (lines 149, 161, 217, 224) but is not imported from frappe-ui.
🐛 Proposed fix
-import { FormControl, Popover } from "frappe-ui";
+import { FormControl, Popover, Button } from "frappe-ui";📝 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.
| <script setup lang="ts"> | |
| import { useEditForm } from "@/stores/editForm"; | |
| import { ref, watch, computed } from "vue"; | |
| import { | |
| ConditionalLogic, | |
| ConditionalOperators, | |
| type Condition, | |
| Actions, | |
| } from "@/types/conditional-render.types"; | |
| import { EllipsisVertical, Trash, Workflow, Zap } from "lucide-vue-next"; | |
| import { FormControl, Popover } from "frappe-ui"; | |
| import type { FormField } from "@/types/formfield"; | |
| <script setup lang="ts"> | |
| import { useEditForm } from "@/stores/editForm"; | |
| import { ref, watch, computed } from "vue"; | |
| import { | |
| ConditionalLogic, | |
| ConditionalOperators, | |
| type Condition, | |
| Actions, | |
| } from "@/types/conditional-render.types"; | |
| import { EllipsisVertical, Trash, Workflow, Zap } from "lucide-vue-next"; | |
| import { FormControl, Popover, Button } from "frappe-ui"; | |
| import type { FormField } from "@/types/formfield"; |
🤖 Prompt for AI Agents
In `@frontend/src/components/builder/field-editor/ConditionalLogicSection.vue`
around lines 1 - 12, The template uses the Button component but it isn't
imported; update the top import from "frappe-ui" to include Button alongside
FormControl and Popover (so import Button, FormControl, Popover from
"frappe-ui") so the Button symbol used in the ConditionalLogicSection component
is defined.
There was a problem hiding this comment.
It's imported globally
There was a problem hiding this comment.
@harshtandiya, thank you for the clarification! If Button is globally imported in your project setup, then no change is needed here. The code is correct as-is.
frontend/src/components/builder/field-editor/ConditionalLogicSection.vue
Outdated
Show resolved
Hide resolved
| export enum LogicOperators { | ||
| And = "And", | ||
| Or = "Or", | ||
| } |
There was a problem hiding this comment.
LogicOperators enum is defined but never used.
The ConditionalLogic type has a conditions array but no field to specify how multiple conditions should be combined (AND vs OR). Consider adding a logic_operator field to ConditionalLogic:
Proposed fix
export type ConditionalLogic = {
conditions: Condition[];
+ logic_operator: LogicOperators;
action: Actions;
target_field: string | null;
};🤖 Prompt for AI Agents
In `@frontend/src/types/conditional-render.types.ts` around lines 21 - 24, The
LogicOperators enum is declared but unused; add a logic_operator field to the
ConditionalLogic type so callers can express how the conditions array should be
combined (AND vs OR). Update the ConditionalLogic definition to include a new
optional or required property (e.g., logic_operator: LogicOperators) and ensure
any construction/validation of ConditionalLogic (places that build or consume
ConditionalLogic) are updated to set or handle this field; use the existing
LogicOperators enum values (And, Or) as the allowed options.
| export type Condition = { | ||
| fieldname: string; | ||
| operator: ConditionalOperators; | ||
| value: string | number | boolean; | ||
| }; |
There was a problem hiding this comment.
IsBetween/IsNotBetween operators require two values but Condition.value only supports a single value.
The value field is typed as string | number | boolean, which cannot represent a range. Consider supporting an array or tuple for range-based operators:
Proposed fix
export type Condition = {
fieldname: string;
operator: ConditionalOperators;
- value: string | number | boolean;
+ value: string | number | boolean | [number, number];
};Alternatively, add separate minValue/maxValue fields, or remove IsBetween/IsNotBetween if they're not yet supported.
📝 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.
| export type Condition = { | |
| fieldname: string; | |
| operator: ConditionalOperators; | |
| value: string | number | boolean; | |
| }; | |
| export type Condition = { | |
| fieldname: string; | |
| operator: ConditionalOperators; | |
| value: string | number | boolean | [number, number]; | |
| }; |
🤖 Prompt for AI Agents
In `@frontend/src/types/conditional-render.types.ts` around lines 26 - 30, The
Condition type's value currently only allows a single primitive (string | number
| boolean) but ConditionalOperators includes range operators
IsBetween/IsNotBetween which need two values; update the Condition definition
(and any consumers) so value can represent ranges—either change value to accept
a tuple/array like [string|number, string|number] or union it with a {minValue:
string|number; maxValue: string|number} shape, and ensure code handling
Condition (e.g., evaluation/validation logic) checks the operator and reads the
tuple or min/max fields for IsBetween/IsNotBetween while preserving existing
single-value behavior for other operators.
| export enum FormFieldTypes { | ||
| Attach = "Attach", | ||
| Data = "Data", | ||
| Number = "Number", | ||
| Email = "Email", | ||
| Date = "Date", | ||
| DateTime = "Date Time", | ||
| DateRange = "Date Range", | ||
| TimePicker = "Time Picker", | ||
| Password = "Password", | ||
| Select = "Select", | ||
| Switch = "Switch", | ||
| Textarea = "Textarea", | ||
| TextEditor = "Text Editor", | ||
| Link = "Link", | ||
| Checkbox = "Checkbox", | ||
| Rating = "Rating", | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate form_field.json to verify the backend claim
find . -name "form_field.json" -type f 2>/dev/null | head -20Repository: BuildWithHussain/forms_pro
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Check the current formfield.ts file to confirm enum contents
cat -n frontend/src/types/formfield.tsRepository: BuildWithHussain/forms_pro
Length of output: 900
🏁 Script executed:
#!/bin/bash
# Run the verification script from the review to check usage
echo "=== Checking for Checkbox field type usage ==="
rg -n "Checkbox" --type=ts --type=vue -g '!*.spec.*' -g '!*.test.*' 2>/dev/null | head -30
echo "=== Checking for Rating field type usage ==="
rg -n "Rating" --type=ts --type=vue -g '!*.spec.*' -g '!*.test.*' 2>/dev/null | head -30Repository: BuildWithHussain/forms_pro
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Read the form_field.json to check what field types are supported in backend
cat ./forms_pro/forms_pro/doctype/form_field/form_field.json | head -200Repository: BuildWithHussain/forms_pro
Length of output: 1799
Enum includes field types not supported by backend.
The FormFieldTypes enum includes Checkbox and Rating field types that are not listed in the backend form_field.json fieldtype options. The backend supports only: Attach, Data, Number, Email, Date, Date Time, Date Range, Time Picker, Password, Select, Switch, Textarea, Text Editor, and Link. This mismatch could cause issues if these types are selected in the UI but rejected by backend validation.
🤖 Prompt for AI Agents
In `@frontend/src/types/formfield.ts` around lines 1 - 18, The FormFieldTypes enum
includes unsupported values 'Checkbox' and 'Rating' that don't exist in the
backend form_field.json; remove 'Checkbox' and 'Rating' from the export in the
FormFieldTypes enum so the front-end only exposes backend-supported types
(Attach, Data, Number, Email, Date, Date Time, Date Range, Time Picker,
Password, Select, Switch, Textarea, Text Editor, Link), and update any code that
references FormFieldTypes.Checkbox or FormFieldTypes.Rating (forms, validators,
tests) to use a supported type or a clear fallback/mapping to avoid sending
invalid field types to the backend.
…he codebase * Updated JSON, Python, and Vue components to reflect the new field name. * Adjusted parsing and evaluation functions to accommodate the change. * Ensured consistency in the handling of conditional logic throughout the application.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/utils/conditionals.ts`:
- Around line 39-41: getFieldValue references a non-existent backend field type
"Checkbox", causing inconsistency with form_field.json which only defines
"Switch"; either remove "Checkbox" from the boolean branch or normalize it to
"Switch". Fix by editing getFieldValue to stop checking for "Checkbox" (only
treat fieldType === "Switch" as boolean) or add a short normalization step (map
"Checkbox" to "Switch" before the existing checks) and update any callers/tests
to use the normalized type; reference getFieldValue, fieldType, "Checkbox", and
"Switch" when making the change.
- Around line 188-218: The IsBetween and IsNotBetween branches in evaluate
(ConditionalOperators.IsBetween / ConditionalOperators.IsNotBetween) use
inconsistent defaults for malformed input—IsBetween returns false while
IsNotBetween returns true; make them consistent by treating malformed inputs as
unevaluable and returning false in both cases: update the IsNotBetween checks
that currently return true (the initial typeof conditionValue !== "string" /
normalizedFieldValue === null check and the isNaN/min/max/type checks) to return
false, matching the IsBetween behavior so both operators behave symmetrically
when conditionValue or normalizedFieldValue is invalid.
♻️ Duplicate comments (4)
frontend/src/types/formfield.ts (1)
16-17: Enum includes field types not supported by backend.
CheckboxandRatingare not in the backend'sform_field.jsonfieldtype options. This mismatch could cause validation failures if these types are selected in the UI.frontend/src/utils/conditionals.ts (1)
256-311: Logic confusion inshouldFieldBeVisible- checks two different concepts.Lines 261-264 check if the current field has
conditional_logic, but lines 269-276 search for rules in OTHER fields that target this field. These are unrelated checks:
- A field's own
conditional_logicdefines rules that affect OTHER fields (viatarget_field)- This function should only check if OTHER fields have rules targeting THIS field
The early return creates a bug: fields without their own
conditional_logicwill be visible regardless of whether other fields have rules hiding them.🐛 Proposed fix - remove the early return
export function shouldFieldBeVisible( field: FormField, formValues: Record<string, any>, allFields: FormField[] ): boolean { - // If field has no conditional_logic, it's always visible - if (!field.conditional_logic || field.conditional_logic.trim() === "") { - return true; - } - // Find all conditional logic rules that target this field const targetingRules: ConditionalLogic[] = [];frontend/src/components/builder/field-editor/ConditionalLogicSection.vue (2)
11-11: MissingButtonimport causes runtime error.The
Buttoncomponent is used in the template (lines 149, 161, 217, 224) but is not imported fromfrappe-ui.🐛 Proposed fix
-import { FormControl, Popover } from "frappe-ui"; +import { FormControl, Popover, Button } from "frappe-ui";
196-214: Action and target field controls are duplicated inside the condition loop.The "Then" section (action and target_field selectors) is inside the
v-forloop over conditions (lines 137-215). Sinceactionandtarget_fieldare properties ofconditionalLogic(not per-condition), this creates duplicate controls when multiple conditions exist.🔧 Proposed fix - move outside the loop
</div> + </div> + <!-- Action section - outside the condition loop --> + <div class="flex flex-col gap-2 mt-4 pt-4 border-t"> + <div class="flex items-center gap-2 text-ink-gray-5 text-sm"> + <Zap class="w-4 h-4" /> + <span>Then</span> + </div> + <FormControl + type="select" + placeholder="Select Action" + variant="outline" + :options="actionOptions" + v-model="conditionalLogic.action" + /> + <FormControl + type="select" + placeholder="Select Field" + variant="outline" + :options="availableFieldOptions" + v-model="conditionalLogic.target_field" + /> </div> - <!-- Actions --> - <div class="flex items-center gap-2 ..."> - ... - </div> - </div>Move lines 196-214 outside the
v-forblock that ends at line 215.
🧹 Nitpick comments (2)
frontend/src/components/builder/field-editor/ConditionalLogicSection.vue (2)
70-78: Consider usingqueueMicrotaskinstead ofsetTimeout.Using
setTimeout(..., 0)to reset the flag works butqueueMicrotaskis more semantically correct for deferring to the next microtask and avoids timer overhead.♻️ Proposed refactor
} finally { // Reset flag after a microtask to allow the other watcher to process - setTimeout(() => { + queueMicrotask(() => { isUpdatingFromStore.value = false; - }, 0); + }); }
101-106: Field options include the currently selected field.
availableFieldOptionsincludes all fields, which means a field's conditional logic could target itself. Consider filtering out the current field to prevent self-referencing rules.♻️ Proposed fix
const availableFieldOptions = computed(() => { - return editFormStore.fields.map((field: FormField) => ({ + return editFormStore.fields + .filter((field: FormField) => field.fieldname !== editFormStore.selectedField?.fieldname) + .map((field: FormField) => ({ label: field.label, value: field.fieldname, })); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
forms_pro/forms_pro/doctype/form_field/form_field.jsonforms_pro/forms_pro/doctype/form_field/form_field.pyfrontend/src/components/builder/field-editor/ConditionalLogicSection.vuefrontend/src/components/builder/field-editor/FieldPropertiesForm.vuefrontend/src/types/formfield.tsfrontend/src/utils/conditionals.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/builder/field-editor/FieldPropertiesForm.vue
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/types/formfield.ts (1)
forms_pro/forms_pro/doctype/form_field/form_field.py (1)
FormField(8-73)
frontend/src/utils/conditionals.ts (2)
frontend/src/types/conditional-render.types.ts (2)
ConditionalLogic(38-42)Condition(26-30)frontend/src/types/formfield.ts (1)
FormField(20-30)
⏰ 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)
- GitHub Check: Server
🔇 Additional comments (6)
forms_pro/forms_pro/doctype/form_field/form_field.json (1)
67-71: LGTM - New conditional_logic field added correctly.The
Codefieldtype is appropriate for storing JSON-formatted conditional logic strings.forms_pro/forms_pro/doctype/form_field/form_field.py (2)
17-17: Type annotation added correctly.The
conditional_logic: DF.Code | Nonetype annotation aligns with the JSON field definition.
65-72: No action needed. Theconditional_logicfield is correctly excluded fromto_frappe_field. It is a frontend-only feature used to control field visibility and conditional requirements during form submission, and is not part of the Frappe field schema. The backend properly syncs only the field properties that Frappe DocField supports (label,fieldtype,reqd,options,description,default).Likely an incorrect or invalid review comment.
frontend/src/types/formfield.ts (1)
20-30: LGTM - FormField type updated correctly.The
conditional_logic?: stringproperty aligns with the backend'sDF.Codefield, and usingFormFieldTypesenum forfieldtypeimproves type safety.frontend/src/utils/conditionals.ts (2)
12-25: LGTM - JSON parsing with error handling.The function correctly handles null/undefined/empty strings and catches JSON parse errors.
316-349: LGTM -shouldFieldBeRequiredcorrectly evaluates targeting rules.Unlike
shouldFieldBeVisible, this function correctly scans all fields for rules targeting the current field without an erroneous early return.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if (fieldType === "Switch" || fieldType === "Checkbox") { | ||
| return Boolean(fieldValue); | ||
| } |
There was a problem hiding this comment.
Checkbox field type referenced but not in backend.
getFieldValue handles Checkbox field type, but the backend form_field.json only defines Switch for boolean fields. Consider removing Checkbox or mapping it to Switch for consistency.
🔧 Proposed fix
// Handle boolean/switch fields
- if (fieldType === "Switch" || fieldType === "Checkbox") {
+ if (fieldType === "Switch") {
return Boolean(fieldValue);
}📝 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 (fieldType === "Switch" || fieldType === "Checkbox") { | |
| return Boolean(fieldValue); | |
| } | |
| // Handle boolean/switch fields | |
| if (fieldType === "Switch") { | |
| return Boolean(fieldValue); | |
| } |
🤖 Prompt for AI Agents
In `@frontend/src/utils/conditionals.ts` around lines 39 - 41, getFieldValue
references a non-existent backend field type "Checkbox", causing inconsistency
with form_field.json which only defines "Switch"; either remove "Checkbox" from
the boolean branch or normalize it to "Switch". Fix by editing getFieldValue to
stop checking for "Checkbox" (only treat fieldType === "Switch" as boolean) or
add a short normalization step (map "Checkbox" to "Switch" before the existing
checks) and update any callers/tests to use the normalized type; reference
getFieldValue, fieldType, "Checkbox", and "Switch" when making the change.
frontend/src/utils/conditionals.ts
Outdated
| case ConditionalOperators.IsBetween: | ||
| // For "Is Between", value should be a string like "min,max" | ||
| if (normalizedFieldValue === null || typeof conditionValue !== "string") { | ||
| return false; | ||
| } | ||
| const [min, max] = conditionValue.split(",").map((v) => Number(v.trim())); | ||
| if ( | ||
| isNaN(min) || | ||
| isNaN(max) || | ||
| typeof normalizedFieldValue !== "number" | ||
| ) { | ||
| return false; | ||
| } | ||
| return normalizedFieldValue >= min && normalizedFieldValue <= max; | ||
|
|
||
| case ConditionalOperators.IsNotBetween: | ||
| // For "Is Not Between", value should be a string like "min,max" | ||
| if (normalizedFieldValue === null || typeof conditionValue !== "string") { | ||
| return true; | ||
| } | ||
| const [minNot, maxNot] = conditionValue | ||
| .split(",") | ||
| .map((v) => Number(v.trim())); | ||
| if ( | ||
| isNaN(minNot) || | ||
| isNaN(maxNot) || | ||
| typeof normalizedFieldValue !== "number" | ||
| ) { | ||
| return true; | ||
| } | ||
| return normalizedFieldValue < minNot || normalizedFieldValue > maxNot; |
There was a problem hiding this comment.
IsBetween and IsNotBetween return inconsistent defaults for malformed input.
IsBetweenreturnsfalsewhenconditionValueis not a string (line 190-192)IsNotBetweenreturnstruewhenconditionValueis not a string (line 205-207)
This asymmetry could cause unexpected behavior. If the condition value is malformed, both should probably return false (condition not evaluable) or the same default.
🔧 Proposed fix for consistent behavior
case ConditionalOperators.IsNotBetween:
// For "Is Not Between", value should be a string like "min,max"
if (normalizedFieldValue === null || typeof conditionValue !== "string") {
- return true;
+ return false; // Malformed condition should not affect field
}📝 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.
| case ConditionalOperators.IsBetween: | |
| // For "Is Between", value should be a string like "min,max" | |
| if (normalizedFieldValue === null || typeof conditionValue !== "string") { | |
| return false; | |
| } | |
| const [min, max] = conditionValue.split(",").map((v) => Number(v.trim())); | |
| if ( | |
| isNaN(min) || | |
| isNaN(max) || | |
| typeof normalizedFieldValue !== "number" | |
| ) { | |
| return false; | |
| } | |
| return normalizedFieldValue >= min && normalizedFieldValue <= max; | |
| case ConditionalOperators.IsNotBetween: | |
| // For "Is Not Between", value should be a string like "min,max" | |
| if (normalizedFieldValue === null || typeof conditionValue !== "string") { | |
| return true; | |
| } | |
| const [minNot, maxNot] = conditionValue | |
| .split(",") | |
| .map((v) => Number(v.trim())); | |
| if ( | |
| isNaN(minNot) || | |
| isNaN(maxNot) || | |
| typeof normalizedFieldValue !== "number" | |
| ) { | |
| return true; | |
| } | |
| return normalizedFieldValue < minNot || normalizedFieldValue > maxNot; | |
| case ConditionalOperators.IsBetween: | |
| // For "Is Between", value should be a string like "min,max" | |
| if (normalizedFieldValue === null || typeof conditionValue !== "string") { | |
| return false; | |
| } | |
| const [min, max] = conditionValue.split(",").map((v) => Number(v.trim())); | |
| if ( | |
| isNaN(min) || | |
| isNaN(max) || | |
| typeof normalizedFieldValue !== "number" | |
| ) { | |
| return false; | |
| } | |
| return normalizedFieldValue >= min && normalizedFieldValue <= max; | |
| case ConditionalOperators.IsNotBetween: | |
| // For "Is Not Between", value should be a string like "min,max" | |
| if (normalizedFieldValue === null || typeof conditionValue !== "string") { | |
| return false; // Malformed condition should not affect field | |
| } | |
| const [minNot, maxNot] = conditionValue | |
| .split(",") | |
| .map((v) => Number(v.trim())); | |
| if ( | |
| isNaN(minNot) || | |
| isNaN(maxNot) || | |
| typeof normalizedFieldValue !== "number" | |
| ) { | |
| return true; | |
| } | |
| return normalizedFieldValue < minNot || normalizedFieldValue > maxNot; |
🤖 Prompt for AI Agents
In `@frontend/src/utils/conditionals.ts` around lines 188 - 218, The IsBetween and
IsNotBetween branches in evaluate (ConditionalOperators.IsBetween /
ConditionalOperators.IsNotBetween) use inconsistent defaults for malformed
input—IsBetween returns false while IsNotBetween returns true; make them
consistent by treating malformed inputs as unevaluable and returning false in
both cases: update the IsNotBetween checks that currently return true (the
initial typeof conditionValue !== "string" / normalizedFieldValue === null check
and the isNaN/min/max/type checks) to return false, matching the IsBetween
behavior so both operators behave symmetrically when conditionValue or
normalizedFieldValue is invalid.
* Added missing commas for consistency in function parameters. * Removed deprecated "Is Between" and "Is Not Between" conditional checks. * Improved visibility determination logic for fields based on conditional rules.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.