Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ballerine Validator(WIP) #2874

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@
"clsx": "^1.2.1",
"cmdk": "^0.2.0",
"dayjs": "^1.11.6",
"email-validator": "^2.0.4",
"i18n-iso-countries": "^7.6.0",
"json-logic-js": "^2.0.2",
"lodash": "^4.17.21",
"lucide-react": "^0.144.0",
"react": "^18.0.37",
Expand All @@ -76,6 +78,9 @@
"@storybook/react": "^7.0.26",
"@storybook/react-vite": "^7.0.26",
"@storybook/testing-library": "^0.0.14-next.2",
"@testing-library/dom": "^10.4.0",
"@testing-library/react": "^13.3.0",
"@types/json-logic-js": "^2.0.1",
"@types/lodash": "^4.14.191",
"@types/node": "^20.4.1",
"@types/react": "^18.0.37",
Expand Down
1 change: 1 addition & 0 deletions packages/ui/src/components/organisms/Form/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
./_Validator
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { ValidatorContext } from './context';
import { IValidatorRef, useValidatorRef } from './hooks/internal/useValidatorRef';
import { IValidationSchema } from './types';

export interface IValidatorProviderProps<TValue> {
children: React.ReactNode | React.ReactNode[];
schema: IValidationSchema[];
value: TValue;

ref?: React.RefObject<IValidatorRef>;
validateOnChange?: boolean;
validateSync?: boolean;
}

export const ValidatorProvider = <TValue,>({
children,
schema,
value,
ref,
}: IValidatorProviderProps<TValue>) => {
useValidatorRef(ref);

return <ValidatorContext.Provider value={{}}>{children}</ValidatorContext.Provider>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Implement proper context value

The context provider is initialized with an empty object, which doesn't match the expected interface from the AI summary.

Implement the required context value structure:

-  return <ValidatorContext.Provider value={{}}>{children}</ValidatorContext.Provider>;
+  return (
+    <ValidatorContext.Provider
+      value={{
+        errors: [],
+        values: value,
+        validate: async () => {
+          // Implement validation logic
+        },
+      }}
+    >
+      {children}
+    </ValidatorContext.Provider>
+  );

Committable suggestion skipped: line range outside the PR's diff.

};
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from './types';
export * from './validator-context';
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { IValidationError } from '../types';

export interface IValidatorContext<TValues> {
errors: IValidationError[];
values: TValues;
isValid: boolean;
validate: () => void;
}
Comment on lines +3 to +8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance type definitions for better validation control

The current interface could be improved to provide better type safety and flexibility:

Consider these enhancements:

+/** Options for validation execution */
+export interface IValidationOptions {
+  validateAll?: boolean;
+  stopOnFirst?: boolean;
+}
+
+/** Result of validation execution */
+export interface IValidationResult {
+  isValid: boolean;
+  errors: IValidationError[];
+}

 export interface IValidatorContext<TValues> {
   errors: IValidationError[];
   values: TValues;
   isValid: boolean;
-  validate: () => void;
+  validate: (options?: IValidationOptions) => Promise<IValidationResult>;
 }

This provides:

  1. Async validation support
  2. Configurable validation behavior
  3. Structured validation results
  4. Better type safety

Consider adding JSDoc comments to document the interface and its generic parameter usage.

📝 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.

Suggested change
export interface IValidatorContext<TValues> {
errors: IValidationError[];
values: TValues;
isValid: boolean;
validate: () => void;
}
/** Options for validation execution */
export interface IValidationOptions {
validateAll?: boolean;
stopOnFirst?: boolean;
}
/** Result of validation execution */
export interface IValidationResult {
isValid: boolean;
errors: IValidationError[];
}
export interface IValidatorContext<TValues> {
errors: IValidationError[];
values: TValues;
isValid: boolean;
validate: (options?: IValidationOptions) => Promise<IValidationResult>;
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { createContext } from 'react';
import { IValidatorContext } from './types';

export const ValidatorContext = createContext<IValidatorContext<unknown>>({
errors: [],
values: {},
isValid: true,
validate: () => {
throw new Error('Validator context is not provided.');
},
});
Comment on lines +4 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using a more specific generic type parameter

The context is currently typed with unknown, which is too permissive. Consider using a constrained generic type parameter to ensure type safety across the validation framework.

-export const ValidatorContext = createContext<IValidatorContext<unknown>>({
+export const ValidatorContext = createContext<IValidatorContext<TValue extends object>>({

Also, the error message could be more descriptive about where to add the provider:

   validate: () => {
-    throw new Error('Validator context is not provided.');
+    throw new Error('ValidatorContext.Provider is not found in component tree. Wrap your component with ValidatorProvider.');
   },

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +1 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Confirmed: Two separate validator contexts exist with overlapping functionality

There are indeed two separate validator contexts with similar purposes but different implementations:

  • ValidatorContext in Validator/context/validator-context.ts
  • validatorContext in _Validator/validator.context.ts

Both contexts handle form validation but have different interfaces and are used in separate components. This creates technical debt and potential confusion:

  • The newer context (ValidatorContext) includes values and is generic
  • The older context (with underscore prefix _Validator) has a simpler interface
  • They maintain separate error handling structures

Recommendations:

  • Consolidate these validators into a single implementation
  • Consider deprecating the older _Validator version
  • Update all components to use the newer ValidatorContext
🔗 Analysis chain

Verify potential context naming conflicts

There appears to be two validator contexts in the codebase (Validator/context/validator-context.ts and _Validator/validator.context.ts). This could lead to confusion and maintenance issues.


Let me check the actual content of these files to understand if there's a real conflict or if these are related components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for validator context definitions and usages
rg -l "validatorContext|ValidatorContext" --type ts

Length of output: 602


Script:

#!/bin/bash
# Check the content of both validator context files and their usages
echo "=== validator-context.ts ==="
cat packages/ui/src/components/organisms/Form/Validator/context/validator-context.ts

echo -e "\n=== validator.context.ts ==="
cat packages/ui/src/components/organisms/Form/_Validator/validator.context.ts

echo -e "\n=== Checking imports in _Validator/Validator.tsx ==="
rg "import.*[Vv]alidator[Cc]ontext" packages/ui/src/components/organisms/Form/_Validator/Validator.tsx

echo -e "\n=== Checking imports in Validator/ValidatorProvider.tsx ==="
rg "import.*[Vv]alidator[Cc]ontext" packages/ui/src/components/organisms/Form/Validator/ValidatorProvider.tsx

Length of output: 1534

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './useValidator';
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { useContext } from 'react';
import { ValidatorContext } from '../../../context';

export const useValidator = () => {
const context = useContext(ValidatorContext);

if (!context) {
throw new Error('Validator context is not provided.');
}

return context;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './useValidate';
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { IValidationSchema } from '../../../types';

import debounce from 'lodash/debounce';
import { useCallback, useEffect, useState } from 'react';
import { IValidationError } from '../../../types';
import { validate } from '../../../utils/validate';

export interface IUseAsyncValidateParams {
validationDelay?: number;
validateAsync?: boolean;
validateOnChange?: boolean;
abortEarly?: boolean;
}

export const useAsyncValidate = (
context: object,
schema: IValidationSchema[],
params: IUseAsyncValidateParams = {},
) => {
const {
validationDelay = 500,
validateAsync = false,
validateOnChange = true,
abortEarly = false,
} = params;

const [validationErrors, setValidationErrors] = useState<IValidationError[]>(() =>
validateAsync ? validate(context, schema, { abortEarly }) : [],
);
Comment on lines +27 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding error handling for initial validation.

The initial validation in useState could throw an error and crash the component.

Apply this fix:

 const [validationErrors, setValidationErrors] = useState<IValidationError[]>(() =>
-  validateAsync ? validate(context, schema, { abortEarly }) : [],
+  validateAsync ? 
+    try {
+      return validate(context, schema, { abortEarly });
+    } catch (error) {
+      console.error('Initial validation failed:', error);
+      return [];
+    }
+    : [],
 );

Committable suggestion skipped: line range outside the PR's diff.


const validateWithDebounce = useCallback(
debounce((context: object, schema: IValidationSchema[], params: IUseAsyncValidateParams) => {
const errors = validate(context, schema, params);
setValidationErrors(errors);
}, validationDelay),
[validationDelay],
);
Comment on lines +31 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix dependency array in useCallback and handle stale closures.

The current implementation has potential issues with stale closures and unnecessary recreations.

Apply this fix:

 const validateWithDebounce = useCallback(
-  debounce((context: object, schema: IValidationSchema[], params: IUseAsyncValidateParams) => {
+  debounce((context: object, schema: IValidationSchema[]) => {
     const errors = validate(context, schema, params);
     setValidationErrors(errors);
   }, validationDelay),
-  [validationDelay],
+  [validationDelay, params.abortEarly],
 );

Committable suggestion skipped: line range outside the PR's diff.


useEffect(() => {
if (!validateAsync || !validateOnChange) return;

validateWithDebounce(context, schema, { abortEarly });
}, [context, schema, validateAsync, validateOnChange, abortEarly, validateWithDebounce]);
Comment on lines +39 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent memory leaks by cleaning up pending validations.

The current implementation doesn't cancel pending validations on unmount or when dependencies change.

Apply this fix:

 useEffect(() => {
   if (!validateAsync || !validateOnChange) return;

   validateWithDebounce(context, schema, { abortEarly });
+
+  return () => {
+    validateWithDebounce.cancel();
+  };
 }, [context, schema, validateAsync, validateOnChange, abortEarly, validateWithDebounce]);
📝 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.

Suggested change
useEffect(() => {
if (!validateAsync || !validateOnChange) return;
validateWithDebounce(context, schema, { abortEarly });
}, [context, schema, validateAsync, validateOnChange, abortEarly, validateWithDebounce]);
useEffect(() => {
if (!validateAsync || !validateOnChange) return;
validateWithDebounce(context, schema, { abortEarly });
return () => {
validateWithDebounce.cancel();
};
}, [context, schema, validateAsync, validateOnChange, abortEarly, validateWithDebounce]);


return validationErrors;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import { renderHook } from '@testing-library/react';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { validate } from '../../../utils/validate';
import { useAsyncValidate } from './useAsyncValidate';

// Mock dependencies
vi.mock('../../../utils/validate', () => ({
validate: vi.fn().mockReturnValue([
{
id: 'name',
originId: 'name',
message: ['error'],
invalidValue: 'John',
},
]),
}));

vi.mock('lodash/debounce', () => ({
default: (fn: any) => fn,
}));

describe('useAsyncValidate', () => {
const mockContext = { name: 'John' };
const mockSchema = [{ id: 'name', validators: [], rules: [] }];

beforeEach(() => {
vi.clearAllMocks();
});

it('should initialize with empty validation errors', () => {
const { result } = renderHook(() => useAsyncValidate(mockContext, mockSchema));
expect(result.current).toEqual([]);
});

it('should not validate when validateAsync is false', () => {
renderHook(() => useAsyncValidate(mockContext, mockSchema, { validateAsync: false }));
expect(validate).not.toHaveBeenCalled();
});

it('should not validate when validateOnChange is false', () => {
renderHook(() => useAsyncValidate(mockContext, mockSchema, { validateOnChange: false }));
expect(validate).not.toHaveBeenCalled();
});

it('should validate and set errors when validateAsync and validateOnChange are true', () => {
const { result } = renderHook(() =>
useAsyncValidate(mockContext, mockSchema, {
validateAsync: true,
validateOnChange: true,
}),
);

expect(validate).toHaveBeenCalledWith(mockContext, mockSchema, { abortEarly: false });
expect(result.current).toEqual([
{
id: 'name',
originId: 'name',
message: ['error'],
invalidValue: 'John',
},
]);
});

it('should pass abortEarly param to validate function', () => {
renderHook(() =>
useAsyncValidate(mockContext, mockSchema, {
validateAsync: true,
validateOnChange: true,
abortEarly: true,
}),
);

expect(validate).toHaveBeenCalledWith(mockContext, mockSchema, { abortEarly: true });
});
Comment on lines +45 to +74
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling test cases.

The current tests don't verify behavior when validation fails unexpectedly. Add test cases for error scenarios to ensure robust error handling.

it('should handle validation errors gracefully', async () => {
  vi.mocked(validate).mockImplementationOnce(() => {
    throw new Error('Validation failed');
  });

  const { result } = renderHook(() =>
    useAsyncValidate(mockContext, mockSchema, {
      validateAsync: true,
      validateOnChange: true,
    }),
  );

  expect(result.current).toEqual([]);
  // Optionally, verify error was logged or handled appropriately
});


it('should revalidate when context changes', () => {
const { rerender } = renderHook(
({ context }) =>
useAsyncValidate(context, mockSchema, {
validateAsync: true,
validateOnChange: true,
}),
{
initialProps: { context: mockContext },
},
);

const newContext = { name: 'Jane' };
rerender({ context: newContext });

expect(validate).toHaveBeenCalledWith(newContext, mockSchema, { abortEarly: false });
});

it('should revalidate when schema changes', () => {
const { rerender } = renderHook(
({ schema }) =>
useAsyncValidate(mockContext, schema, {
validateAsync: true,
validateOnChange: true,
}),
{
initialProps: { schema: mockSchema },
},
);

const newSchema = [{ id: 'email', validators: [], rules: [] }];
rerender({ schema: newSchema });

expect(validate).toHaveBeenCalledWith(mockContext, newSchema, { abortEarly: false });
});
});
Comment on lines +76 to +111
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add cleanup behavior test cases.

Add tests to verify proper cleanup when the component unmounts, especially important for async operations.

it('should cleanup pending validations on unmount', () => {
  const { unmount } = renderHook(() =>
    useAsyncValidate(mockContext, mockSchema, {
      validateAsync: true,
      validateOnChange: true,
    }),
  );

  unmount();
  
  // Verify that pending validations are cancelled
  // and resources are cleaned up
});

Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { useCallback, useState } from 'react';
import { IValidationError, IValidationSchema } from '../../../types';
import { validate } from '../../../utils/validate';

export interface IUseManualValidateParams {
abortEarly?: boolean;
}

export const useManualValidate = (
context: object,
schema: IValidationSchema[],
params: IUseManualValidateParams = {},
): [IValidationError[], () => void] => {
const [validationErrors, setValidationErrors] = useState<IValidationError[]>([]);

const { abortEarly = false } = params;

const _validate = useCallback(() => {
const errors = validate(context, schema, { abortEarly });
setValidationErrors(errors);
}, [context, schema, abortEarly]);
Comment on lines +18 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for the validate function.

The validation function should handle potential errors that might occur during validation to prevent uncaught exceptions.

 const _validate = useCallback(() => {
-    const errors = validate(context, schema, { abortEarly });
-    setValidationErrors(errors);
+    try {
+      const errors = validate(context, schema, { abortEarly });
+      setValidationErrors(errors);
+    } catch (error) {
+      console.error('Validation failed:', error);
+      setValidationErrors([{
+        path: '',
+        message: 'An unexpected error occurred during validation'
+      }]);
+    }
 }, [context, schema, abortEarly]);
📝 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.

Suggested change
const _validate = useCallback(() => {
const errors = validate(context, schema, { abortEarly });
setValidationErrors(errors);
}, [context, schema, abortEarly]);
const _validate = useCallback(() => {
try {
const errors = validate(context, schema, { abortEarly });
setValidationErrors(errors);
} catch (error) {
console.error('Validation failed:', error);
setValidationErrors([{
path: '',
message: 'An unexpected error occurred during validation'
}]);
}
}, [context, schema, abortEarly]);


return [validationErrors, _validate];
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import { act, renderHook } from '@testing-library/react';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { validate } from '../../../utils/validate';
import { useManualValidate } from './useManualValidate';

vi.mock('../../../utils/validate', () => ({
validate: vi.fn().mockReturnValue([
{
id: 'name',
originId: 'name',
message: ['error'],
invalidValue: 'John',
},
]),
}));

describe('useManualValidate', () => {
const mockContext = { name: 'John' };
const mockSchema = [{ id: 'name', validators: [], rules: [] }];

beforeEach(() => {
vi.clearAllMocks();
});

it('should initialize with empty validation errors', () => {
const { result } = renderHook(() => useManualValidate(mockContext, mockSchema));

expect(result.current.validationErrors).toEqual([]);
});

it('should validate and set errors when validate is called', () => {
const { result } = renderHook(() => useManualValidate(mockContext, mockSchema));

act(() => {
result.current.validate();
});

expect(validate).toHaveBeenCalledWith(mockContext, mockSchema, { abortEarly: false });
expect(result.current.validationErrors).toEqual([
{
id: 'name',
originId: 'name',
message: ['error'],
invalidValue: 'John',
},
]);
});

it('should pass abortEarly param to validate function', () => {
const { result } = renderHook(() =>
useManualValidate(mockContext, mockSchema, { abortEarly: true }),
);

act(() => {
result.current.validate();
});

expect(validate).toHaveBeenCalledWith(mockContext, mockSchema, { abortEarly: true });
});

it('should memoize validate callback with correct dependencies', () => {
const { result, rerender } = renderHook(
({ context, schema, params }) => useManualValidate(context, schema, params),
{
initialProps: {
context: mockContext,
schema: mockSchema,
params: { abortEarly: false },
},
},
);

const firstValidate = result.current.validate;

// Rerender with same props
rerender({
context: mockContext,
schema: mockSchema,
params: { abortEarly: false },
});

expect(result.current.validate).toBe(firstValidate);

// Rerender with different context
rerender({
context: { ...mockContext, newField: 'value' } as any,
schema: mockSchema,
params: { abortEarly: false },
});

Comment on lines +85 to +90
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove type assertion and use proper typing

The use of as any is a code smell and should be avoided. Instead, properly type the test data:

-      context: { ...mockContext, newField: 'value' } as any,
+      context: { ...mockContext, newField: 'value' },

Consider creating an interface for the context type to make the test more type-safe:

interface TestContext {
  name: string;
  newField?: string;
}

const mockContext: TestContext = { name: 'John' };

expect(result.current.validate).not.toBe(firstValidate);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { useMemo } from 'react';
import { IValidationSchema } from '../../../types';
import { validate } from '../../../utils/validate';

export interface IUseSyncValidateParams {
abortEarly?: boolean;
validateSync?: boolean;
validateOnChange?: boolean;
}

export const useSyncValidate = (
context: object,
schema: IValidationSchema[],
params: IUseSyncValidateParams = {},
) => {
const { abortEarly = false, validateSync = false, validateOnChange = true } = params;

return useMemo(() => {
if (!validateSync || !validateOnChange) return [];

return validate(context, schema, { abortEarly });
}, [context, schema, abortEarly, validateSync, validateOnChange]);
Comment on lines +21 to +22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential validation errors.

The validate function call could throw errors that should be handled gracefully.

   return useMemo(() => {
-    return validate(context, schema, { abortEarly });
+    try {
+      return validate(context, schema, { abortEarly });
+    } catch (error) {
+      console.error('Validation error:', error);
+      return [];  // Return empty array on validation failure
+    }
   }, [context, schema, abortEarly, validateSync, validateOnChange]);
📝 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.

Suggested change
return validate(context, schema, { abortEarly });
}, [context, schema, abortEarly, validateSync, validateOnChange]);
try {
return validate(context, schema, { abortEarly });
} catch (error) {
console.error('Validation error:', error);
return []; // Return empty array on validation failure
}
}, [context, schema, abortEarly, validateSync, validateOnChange]);

};
Loading
Loading