Skip to content

check engine name additional calls issue #1768

Merged
johbaxter merged 9 commits intodevfrom
1691-remove-checkenginename-on-unnecessary-fields
Sep 3, 2025
Merged

check engine name additional calls issue #1768
johbaxter merged 9 commits intodevfrom
1691-remove-checkenginename-on-unnecessary-fields

Conversation

@bannaarisamy-shanmugham-kanini
Copy link
Copy Markdown
Contributor

Description

Fix additional calls on CheckEngine API call, when every fields are being updated

Changes Made

Fixed CheckEngine API call, to work only on the Category name is changed

How to Test

  1. Create a database connection
  2. select any database
  3. Type a name under category name
  4. try to change other fields like host, etc and see the network tab, in earlier versions it will have different API calls, but now it will trigger API call on respective field.

Notes

@bannaarisamy-shanmugham-kanini bannaarisamy-shanmugham-kanini linked an issue Aug 25, 2025 that may be closed by this pull request
2 tasks
@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

check engine name additional calls issue


User description

Description

Fix additional calls on CheckEngine API call, when every fields are being updated

Changes Made

Fixed CheckEngine API call, to work only on the Category name is changed

How to Test

  1. Create a database connection
  2. select any database
  3. Type a name under category name
  4. try to change other fields like host, etc and see the network tab, in earlier versions it will have different API calls, but now it will trigger API call on respective field.

Notes


PR Type

Enhancement, Bug fix


Description

  • Validate only focused field to reduce calls

  • Add focused field tracking across inputs

  • Use strict equality in validations

  • Improve React keys for stability


Diagram Walkthrough

flowchart LR
  form["ImportForm"]
  focus["Track focused field (state)"]
  validate["Validate only when field focused"]
  strictEq["Use strict equality checks"]
  keys["Stable keys for lists"]

  form -- "onFocus/onBlur handlers" --> focus
  focus -- "gate custom validators" --> validate
  form -- "compare values" --> strictEq
  form -- "unique keys for items" --> keys
Loading

File Walkthrough

Relevant files
Enhancement
ImportForm.tsx
Focus-aware validation and strict checks in ImportForm     

packages/client/src/components/import/ImportForm.tsx

  • Introduce focusedField state with onFocus/onBlur handlers.
  • Gate custom validateFormField by currently focused field.
  • Replace loose equality with strict equality checks.
  • Improve React keys for mapped elements; minor cleanups.
+46/-17 

@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /review

@QodoAI-Agent
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Validation Scope

Custom validation now only runs when the field is focused due to the focusedField check. Confirm this does not skip necessary validations on blur, submit, or programmatic changes, and that errors clear/set as expected.

name={val.fieldName}
control={control}
rules={{
	required: val.rules.required,
	validate: {
		...(val.rules?.custom ? {
			checkField: async (fieldVal) => {
				if(focusedField !== val.fieldName) return;
				return validateFormField(
					val,
					fieldVal,
				);
			}
		} : {})
	},
Focus Handling

Multiple inputs add onFocus/onBlur to mutate focusedField. Ensure components like Select propagate focus/blur reliably; otherwise validation gating may be inconsistent across browsers and screen readers.

			onFocus={() => {
				setFocusedField(val.fieldName);
			}}
			onBlur={() => {
				setFocusedField("");
			}}
			// InputProps={{
			//     startAdornment:
			//         val.helperText ? (
			//             <Tooltip
			//                 title={
			//                     val.helperText
			//                 }
			//             >
			//                 <IconButton
			//                     size={
			//                         'small'
			//                     }
			//                 >
			//                     <Help />
			//                 </IconButton>
			//             </Tooltip>
			//         ) : null,
			// }}
			helperText={
				invalid
					? error?.type ===
						"checkField"
						? val.rules
								.custom
								.message
						: error?.message
					: val.helperText
			}
			error={invalid}
			inputProps={{
				"data-testid": `importForm-textField-${val.fieldName}`,
			}}
			{...field}
		></TextField>
	);
} else if (
	val.options.component === "password"
) {
	return (
		<TextField
			id={`${val.fieldName}`}
			type="password"
			fullWidth
			required={
				val.rules.required
			}
			label={val.label}
			disabled={val.disabled}
			value={
				field.value
					? field.value
					: ""
			}
			onChange={(value) =>
				field.onChange(value)
			}
			inputProps={{
				"data-testid": `importForm-textField-${val.fieldName}`,
			}}
			onFocus={() => {
				setFocusedField(val.fieldName);
			}}
			onBlur={() => {
				setFocusedField("");
			}}
			helperText={val.helperText}
		></TextField>
	);
} else if (
	val.options.component === "select"
) {
	return (
		<Select
			fullWidth
			required={
				val.rules.required
			}
			label={val.label}
			disabled={val.disabled}
			value={
				field.value
					? field.value
					: ""
			}
			onChange={(value) => {
				field.onChange(value);
				checkForDisplayRulesSet(
					field,
					value.target.value,
				);
			}}
			SelectProps={{
					onFocus: ()=>{
						setFocusedField(val.fieldName);
					},
					onBlur: () => {
						setFocusedField("");
					}
			}}
			InputProps={{
Key Stability

Keys were changed from index to value-derived strings. Verify that values are unique and stable across renders (e.g., opt.value collisions) to avoid React reconciliation issues.

<Menu.Item
	key={`${opt.value}-${i}`}
	value={
		opt.value
	}
	data-testid={
		opt.display
	}
>
	{
		opt.display
	}
</Menu.Item>

@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /improve

@QodoAI-Agent
Copy link
Copy Markdown

QodoAI-Agent commented Aug 25, 2025

PR Code Suggestions ✨

Latest suggestions up to 80b602b

CategorySuggestion                                                                                                                                    Impact
Possible issue
Return explicit pass on skip

When the condition fails, the validator returns undefined, which react-hook-form
treats as a pass and also hides potential errors. Explicitly return true to indicate
a valid state when skipping validation. This prevents ambiguous outcomes and ensures
consistent error handling.

packages/client/src/components/import/ImportForm.tsx [726-739]

 validate: {
-	...(val.rules?.custom ? {
-		checkField: async (fieldVal) => {
-			if(_lastField.current.lastFocussedField === val.fieldName) {
-				return validateFormField(
-					val,
-					fieldVal,
-				);
+	...(val.rules?.custom
+		? {
+				checkField: async (fieldVal) => {
+					if (_lastField.current.lastFocussedField === val.fieldName) {
+						return validateFormField(val, fieldVal);
+					}
+					return true; // explicitly pass when not the focused field
+				},
 			}
-		}
-	} : {})
+		: {}),
 },
Suggestion importance[1-10]: 7

__

Why: Correctly notes that the validator path may return undefined, which can be ambiguous; explicitly returning true improves clarity and consistency without changing intended logic.

Medium
Avoid blur-driven validation churn

Using "onBlur" validation alongside async validators can trigger unintended API
calls when focus changes between fields. Switch validation mode to "onSubmit" and
drive targeted async validation via explicit blur handlers using _lastField to
throttle checks. This prevents cross-field blurs from firing validators and reduces
duplicate requests.

packages/client/src/components/import/ImportForm.tsx [92-109]

 const {
 		control,
 		handleSubmit,
 		reset,
 		watch,
 		setValue,
 		getValues,
 		setError,
 		formState: { isValid },
 	} = useForm({
-		mode: "onBlur",
+		mode: "onSubmit",
 		defaultValues: defaultFields.reduce((acc, field) => {
 			acc[field.fieldName] = field.defaultValue || "";
 			return acc;
 		}, {}),
 	});
-const _lastField = useRef({lastFocussedField: "", lastFocussedValue:""});
+const _lastField = useRef({ lastFocussedField: "", lastFocussedValue: "" });
Suggestion importance[1-10]: 4

__

Why: The change to onBlur appears intentional to support new focus-based logic; reverting to onSubmit may undermine that behavior. While it could reduce API calls, it conflicts with the PR’s direction and lacks proof of an actual bug.

Low
General
Focus invalid field on error

Provide the shouldFocus option to direct user attention to the invalid field after
an async check fails. This restores accessibility and prevents silent failures when
the field is offscreen or unchanged.

packages/client/src/components/import/ImportForm.tsx [679-683]

 if (output.exists) {
-	// setFocus(field.fieldName);
-	setError(field.fieldName, {message: field.rules.custom.message, type: 'checkField'});
+	setError(field.fieldName, { message: field.rules.custom.message, type: "checkField" }, { shouldFocus: true });
 	return false;
 }
Suggestion importance[1-10]: 6

__

Why: Using setError with shouldFocus restores the previous focus behavior and improves UX/accessibility; it’s a reasonable enhancement with moderate impact.

Low

Previous suggestions

Suggestions up to commit 7a7683f
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guarantee validator return type

Ensure the validator always returns a boolean or string; returning undefined when
the field is not focused can cause inconsistent validation states. Explicitly return
true when the field isn't focused so RHF treats the field as valid. Also keep strict
equality.

packages/client/src/components/import/ImportForm.tsx [726-737]

 validate: {
-  ...(val.rules?.custom ? {
-    checkField: async (fieldVal) => {
-      if(focusedField !== val.fieldName) return;
-      return validateFormField(
-        val,
-        fieldVal,
-      );
-    }
-  } : {})
+  ...(val.rules?.custom
+    ? {
+        checkField: async (fieldVal) => {
+          if (focusedField !== val.fieldName) return true;
+          return validateFormField(val, fieldVal);
+        },
+      }
+    : {}),
 },
Suggestion importance[1-10]: 8

__

Why: Correctly notes that returning undefined from checkField when the field isn't focused can cause inconsistent RHF validation; returning true preserves validity. Matches the new hunk code context and offers a safe, impactful fix.

Medium
General
Use stable React keys

Using a changing label in React keys can cause unnecessary re-mounts and lost field
state if labels update. Remove val.label from the key and rely on stable identifiers
to preserve component and form state.

packages/client/src/components/import/ImportForm.tsx [721]

-<StyledKeyValue key={`${val.fieldName}-${val.label}-${i}`}>
+<StyledKeyValue key={`${val.fieldName}-${i}`}>
Suggestion importance[1-10]: 6

__

Why: Removing val.label from the key avoids remounts if labels change and relies on more stable identifiers. The existing line is correctly located; improvement is reasonable but moderate in impact.

Low
Ensure reliable focus handling

MUI Select fires focus/blur on the input element; attaching these to SelectProps may
not reliably trigger. Move handlers to onFocus/onBlur of the wrapping field
component to ensure focusedField is set consistently.

packages/client/src/components/import/ImportForm.tsx [870-877]

-SelectProps={{
-    onFocus: ()=>{
-      setFocusedField(val.fieldName);
-    },
-    onBlur: () => {
-      setFocusedField("");
-    }
+onFocus={() => {
+  setFocusedField(val.fieldName);
 }}
+onBlur={() => {
+  setFocusedField("");
+}}
+SelectProps={{}}
Suggestion importance[1-10]: 5

__

Why: Observes that attaching focus/blur via SelectProps may be unreliable; suggesting moving handlers to the wrapping field aligns with other fields using direct onFocus/onBlur. It's a reasonable UX robustness tweak but not critical.

Low
Suggestions up to commit bd0fecb
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix select value handling

The onChange handler assumes a DOM event and uses value.target.value, which can
break if Select passes the value directly. Use the value parameter consistently to
avoid undefined access and incorrect updates. This fixes potential runtime errors on
selection.

packages/client/src/components/import/ImportForm.tsx [862-881]

 <Select
   ...
   onChange={(value) => {
     field.onChange(value);
-    checkForDisplayRulesSet(
-      field,
-      value.target.value,
-    );
+    checkForDisplayRulesSet(field, value);
   }}
   SelectProps={{
-    onFocus: ()=>{
+    onFocus: () => {
       setFocusedField(val.fieldName);
     },
     onBlur: () => {
       setFocusedField("");
     }
   }}
   InputProps={{
     "data-testid": `importForm-selectField-${val.fieldName}`,
   }}
   helperText={val.helperText}
 >
Suggestion importance[1-10]: 8

__

Why: The current code assumes an event object and accesses value.target.value, which is likely incorrect for a custom Select; using the value directly avoids runtime errors and ensures display rules update correctly.

Medium
Ensure validator returns boolean

Avoid returning undefined from the validator when the field isn't focused, as this
can incorrectly flag the field as invalid. Explicitly return true to indicate no
error when skipping validation. This prevents intermittent validation failures and
ensures predictable behavior.

packages/client/src/components/import/ImportForm.tsx [109-137]

-const [focusedField, setFocusedField] = useState("");
+const [focusedField, setFocusedField] = useState<string>("");
 
 ...
 
 validate: {
   ...(val.rules?.custom ? {
     checkField: async (fieldVal) => {
-      if(focusedField !== val.fieldName) return;
-      return validateFormField(
-        val,
-        fieldVal,
-      );
+      if (focusedField !== val.fieldName) return true;
+      return validateFormField(val, fieldVal);
     }
   } : {})
 },
Suggestion importance[1-10]: 7

__

Why: Returning true instead of undefined when skipping validation is correct and prevents false negatives in react-hook-form. The change is small but improves reliability of custom validation logic tied to focus state.

Medium
General
Prevent stale focus after drop

Setting focusedField on file drop without clearing it can leave the form in a
"focused" state and suppress other validations. Clear focus when the drop
interaction ends to avoid stale focus affecting other fields. This ensures
validations run for subsequent fields.

packages/client/src/components/import/ImportForm.tsx [950-959]

 <FileDropzone
   value={field.value}
   disabled={false}
-  onChange={(
-    newValues,
-  ) => {
-    field.onChange(
-      newValues,
-    );
+  onChange={(newValues) => {
+    field.onChange(newValues);
     setFocusedField(val.fieldName);
+    // Clear focus after processing file change to avoid stale focus
+    setTimeout(() => setFocusedField(""), 0);
   }}
 />
Suggestion importance[1-10]: 5

__

Why: Clearing focusedField after handling FileDropzone changes can prevent unintended validation suppression; the approach is reasonable though impact is moderate and context-dependent.

Low

@BhaktiKanini
Copy link
Copy Markdown

BhaktiKanini commented Aug 26, 2025

hi @bannaarisamy-shanmugham-kanini I tested the model, vector, and database catalog as per the ticket, and everything is working fine. However, when a large number of characters are entered for Catalog name, the system calls the API multiple times, which is not the expected behavior
#1773
CC: @rameshpaulraj

@bannaarisamy-shanmugham-kanini
Copy link
Copy Markdown
Contributor Author

Hi @BhaktiKanini , This issue is fixed and latest code is now available for test. Kindly verify.

cc: @rameshpaulraj

@BhaktiKanini
Copy link
Copy Markdown

Hi @bannaarisamy-shanmugham-kanini, I have retested this issue, but it still persists. When we edit the catalog name and click outside, the API is being called twice
CC: @rameshpaulraj

@bannaarisamy-shanmugham-kanini
Copy link
Copy Markdown
Contributor Author

Hi @BhaktiKanini , Please take a pull and test it. this time, API call is limited correctly based on the value.

cc: @rameshpaulraj

@BhaktiKanini
Copy link
Copy Markdown

@bannaarisamy-shanmugham-kanini I have tested this and it is working fine as expected
CC: @rameshpaulraj @Pranchaudhari

@johbaxter johbaxter merged commit fcfe3fd into dev Sep 3, 2025
3 checks passed
@johbaxter johbaxter deleted the 1691-remove-checkenginename-on-unnecessary-fields branch September 3, 2025 15:50
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 3, 2025

@CodiumAI-Agent /update_changelog

@QodoAI-Agent
Copy link
Copy Markdown

Changelog updates: 🔄

2025-09-03 *

Fixed

  • Reduced unnecessary CheckEngine API calls by validating only when the category name changes and on blur events.

to commit the new content to the CHANGELOG.md file, please type:
'/update_changelog --pr_update_changelog.push_changelog_changes=true'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove CheckEngineName on Unnecessary Fields

4 participants