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 2 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
3 changes: 3 additions & 0 deletions packages/ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
"clsx": "^1.2.1",
"cmdk": "^0.2.0",
"dayjs": "^1.11.6",
"email-validator": "^2.0.4",
"i18n-iso-countries": "^7.6.0",
"lodash": "^4.17.21",
"lucide-react": "^0.144.0",
Expand All @@ -76,6 +77,8 @@
"@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/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,23 @@
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;
}
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 the validateOnChange prop functionality

The validateOnChange prop is defined but not utilized in the component implementation.

Consider implementing validation on value changes when this prop is true:

 export const ValidatorProvider = <TValue,>({
   children,
   schema,
   value,
   ref,
+  validateOnChange = false,
 }: IValidatorProviderProps<TValue>) => {
   useValidatorRef(ref);
 
+  useEffect(() => {
+    if (validateOnChange) {
+      // Implement validation logic here
+    }
+  }, [value, schema, validateOnChange]);

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


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,2 @@
export * from './types';
export * from './useValidatorRef';
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export interface IValidatorRef {
validate: () => void;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { useImperativeHandle } from 'react';
import { useValidator } from '../../external/useValidator/useValidator';
import { IValidatorRef } from './types';

export const useValidatorRef = (refObject?: React.RefObject<IValidatorRef>): IValidatorRef => {
const context = useValidator();

useImperativeHandle(refObject, () => ({
validate: context.validate,
}));

return context;
};
Comment on lines +5 to +13
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 and optimize performance

The current implementation could benefit from error handling and performance optimizations:

Consider these improvements:

 export const useValidatorRef = (refObject?: React.RefObject<IValidatorRef>): IValidatorRef => {
   const context = useValidator();
 
+  if (!context) {
+    throw new Error('useValidatorRef must be used within a ValidatorProvider');
+  }
+
+  const exposedRef = useMemo(
+    () => ({
+      validate: context.validate,
+    }),
+    [context.validate]
+  );
+
-  useImperativeHandle(refObject, () => ({
-    validate: context.validate,
-  }));
+  useImperativeHandle(refObject, () => exposedRef, [exposedRef]);
 
   return context;
 };

Consider adding error boundaries in the parent component to gracefully handle validation errors and provide meaningful feedback to users.

📝 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 const useValidatorRef = (refObject?: React.RefObject<IValidatorRef>): IValidatorRef => {
const context = useValidator();
useImperativeHandle(refObject, () => ({
validate: context.validate,
}));
return context;
};
export const useValidatorRef = (refObject?: React.RefObject<IValidatorRef>): IValidatorRef => {
const context = useValidator();
if (!context) {
throw new Error('useValidatorRef must be used within a ValidatorProvider');
}
const exposedRef = useMemo(
() => ({
validate: context.validate,
}),
[context.validate]
);
useImperativeHandle(refObject, () => exposedRef, [exposedRef]);
return context;
};

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { renderHook } from '@testing-library/react';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { useValidator } from '../../external/useValidator/useValidator';
import { useValidatorRef } from './useValidatorRef';

const mockValidate = vi.fn();

vi.mock('../../external/useValidator/useValidator', () => ({
useValidator: vi.fn(() => ({
validate: mockValidate,
})),
}));
Comment on lines +8 to +12
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

Improve mock implementation robustness

The current mock implementation could be enhanced to better test error scenarios.

 vi.mock('../../external/useValidator/useValidator', () => ({
   useValidator: vi.fn(() => ({
     validate: mockValidate,
+    isValid: true,
+    errors: [],
+    validateField: vi.fn(),
   })),
 }));
📝 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
vi.mock('../../external/useValidator/useValidator', () => ({
useValidator: vi.fn(() => ({
validate: mockValidate,
})),
}));
vi.mock('../../external/useValidator/useValidator', () => ({
useValidator: vi.fn(() => ({
validate: mockValidate,
isValid: true,
errors: [],
validateField: vi.fn(),
})),
}));


describe('useValidatorRef', () => {
const mockRef = { current: null };

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

it('should return context from useValidator', () => {
const { result } = renderHook(() => useValidatorRef());

expect(result.current).toEqual({
validate: mockValidate,
});
expect(useValidator).toHaveBeenCalled();
});

it('should set ref.current.validate to context.validate when ref is provided', () => {
renderHook(() => useValidatorRef(mockRef));

expect(mockRef.current).toEqual({
validate: mockValidate,
});
});

it('should not set ref when no ref object is provided', () => {
const { result } = renderHook(() => useValidatorRef());

expect(result.current).toEqual({
validate: mockValidate,
});
});
});
Comment on lines +14 to +45
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 test coverage for error scenarios

The test suite is missing coverage for error handling scenarios.

Add these test cases:

it('should handle errors during validation', () => {
  mockValidate.mockImplementationOnce(() => {
    throw new Error('Validation failed');
  });
  
  const { result } = renderHook(() => useValidatorRef(mockRef));
  expect(() => result.current.validate()).toThrow('Validation failed');
});

it('should handle undefined ref gracefully', () => {
  const { result } = renderHook(() => useValidatorRef(undefined));
  expect(result.current.validate).toBeDefined();
});

4 changes: 4 additions & 0 deletions packages/ui/src/components/organisms/Form/Validator/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export * from './hooks/external/useValidator';
export * from './types';
export * from './utils/register-validator';
export * from './ValidatorProvider';
44 changes: 44 additions & 0 deletions packages/ui/src/components/organisms/Form/Validator/types/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
export type TBaseValidationRules = 'json-logic';

export interface IValidationRule {
type: TBaseValidationRules;
value: object;
}
Comment on lines +1 to +6
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

Improve type safety and documentation for validation rules

  1. The TBaseValidationRules type is too restrictive. Consider allowing for future rule types.
  2. The value type in IValidationRule should be more specific than object.
-export type TBaseValidationRules = 'json-logic';
+/** Supported validation rule types */
+export type TBaseValidationRules = 
+  | 'json-logic'
+  | 'expression' // Future rule types
+  | 'custom';    // Future rule types

+/** Structure of a validation rule */
 export interface IValidationRule {
   type: TBaseValidationRules;
-  value: object;
+  value: Record<string, unknown>;
 }
📝 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 type TBaseValidationRules = 'json-logic';
export interface IValidationRule {
type: TBaseValidationRules;
value: object;
}
/** Supported validation rule types */
export type TBaseValidationRules =
| 'json-logic'
| 'expression' // Future rule types
| 'custom'; // Future rule types
/** Structure of a validation rule */
export interface IValidationRule {
type: TBaseValidationRules;
value: Record<string, unknown>;
}


export type TBaseValidators =
| 'required'
| 'minLength'
| 'maxLength'
| 'pattern'
| 'minimum'
| 'maximum';

export interface ICommonValidator<T = object, TValidatorType = TBaseValidators> {
type: TValidatorType;
value: T;
message?: string;
applyWhen?: IValidationRule;
}

export interface IValidationSchema {
id: string;
valueDestination?: string;
validators: ICommonValidator[];
children?: IValidationSchema[];
}

export interface IValidationError {
id: string;
originId: string;
invalidValue: unknown;
message: string[];
}
Comment on lines +33 to +38
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

Based on the search results, I can now generate the final response as we have found two different implementations of IValidationError interface with conflicting message types:

Validation error message type inconsistency needs to be resolved

There are two different implementations of IValidationError interface with conflicting message types:

  • In packages/ui/src/components/organisms/Form/_Validator/hooks/useValidate/useValidate.ts: message: string
  • In packages/ui/src/components/organisms/Form/Validator/types/index.ts: message: string[]

This inconsistency needs to be resolved to maintain type safety and prevent potential runtime errors.

🔗 Analysis chain

Review message type consistency across validation system

There's a potential inconsistency in the message handling:

  • Validators throw single error messages
  • IValidationError expects an array of messages

Let's verify the message handling across the validation system:


Let me search for the actual error handling implementation to see how validation errors are collected and processed.


Let me try a different approach to find how validation errors are collected and processed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for message handling in validators and error processing
rg -A 2 "message[\"']?:" packages/ui/src/components/organisms/Form/

Length of output: 30214


Script:

#!/bin/bash
# Search for validation error handling implementation
ast-grep --pattern 'catch ($_) {
  $$$
}'

Length of output: 43


Script:

#!/bin/bash
# Search for validation error handling and collection
rg -A 5 "IValidationError" packages/ui/src/components/organisms/Form/

Length of output: 4835


export * from '../hooks/internal/useValidatorRef/types';

export type TValidator<T, TValidatorParams = unknown> = (
value: T,
validator: ICommonValidator<TValidatorParams>,
) => void;

export type TDeepthLevelStack = number[];
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { TDeepthLevelStack } from '../../types';

import { IValidationError } from '../../types';
import { formatId } from '../format-id';

export const createValidationError = ({
id,
invalidValue,
message,
stack,
}: {
id: string;
invalidValue: unknown;
message: string;
stack: TDeepthLevelStack;
}): IValidationError => {
const formattedId = formatId(id, stack);

const error: IValidationError = {
id: formattedId,
originId: id,
invalidValue,
message: [message],
};

return error;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { describe, expect, it } from 'vitest';
import { createValidationError } from './create-validation-error';

describe('createValidationError', () => {
it('should create validation error with formatted id', () => {
const params = {
id: 'test',
invalidValue: 'invalid',
message: 'error message',
stack: [1, 2],
};

const result = createValidationError(params);

expect(result).toEqual({
id: 'test-1-2',
originId: 'test',
invalidValue: 'invalid',
message: ['error message'],
});
});

it('should handle empty stack', () => {
const params = {
id: 'test',
invalidValue: 123,
message: 'error message',
stack: [],
};

const result = createValidationError(params);

expect(result).toEqual({
id: 'test-',
originId: 'test',
invalidValue: 123,
message: ['error message'],
});
});

it('should handle single stack value', () => {
const params = {
id: 'test',
invalidValue: null,
message: 'error message',
stack: [1],
};

const result = createValidationError(params);

expect(result).toEqual({
id: 'test-1',
originId: 'test',
invalidValue: null,
message: ['error message'],
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './create-validation-error';
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export const formatErrorMessage = (message: string, key: string, value: string) =>
message.replaceAll(`{${key}}`, value);
Comment on lines +1 to +2
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 input validation and improve type safety

The current implementation could be enhanced in several ways:

  1. Add null/undefined checks
  2. Support multiple value types (number, boolean)
  3. Handle edge cases with special characters

Consider this improved implementation:

-export const formatErrorMessage = (message: string, key: string, value: string) =>
-  message.replaceAll(`{${key}}`, value);
+export const formatErrorMessage = (
+  message: string,
+  key: string,
+  value: string | number | boolean
+) => {
+  if (!message || !key) return message;
+  const safeValue = String(value);
+  return message.replaceAll(`{${key}}`, safeValue);
+};
📝 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 const formatErrorMessage = (message: string, key: string, value: string) =>
message.replaceAll(`{${key}}`, value);
export const formatErrorMessage = (
message: string,
key: string,
value: string | number | boolean
) => {
if (!message || !key) return message;
const safeValue = String(value);
return message.replaceAll(`{${key}}`, safeValue);
};

Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { describe, expect, it } from 'vitest';
import { formatErrorMessage } from './format-error-message';

describe('formatErrorMessage', () => {
it('should replace single placeholder with value', () => {
const message = 'This is a {test} message';
const result = formatErrorMessage(message, 'test', 'sample');
expect(result).toBe('This is a sample message');
});

it('should replace multiple occurrences of the same placeholder', () => {
const message = 'The {value} is equal to {value}';
const result = formatErrorMessage(message, 'value', '42');
expect(result).toBe('The 42 is equal to 42');
});

it('should not modify message when placeholder is not found', () => {
const message = 'This message has no placeholders';
const result = formatErrorMessage(message, 'key', 'value');
expect(result).toBe('This message has no placeholders');
});

it('should handle empty strings', () => {
const message = '';
const result = formatErrorMessage(message, 'key', 'value');
expect(result).toBe('');
});

it('should handle special characters in placeholder values', () => {
const message = 'Special {char} test';
const result = formatErrorMessage(message, 'char', '$@#');
expect(result).toBe('Special $@# test');
});
});
Comment on lines +4 to +34
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 test coverage for edge cases and security

The test suite should include additional scenarios for better coverage and security validation.

Add these test cases:

it('should handle nested placeholders', () => {
  const message = 'Nested {outer{inner}} test';
  const result = formatErrorMessage(message, 'outer{inner}', 'value');
  expect(result).toBe('Nested value test');
});

it('should escape HTML in values to prevent XSS', () => {
  const message = 'Test {script}';
  const result = formatErrorMessage(message, 'script', '<script>alert("xss")</script>');
  expect(result).toBe('Test &lt;script&gt;alert("xss")&lt;/script&gt;');
});

it('should handle malformed placeholders', () => {
  const message = 'Malformed {key test';
  const result = formatErrorMessage(message, 'key', 'value');
  expect(result).toBe('Malformed {key test');
});

it('should handle multiple different placeholders', () => {
  const message = '{first} and {second}';
  const result1 = formatErrorMessage(message, 'first', '1');
  const result2 = formatErrorMessage(result1, 'second', '2');
  expect(result2).toBe('1 and 2');
});

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './format-error-message';
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { TDeepthLevelStack } from '../../types';

export const formatId = (id: string, stack: TDeepthLevelStack) => {
const _id = `${id}${stack.length ? `-${stack.join('-')}` : ''}`;

return _id;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { describe, expect, it } from 'vitest';
import { formatId } from './format-id';

describe('formatId', () => {
it('should append stack values to id', () => {
const id = 'test';
const stack = [1, 2];

const result = formatId(id, stack);

expect(result).toBe('test-1-2');
});

it('should handle empty stack', () => {
const id = 'test';
const stack: number[] = [];

const result = formatId(id, stack);

expect(result).toBe('test');
});

it('should handle single stack value', () => {
const id = 'test';
const stack = [1];

const result = formatId(id, stack);

expect(result).toBe('test-1');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './format-id';
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { TDeepthLevelStack } from '../../types';

export const formatValueDestination = (valueDestination: string, stack: TDeepthLevelStack) => {
let _valueDestination = valueDestination;

stack.forEach((stack, index) => {
_valueDestination = _valueDestination.replace(`$${index}`, stack.toString());
});

return _valueDestination;
};
Comment on lines +3 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.

⚠️ Potential issue

Multiple critical improvements needed in the implementation

The current implementation has several potential issues that need to be addressed:

  1. String.replace() only replaces the first occurrence of a pattern
  2. No input validation for null/undefined values
  3. Unsafe toString() conversion

Consider this improved implementation:

-export const formatValueDestination = (valueDestination: string, stack: TDeepthLevelStack) => {
+export const formatValueDestination = (valueDestination: string | undefined | null, stack: TDeepthLevelStack) => {
+  if (!valueDestination) return '';
+  if (!Array.isArray(stack)) return valueDestination;
+
   let _valueDestination = valueDestination;

   stack.forEach((stack, index) => {
-    _valueDestination = _valueDestination.replace(`$${index}`, stack.toString());
+    const placeholder = `$${index}`;
+    const value = typeof stack === 'object' ? JSON.stringify(stack) : String(stack);
+    while (_valueDestination.includes(placeholder)) {
+      _valueDestination = _valueDestination.replace(placeholder, value);
+    }
   });

   return _valueDestination;
 };
📝 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 const formatValueDestination = (valueDestination: string, stack: TDeepthLevelStack) => {
let _valueDestination = valueDestination;
stack.forEach((stack, index) => {
_valueDestination = _valueDestination.replace(`$${index}`, stack.toString());
});
return _valueDestination;
};
export const formatValueDestination = (valueDestination: string | undefined | null, stack: TDeepthLevelStack) => {
if (!valueDestination) return '';
if (!Array.isArray(stack)) return valueDestination;
let _valueDestination = valueDestination;
stack.forEach((stack, index) => {
const placeholder = `$${index}`;
const value = typeof stack === 'object' ? JSON.stringify(stack) : String(stack);
while (_valueDestination.includes(placeholder)) {
_valueDestination = _valueDestination.replace(placeholder, value);
}
});
return _valueDestination;
};

Loading
Loading