James/b/updated qb branch

[James-Baloyi]

Summary by CodeRabbit

Release Notes

• New Features
  • Enhanced Query Builder with custom tree rendering, richer empty-state UI, conjunction/relation selectors, drag-handle placeholders, and added widgets for Yes/No and “ignore if unassigned”, including function/expression editing and the new “Show JSON testing tools” option.
  • Improved expression editor and property selection (placeholders/variants and clearer option titles).
• Bug Fixes
  • More accurate JSON-logic processing and safer mustache evaluation (typed constants, string/date/numeric comparison fixes, added evaluate operator).
• Tests
  • Expanded Query Builder and editor metadata tests; added conditional matchMedia polyfill.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@shesha-reactjs/src/components/queryBuilder/customBuilder/index.tsx`:
- Around line 1095-1098: The canDelete and canDrag variables currently only
check the readOnly flag, but should use the same group-readonly gate as relation
editing controls to ensure consistency with immutableGroupsMode. Replace the
readOnly checks in both canDelete and canDrag definitions with
getGroupReadonly(config, readOnly) to match the gating logic used in relation
editing. Apply this same change to the second occurrence of similar logic at
lines 1121-1126 to ensure all structural actions (delete, drag, add, and
relation editing) use the unified group-readonly state.

In `@shesha-reactjs/src/components/queryBuilder/queryBuilderContent.tsx`:
- Around line 63-64: Replace the type cast in the `children1` variable access
with a type guard function that validates the value has a size property before
accessing it. Create a reusable type guard function like `hasSize` that checks
if the value is an object with a numeric size property, then use that guard in
the `hasRules` boolean expression instead of the `as { size: number }` cast. For
line 81, replace the chained type casts in the `QbUtils.getTree()` call (the
pattern `as unknown as IPlainTreeNode) as unknown as JsonTree`) with an explicit
type guard function that properly validates the structure before accessing it,
or update the type contracts for `normalizeTreeForJsonLogic` to accept the
concrete types returned by `QbUtils.getTree()` to eliminate the need for
multiple casts.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 54bb1a3c-17f0-4ffe-a479-b8f830955caf

📥 Commits

Reviewing files that changed from the base of the PR and between b955bce and 17a28ec.

⛔ Files ignored due to path filters (1)

• shesha-reactjs/eslint.config.mjs is excluded by none and included by none

📒 Files selected for processing (10)

• shesha-reactjs/src/components/queryBuilder/conjunctionSelect/index.tsx
• shesha-reactjs/src/components/queryBuilder/customBuilder/index.tsx
• shesha-reactjs/src/components/queryBuilder/groupDragAction/index.tsx
• shesha-reactjs/src/components/queryBuilder/itemWithRelation/index.tsx
• shesha-reactjs/src/components/queryBuilder/queryBuilderContent.tsx
• shesha-reactjs/src/components/queryBuilder/ruleDragHandlePlaceholder/index.tsx
• shesha-reactjs/src/components/queryBuilder/sourceSelector/index.tsx
• shesha-reactjs/src/components/queryBuilder/treeRelations.ts
• shesha-reactjs/src/components/queryBuilder/widgets/booleanButtonSelect/index.tsx
• shesha-reactjs/src/components/queryBuilder/widgets/refListDropDown/index.tsx

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (7)

shesha-reactjs/src/components/queryBuilder/treeRelations.ts (1)

110-118: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve existing group negation during relation normalization.

This rewrite always exports not: false, so a loaded or edited negated group that also contains inline relations loses its negation before jsonLogicFormat serializes the filter.

🐛 Proposed fix

+  const exportedProperties = strippedProperties ?? {};
+
   return {
     ...normalizedNode,
     properties: {
-      ...stripRelationProperty(node.properties),
+      ...exportedProperties,
       conjunction: clauseNodes.length > 1 ? 'OR' : 'AND',
-      not: false,
+      not: exportedProperties.not === true,
     },
     children1: clauseNodes,
   };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shesha-reactjs/src/components/queryBuilder/treeRelations.ts` around lines 110
- 118, The issue is that the normalization logic in the return statement is
hardcoding `not: false`, which overwrites any existing negation state from the
original node and causes negated groups containing inline relations to lose
their negation. To fix this, preserve the original negation value from the input
node by replacing the hardcoded `not: false` with the actual negation state from
the node object (access the negation property from the original node parameter
to retain its current value).

shesha-reactjs/src/designer-components/queryBuilder/queryBuilderField.tsx (2)

72-80: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Block OK while the JSON editor has a parse error.

After applyJson fails, jsonInputError is set but OK still commits the previous draftLogic and closes the modal, silently discarding the invalid JSON the user is looking at.

🐛 Proposed fix

   const onOkClick = (): void => {
     if (builderErrors.length > 0) {
       setActiveTab(queryBuilderTabKey);
       return;
     }
+
+    if (jsonInputError) {
+      setActiveTab(jsonInputTabKey);
+      return;
+    }
 
     if (props.onChange)
       props.onChange(draftLogic ?? null);
@@
         )}
         onOk={onOkClick}
-        okButtonProps={{ hidden: readOnly }}
+        okButtonProps={{ hidden: readOnly, disabled: builderErrors.length > 0 || Boolean(jsonInputError) }}
         onCancel={() => setModalVisible(false)}

Also applies to: 116-119, 230-231

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shesha-reactjs/src/designer-components/queryBuilder/queryBuilderField.tsx`
around lines 72 - 80, The onOkClick function does not check for JSON parsing
errors before committing changes and closing the modal. Add a check for
jsonInputError in the onOkClick function (around lines 72-80) alongside the
existing builderErrors check. If jsonInputError exists, set the active tab to
the appropriate JSON editor tab key and return early to prevent the modal from
closing and discarding the invalid JSON the user is viewing. Apply the same fix
to the other locations mentioned (lines 116-119 and 230-231) where
OK/confirmation actions occur.

---

187-206: ⚠️ Potential issue | 🟡 Minor

Keep the JSON preview Collapse consistently controlled.

The Collapse switches between controlled and uncontrolled modes: activeKey is only passed when jsonExpanded is true. This inconsistency can cause React warnings and unpredictable state synchronization. Additionally, since actionButtons is used as the panel label, clicking them may unintentionally toggle the preview due to event propagation. The expand icon spans have onClick handlers that are redundant with a proper onChange callback.

Pass activeKey in both states and implement an onChange handler to keep the component consistently controlled. Also wrap actionButtons in the label with stopPropagation() to prevent unintended toggling when interacting with action buttons.

Suggested approach

-  const onExpandClick = (): void => {
-    setJsonExpanded(!jsonExpanded);
+  const onCollapseChange = (key: string | string[]): void => {
+    const activeKeys = Array.isArray(key) ? key : [key];
+    setJsonExpanded(activeKeys.includes('1'));
   };
@@
       <Collapse
         className={styles.shaQueryBuilderField}
-        {...(jsonExpanded ? { activeKey: '1' } : {})}
+        activeKey={jsonExpanded ? ['1'] : []}
+        onChange={onCollapseChange}
         expandIconPlacement="end"
         bordered={false}
         ghost={true}
         expandIcon={({ isActive }) =>
           isActive ? (
-            <span onClick={onExpandClick}>
+            <span>
               hide json <CaretRightOutlined rotate={90} />
             </span>
           ) : (
-            <span onClick={onExpandClick}>
+            <span>
               show json <CaretRightOutlined rotate={0} />
             </span>
           )}
         items={[
           {
             key: '1',
-            label: actionButtons,
+            label: <div onClick={(event) => event.stopPropagation()}>{actionButtons}</div>,

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shesha-reactjs/src/designer-components/queryBuilder/queryBuilderField.tsx`
around lines 187 - 206, The Collapse component in queryBuilderField.tsx is
inconsistently controlled, switching between controlled and uncontrolled modes
based on jsonExpanded state, which causes React warnings. The activeKey prop
should always be passed (either with value '1' when jsonExpanded is true or
empty/undefined when false) to keep the component consistently controlled.
Replace the redundant onClick handlers in the expand icon spans with a proper
onChange callback that updates the jsonExpanded state. Additionally, wrap the
actionButtons label content with event.stopPropagation() to prevent unintended
Collapse toggling when users interact with those action buttons.

shesha-reactjs/src/components/queryBuilder/widgets/refListDropDown/index.tsx (1)

12-33: ⚠️ Potential issue | 🟡 Minor

Remove the type cast from the factory function.

The explicit cast as FactoryWithContext<NumberWidgetProps> on line 33 is unnecessary and masks potential type mismatches. The widget type annotation (RefListDropdownWidgetType) on line 8 should already enforce the correct factory signature. Removing the cast aligns with the established pattern in the codebase where other widgets rely on the type annotation without casting (see entityAutocomplete, field, booleanButtonSelect, etc.).

Per coding guidelines: "Prefer type guards over type casting for type checking" and "Leverage TypeScript to its full potential as a type system."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shesha-reactjs/src/components/queryBuilder/widgets/refListDropDown/index.tsx`
around lines 12 - 33, Remove the unnecessary type cast `as
FactoryWithContext<NumberWidgetProps>` from the end of the factory function
definition. The factory function assigned to the factory property already has
its type enforced by the `RefListDropdownWidgetType` annotation, so the explicit
cast is redundant. This aligns with the pattern used by other widgets in the
codebase like `entityAutocomplete`, `field`, and `booleanButtonSelect` which
rely on their type annotations without casting.

Source: Coding guidelines

shesha-reactjs/src/components/queryBuilder/fieldAutocomplete/index.tsx (1)

37-69: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve the caller’s readonly state.

QueryRuleRow passes readonly: fieldReadonly, but this component ignores it and only checks immutableFieldsMode, so read-only builders can still expose an editable field selector.

🐛 Proposed fix

-  const readOnly = isDefined(config) && config.settings.immutableFieldsMode === true;
+  const readOnly = Boolean(
+    props.readonly ||
+    (isDefined(config) && config.settings.immutableFieldsMode === true),
+  );

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shesha-reactjs/src/components/queryBuilder/fieldAutocomplete/index.tsx`
around lines 37 - 69, The component is not respecting the readonly state passed
by the caller (QueryRuleRow). The readOnly variable currently only checks
config.settings.immutableFieldsMode but ignores any readonly prop passed to this
component. Add readonly as a prop to the component and update the readOnly
variable assignment to combine both the readonly prop from the caller and the
immutableFieldsMode setting using logical OR, so that readOnly is true if either
condition is met. This ensures that when the caller explicitly passes readonly,
the field selector respects that constraint regardless of the
immutableFieldsMode setting.

shesha-reactjs/src/components/queryBuilder/funcSelect/index.tsx (1)

16-118: 🧹 Nitpick | 🔵 Trivial

Remove the redundant type assertion on the FuncSelect export.

The export declaration at line 16 already specifies the type as FactoryWithContext<FieldProps>. The trailing as FactoryWithContext<FieldProps> cast at line 118 is unnecessary and conflicts with the coding guideline to prefer type guards over type casting.

♻️ Proposed refactor

-export const FuncSelect: FactoryWithContext<FieldProps> = ((props) => {
+export const FuncSelect: FactoryWithContext<FieldProps> = (props) => {
   const onChange = (key: string | undefined): void => {
     props.setField(key ?? "");
   };
@@
-}) as FactoryWithContext<FieldProps>;
+};

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shesha-reactjs/src/components/queryBuilder/funcSelect/index.tsx` around lines
16 - 118, The FuncSelect export declaration has a redundant type assertion. The
type FactoryWithContext<FieldProps> is already explicitly declared in the
variable declaration on line 16, so the trailing as
FactoryWithContext<FieldProps> type cast at the end of the function definition
is unnecessary and should be removed. Simply remove the as
FactoryWithContext<FieldProps> cast at the end of the arrow function closing
parenthesis while keeping the type annotation on the variable declaration
itself.

Source: Coding guidelines

shesha-reactjs/src/components/queryBuilder/itemWithRelation/index.tsx (1)

121-139: ⚠️ Potential issue | 🟡 Minor

Replace assertions with type guards for tree operations (lines 134–138); remove redundant assertion at line 123.

Line 123 cast is redundant—definition is already typed as { label?: string } from the Record value type. Remove it.

Lines 134 and 138 rely on assertions for tree. Replace with a type guard:

♻️ Type-guard refactor

+type TreeWithSetIn = {
+  setIn: (path: (string | number)[], val: unknown) => unknown;
+};
+
+const hasSetIn = (value: unknown): value is TreeWithSetIn =>
+  typeof value === 'object' &&
+  value !== null &&
+  'setIn' in value &&
+  typeof value.setIn === 'function';
+
   const conjunctionOptions = Object.entries(config.conjunctions ?? {}).map(([key, definition]) => ({
     value: key,
-    label: (definition as { label?: string }).label ?? key,
+    label: definition.label ?? key,
   }));
   const selectOptions = conjunctionOptions;

@@
   const handleChange = (value: string): void => {
-    if (!tree || typeof (tree as { setIn?: unknown }).setIn !== 'function' || !actions.setTree)
+    if (!hasSetIn(tree) || !actions.setTree)
       return;

     const nodeTreePath = getNodeTreePath(itemPath);
-    const updatedTree = (tree as { setIn: (path: (string | number)[], val: unknown) => unknown }).setIn(
+    const updatedTree = tree.setIn(
       [...nodeTreePath, 'properties', '__relation'],
       value,
     );

Aligns with TypeScript best practices: prefer type guards over assertions, and avoid redundant casts when types are already inferred.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shesha-reactjs/src/components/queryBuilder/itemWithRelation/index.tsx` around
lines 121 - 139, Remove the redundant type assertion on the definition variable
in the conjunctionOptions.map() call, as definition is already properly typed as
{label?: string} from the Record value type. In the handleChange function,
replace the multiple type assertions for tree operations with a proper type
guard that checks if tree is an object with a setIn function, narrows the type
accordingly, and then uses that narrowed type for the subsequent setIn call
instead of recasting with assertions. This improves type safety and aligns with
TypeScript best practices.

Source: Coding guidelines

♻️ Duplicate comments (1)

shesha-reactjs/src/components/queryBuilder/conjunctionSelect/index.tsx (1)

46-46: ⚠️ Potential issue | 🟠 Major

Replace unsafe non-null assertion with optional chaining, consistent with other queryBuilder components.

Line 46 uses config! (non-null assertion) and repeated type casting, which bypasses nullability checks and contradicts the pattern used elsewhere in the queryBuilder folder. The funcSelect component (line 53) correctly uses optional chaining: config?.settings.renderSize. Adopt the same safe pattern here to prevent potential runtime crashes and align with the project guideline: "Prefer Type Guards Over Type Casting."

Suggested patch

-        size={(config!.settings as SettingsWithRenderSize).renderSize === 'medium' ? 'middle' : (config!.settings as SettingsWithRenderSize).renderSize as 'small' | 'large' | undefined}
+        size={config?.settings?.renderSize === 'medium' ? 'middle' : config?.settings?.renderSize}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shesha-reactjs/src/components/queryBuilder/conjunctionSelect/index.tsx` at
line 46, Replace the unsafe non-null assertion operator on the `size` prop
configuration in the conjunctionSelect component. Instead of using
`config!.settings` with repeated type casting to SettingsWithRenderSize, use
safe optional chaining with `config?.settings` to access the renderSize
property, matching the pattern already established in the funcSelect component
on line 53. This ensures type safety and prevents potential runtime crashes when
config might be undefined or null.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@shesha-reactjs/src/components/queryBuilder/fieldAutocomplete/index.tsx`:
- Around line 37-69: The component is not respecting the readonly state passed
by the caller (QueryRuleRow). The readOnly variable currently only checks
config.settings.immutableFieldsMode but ignores any readonly prop passed to this
component. Add readonly as a prop to the component and update the readOnly
variable assignment to combine both the readonly prop from the caller and the
immutableFieldsMode setting using logical OR, so that readOnly is true if either
condition is met. This ensures that when the caller explicitly passes readonly,
the field selector respects that constraint regardless of the
immutableFieldsMode setting.

In `@shesha-reactjs/src/components/queryBuilder/funcSelect/index.tsx`:
- Around line 16-118: The FuncSelect export declaration has a redundant type
assertion. The type FactoryWithContext<FieldProps> is already explicitly
declared in the variable declaration on line 16, so the trailing as
FactoryWithContext<FieldProps> type cast at the end of the function definition
is unnecessary and should be removed. Simply remove the as
FactoryWithContext<FieldProps> cast at the end of the arrow function closing
parenthesis while keeping the type annotation on the variable declaration
itself.

In `@shesha-reactjs/src/components/queryBuilder/itemWithRelation/index.tsx`:
- Around line 121-139: Remove the redundant type assertion on the definition
variable in the conjunctionOptions.map() call, as definition is already properly
typed as {label?: string} from the Record value type. In the handleChange
function, replace the multiple type assertions for tree operations with a proper
type guard that checks if tree is an object with a setIn function, narrows the
type accordingly, and then uses that narrowed type for the subsequent setIn call
instead of recasting with assertions. This improves type safety and aligns with
TypeScript best practices.

In `@shesha-reactjs/src/components/queryBuilder/treeRelations.ts`:
- Around line 110-118: The issue is that the normalization logic in the return
statement is hardcoding `not: false`, which overwrites any existing negation
state from the original node and causes negated groups containing inline
relations to lose their negation. To fix this, preserve the original negation
value from the input node by replacing the hardcoded `not: false` with the
actual negation state from the node object (access the negation property from
the original node parameter to retain its current value).

In
`@shesha-reactjs/src/components/queryBuilder/widgets/refListDropDown/index.tsx`:
- Around line 12-33: Remove the unnecessary type cast `as
FactoryWithContext<NumberWidgetProps>` from the end of the factory function
definition. The factory function assigned to the factory property already has
its type enforced by the `RefListDropdownWidgetType` annotation, so the explicit
cast is redundant. This aligns with the pattern used by other widgets in the
codebase like `entityAutocomplete`, `field`, and `booleanButtonSelect` which
rely on their type annotations without casting.

In `@shesha-reactjs/src/designer-components/queryBuilder/queryBuilderField.tsx`:
- Around line 72-80: The onOkClick function does not check for JSON parsing
errors before committing changes and closing the modal. Add a check for
jsonInputError in the onOkClick function (around lines 72-80) alongside the
existing builderErrors check. If jsonInputError exists, set the active tab to
the appropriate JSON editor tab key and return early to prevent the modal from
closing and discarding the invalid JSON the user is viewing. Apply the same fix
to the other locations mentioned (lines 116-119 and 230-231) where
OK/confirmation actions occur.
- Around line 187-206: The Collapse component in queryBuilderField.tsx is
inconsistently controlled, switching between controlled and uncontrolled modes
based on jsonExpanded state, which causes React warnings. The activeKey prop
should always be passed (either with value '1' when jsonExpanded is true or
empty/undefined when false) to keep the component consistently controlled.
Replace the redundant onClick handlers in the expand icon spans with a proper
onChange callback that updates the jsonExpanded state. Additionally, wrap the
actionButtons label content with event.stopPropagation() to prevent unintended
Collapse toggling when users interact with those action buttons.

---

Duplicate comments:
In `@shesha-reactjs/src/components/queryBuilder/conjunctionSelect/index.tsx`:
- Line 46: Replace the unsafe non-null assertion operator on the `size` prop
configuration in the conjunctionSelect component. Instead of using
`config!.settings` with repeated type casting to SettingsWithRenderSize, use
safe optional chaining with `config?.settings` to access the renderSize
property, matching the pattern already established in the funcSelect component
on line 53. This ensures type safety and prevents potential runtime crashes when
config might be undefined or null.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9c0a770b-e41b-406d-a805-207acacba245

📥 Commits

Reviewing files that changed from the base of the PR and between b47a305 and b0cc8d8.

📒 Files selected for processing (19)

• shesha-reactjs/src/components/propertyAutocomplete/propertySelect.tsx
• shesha-reactjs/src/components/queryBuilder/config.ts
• shesha-reactjs/src/components/queryBuilder/conjunctionSelect/index.tsx
• shesha-reactjs/src/components/queryBuilder/customBuilder/index.tsx
• shesha-reactjs/src/components/queryBuilder/emptyRulePlaceholders/index.tsx
• shesha-reactjs/src/components/queryBuilder/fieldAutocomplete/index.tsx
• shesha-reactjs/src/components/queryBuilder/funcSelect/index.tsx
• shesha-reactjs/src/components/queryBuilder/groupEmptyState/index.tsx
• shesha-reactjs/src/components/queryBuilder/groupEmptyState/queryRuleElement.tsx
• shesha-reactjs/src/components/queryBuilder/itemPrefix/index.tsx
• shesha-reactjs/src/components/queryBuilder/itemWithRelation/index.tsx
• shesha-reactjs/src/components/queryBuilder/ruleDragHandlePlaceholder/index.tsx
• shesha-reactjs/src/components/queryBuilder/sourceSelector/index.tsx
• shesha-reactjs/src/components/queryBuilder/treeRelations.ts
• shesha-reactjs/src/components/queryBuilder/widgets/booleanButtonSelect/index.tsx
• shesha-reactjs/src/components/queryBuilder/widgets/ignoreIfUnassigned/index.tsx
• shesha-reactjs/src/components/queryBuilder/widgets/refListDropDown/index.tsx
• shesha-reactjs/src/components/refListDropDown/genericRefListDropDown.tsx
• shesha-reactjs/src/designer-components/queryBuilder/queryBuilderField.tsx

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

shesha-core/src/Shesha.Framework/JsonLogic/JsonLogic2LinqConverter.cs (1)

685-703: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Silent failure when required=true but expression is empty.

When required defaults to true and expression is null or whitespace, the code returns Expression.Constant(null, typeof(string)) instead of throwing an error. This could lead to unexpected query results where a "required" expression silently becomes a null comparison.

Consider throwing when a required expression is empty:

🐛 Proposed fix

 case JsOperators.Evaluate:
     {
         var evalArg = `@operator.Arguments.FirstOrDefault`();
         if (evalArg is JObject evalObj)
         {
             var expression = evalObj.Value<string>("expression");
             var required = evalObj.Value<bool?>("required") ?? true;

             if (string.IsNullOrWhiteSpace(expression) && !required)
                 return null;

+            if (string.IsNullOrWhiteSpace(expression) && required)
+                throw new JsonLogicParsingFailedException($"'{JsOperators.Evaluate}' node has empty expression but 'required' is true", rule);
+
             return Expression.Constant(expression, typeof(string));
         }
         throw new JsonLogicParsingFailedException("Invalid 'evaluate' node format", rule);
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shesha-core/src/Shesha.Framework/JsonLogic/JsonLogic2LinqConverter.cs` around
lines 685 - 703, In the JsOperators.Evaluate case handler, the current code only
returns null when the expression is empty and required is false, but silently
passes through when required is true and expression is empty. Add a validation
check after extracting the expression and required values to throw a
JsonLogicParsingFailedException when the expression is null or whitespace AND
required is true, ensuring that required expressions with empty values are
properly caught and reported as parsing errors rather than silently becoming
null constants.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@shesha-reactjs/src/providers/form/utils.ts`:
- Around line 999-1000: In the early return statement within the
isNullOrWhiteSpace check, change the unevaluatedExpressions value from undefined
to an empty array to maintain type consistency with the normal return path at
line 1041. This ensures downstream consumers always receive a string array type
for unevaluatedExpressions rather than having to handle both undefined and
string[] types.

---

Outside diff comments:
In `@shesha-core/src/Shesha.Framework/JsonLogic/JsonLogic2LinqConverter.cs`:
- Around line 685-703: In the JsOperators.Evaluate case handler, the current
code only returns null when the expression is empty and required is false, but
silently passes through when required is true and expression is empty. Add a
validation check after extracting the expression and required values to throw a
JsonLogicParsingFailedException when the expression is null or whitespace AND
required is true, ensuring that required expressions with empty values are
properly caught and reported as parsing errors rather than silently becoming
null constants.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: fdf9b0f6-284f-4904-93aa-a7ab48a7a200

📥 Commits

Reviewing files that changed from the base of the PR and between b0cc8d8 and b9d4d1e.

⛔ Files ignored due to path filters (1)

• shesha-core/test/Shesha.Tests/JsonLogic/JsonLogic2LinqConverter_ValueTests.cs is excluded by none and included by none

📒 Files selected for processing (6)

• shesha-core/src/Shesha.Framework/JsonLogic/JsonLogic2LinqConverter.cs
• shesha-reactjs/src/components/queryBuilder/config.ts
• shesha-reactjs/src/components/queryBuilder/customBuilder/index.tsx
• shesha-reactjs/src/components/queryBuilder/groupDragAction/index.tsx
• shesha-reactjs/src/components/queryBuilder/widgets/booleanButtonSelect/index.tsx
• shesha-reactjs/src/providers/form/utils.ts

💤 Files with no reviewable changes (1)

• shesha-reactjs/src/components/queryBuilder/config.ts

[IvanIlyichev]

Hi @James-Baloyi. Please ensure all comments are resolved and marked accordingly. Also, note that the value editors on the query builder are currently broken - they trigger an infinite loop and hang the browser tab. Given that, please test all changes locally before submitting, as repeated iterations with the query builder tend to cause crashes or freezes.

[IvanIlyichev]

Hi @James-Baloyi. Please check the following:

1. Remove all usages of as unknown as from the code. This pattern is unsafe, and we should stick to strongly typed code wherever possible. I temporarily used it in a few specific cases during the recent large refactoring just to keep things moving, but I plan to add a linter rule later to prevent these unsafe casts from recurring.

2. The horizontal scrollbar negatively affects the user experience. When configuring groups, I actually couldn't even find the drag‑and‑drop handle at first because it was hidden—and that handle is required for the configuration (see the attached image).

3. Proposed changes hide functionality available in the current implementation

• Functions like Uppercase and Lowercase are not available

- In preveious version the `between` option allows to select source type for both `from` and `to` but current implementation uses single selector for source type. - Arguments editor is not available

[James-Baloyi]

Hi @IvanIlyichev

I've addressed the first two items, and then had a chat with @ihouvet - the suggestion is to remove function altogether and replace it with expression editor. I've attached a screenshot below.

[IvanIlyichev]

Hi @IvanIlyichev

I've addressed the first two items, and then had a chat with @ihouvet - the suggestion is to remove function altogether and replace it with expression editor. I've attached a screenshot below.

Hi @James-Baloyi. In the current implementation, all functions used in the query builder are translated into LINQ expressions and then, via the ORM, into SQL queries. Replacing this with evaluating Mustache expressions on the frontend is not feasible for several reasons:

1. Potential time zone conflicts between the frontend and backend.
2. Data type conversion issues.
3. Performance regression.

Additionally, Mustache is a logic-less templating language, so it cannot accept arguments like proper functions. As I mentioned in my previous comment, relative date evaluation (e.g., now+n days) is essential for filters such as "last 5 days" and Mustache simply does not cover such use cases.

PS: Next time, please use the "re-request review" function, it will help speed up the code review process.