Bal 3521 (WIP DO NOT MERGE) #3015
Conversation
|
WalkthroughThis pull request introduces a comprehensive set of changes to the UI package, focusing on enhancing form handling capabilities. The modifications include the addition of a new Changes
Sequence DiagramsequenceDiagram
participant User
participant DynamicForm
participant EntityFieldGroup
participant EntityFields
participant TaskRunner
User->>DynamicForm: Interact with form
DynamicForm->>EntityFieldGroup: Create/Manage Entities
EntityFieldGroup->>EntityFields: Lock/Unlock Entity
EntityFields->>TaskRunner: Add/Remove Tasks
TaskRunner-->>EntityFields: Task Execution Result
EntityFields-->>EntityFieldGroup: Update Entity State
DynamicForm-->>User: Form Updated
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 11
🔭 Outside diff range comments (2)
packages/ui/src/components/organisms/Form/DynamicForm/_stories/FileUploadShowcase/FileUploadShowcase.tsx (1)
Line range hint
95-97: Add error handling to onSubmit.The onSubmit callback should include error handling.
onSubmit={() => { - console.log('onSubmit'); + try { + console.log('onSubmit'); + // Handle form submission + } catch (error) { + console.error('Form submission failed:', error); + // Show error to user + } }}packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/FileField.tsx (1)
Breaking Change Confirmed: Requires Version Bump and Migration Guide
The change to make
uploadSettingsrequired is indeed breaking as:
- DocumentField extends from FileField's interface
- Package is published to npm (@ballerine/[email protected])
- Tests verify undefined uploadSettings case
Recommendations:
- Bump major version (0.6.0) since package is pre-1.0
- Add migration guide in documentation
- Consider maintaining backward compatibility through a deprecated optional pattern
🔗 Analysis chain
Line range hint
20-26: Breaking Change: uploadSettings is now required.Making
uploadSettingsrequired is a breaking change that could affect existing implementations. Consider:
- Maintaining backward compatibility
- Adding migration guide
- Incrementing major version
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing usage of FileField to assess impact rg -A 5 "uploadSettings[?]?:" .Length of output: 10205
Script:
#!/bin/bash # Search for FileField usage in actual implementations rg "FileField" --type ts --type tsx -B 2 -A 2 . | grep -v "test\|story\|showcase" # Check for any existing migration guides or documentation fd -e md . | xargs rg -l "FileField|migration|breaking.change"Length of output: 174
Script:
#!/bin/bash # Find all ts/tsx files and search for FileField fd -e ts -e tsx . | xargs rg "FileField" -B 2 -A 2 | grep -v "test\|story\|showcase" # Check package.json for version and publication status cat packages/ui/package.json # Look for documentation specifically in the ui package fd -e md . packages/ui/ | xargs rg -l "FileField|migration|breaking.change"Length of output: 15228
🧹 Nitpick comments (33)
packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/EntityFields.tsx (4)
3-3: Consider renaming the typeTDeepthLevelStack.
It appears there may be a minor spelling misalignment in the type name ("Deepth" vs. "Depth"). Renaming it to something likeTDepthLevelStackwill improve clarity.
44-48: Remove or clarify the commented-out code.
Lines 44–48 contain commented-out references touseHttp. If they’re no longer necessary, removing them will keep your codebase lean. Otherwise, add a brief explanation indicating why they remain commented out.
67-67: Fix spelling for "documentFieldDefinitons".
Minor improvement for readability:- const documentFieldDefinitons = + const documentFieldDefinitions =
153-194: Refine the variable name for child elements.
Usingchildrenscan be slightly confusing. Consider a more descriptive alternative likerenderableChildrenorlockedChildrento clearly convey its purpose.packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/types/index.ts (1)
1-6: Double-underscore properties are slightly unconventional.
Properties like__id,__isLocked, and__isCreatedwork, but they may be less intuitive for some team members. Consider a naming approach that conveys the meaning without underscores if you don’t have a specific convention enforcing them.packages/ui/src/common/hooks/useHttp/types.ts (1)
1-6: Consider enhancing the HTTP parameters interface.
- The purpose of the required
resultPathproperty is unclear. Consider adding JSDoc comments to explain its usage.- Consider adding common HTTP parameters:
paramsfor query parametersdataorbodyfor request payloadtimeoutfor request timeoutwithCredentialsfor CORSApply this diff to enhance the interface:
+/** + * Parameters for making HTTP requests + */ export interface IHttpParams { url: string; + /** + * @description Path to extract the result from the response + */ resultPath: string; headers?: Record<string, string>; method?: 'POST' | 'PUT' | 'GET' | 'DELETE'; + params?: Record<string, string>; + data?: unknown; + timeout?: number; + withCredentials?: boolean; }packages/ui/src/common/hooks/useHttp/utils/format-headers.ts (1)
3-15: Add validation and error handling for header formatting.The current implementation lacks validation and error handling for header names and values.
Apply this diff to enhance the function:
+const isValidHeaderName = (name: string) => /^[a-zA-Z0-9-]+$/.test(name); +const isValidHeaderValue = (value: string) => !value.includes('\n') && !value.includes('\r'); + export const formatHeaders = ( headers: Record<string, string>, metadata: Record<string, string> = {}, ) => { const formattedHeaders: Record<string, string> = {}; Object.entries(headers).forEach(([key, value]) => { + if (!isValidHeaderName(key)) { + throw new Error(`Invalid header name: ${key}`); + } + if (!isValidHeaderValue(value)) { + throw new Error(`Invalid header value for ${key}`); + } const formattedValue = formatString(value, metadata); + if (!isValidHeaderValue(formattedValue)) { + throw new Error(`Invalid formatted header value for ${key}`); + } formattedHeaders[key] = formattedValue; }); return formattedHeaders; };packages/ui/src/common/hooks/useHttp/utils/request.ts (1)
24-28: Improve error handling.The current error handling could be enhanced:
- Avoid logging raw errors as they might contain sensitive information.
- Consider transforming the error before re-throwing to ensure consistent error format.
} catch (error) { - console.error('Failed to perform request.', error); + console.error('Failed to perform request.'); + + // Transform error to ensure consistent format + if (axios.isAxiosError(error)) { + throw { + message: error.response?.data?.message || 'Request failed', + status: error.response?.status, + code: error.code + }; + } throw error; }packages/ui/src/common/hooks/useHttp/useHttp.ts (2)
20-22: Improve error type handling.The current error type casting to
Errormight be unsafe. Consider using a more specific error type or proper type checking.- setResponseError(error as Error); + if (error instanceof Error) { + setResponseError(error); + } else { + setResponseError(new Error('An unknown error occurred')); + }
11-27: Add type information for response data.Consider adding generic type parameters to improve type safety for the response data.
- const runRequest = useCallback( + const runRequest = useCallback<(requestPayload?: any) => Promise<T | undefined>>( async (requestPayload?: any) => {packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/hooks/useChildrenDisabledOnLock/useChildrenDisabledOnLock.ts (1)
16-24: Replace hardcoded disable rule with a more meaningful condition.The current disable rule
{==: [1, 1]}is hardcoded to always be true. Consider using a more meaningful condition that reflects the actual locking state.disable: [ ...(child.disable || []), { engine: 'json-logic' as const, value: { - '==': [1, 1], + var: 'isLocked' }, }, ],packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/hooks/useEntityLock/useEntityLock.ts (2)
22-22: Fix typo in comment.The comment has a typo: "it it" should be "it".
- // If the entity has an id, it it means its already created and should't be mutated. + // If the entity has an id, it means it's already created and shouldn't be mutated.
41-58: Optimize performance by avoiding the delete operator.Using the
deleteoperator can impact performance. Consider using an undefined assignment instead.const unlockEntity = useCallback(() => { const updatedArray = entities.map(entity => { if (entity.__id === entityId) { const newEntity = { ...entity, }; - delete newEntity.__isLocked; + newEntity.__isLocked = undefined; onUnlock(newEntity); return newEntity; } return entity; }); onChange(updatedArray); }, [onChange, entityId, entities, onUnlock]);🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/hooks/useEntityFieldGroupList/useEntityFieldGroupList.ts (2)
51-57: Improve error handling.The catch block should:
- Use a more specific error type
- Include error details in the toast message
- Consider retrying the operation
- } catch (error) { - toast.error('Failed to delete entity.'); + } catch (error: unknown) { + const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + toast.error(`Failed to delete entity: ${errorMessage}`);
36-38: Add type guard for value array.The value array access needs better type safety.
- if (!Array.isArray(value)) { + if (!Array.isArray(value) || !value.length) { return; }packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/hooks/useFileUpload/useFileUpload.ts (1)
42-44: Refactor duplicate FormData creation.The FormData creation logic is duplicated between 'change' and 'submit' modes.
Extract to a helper function:
+ const createFileFormData = useCallback((file: File) => { + const formData = new FormData(); + formData.append('file', file); + return formData; + }, []); - const formData = new FormData(); - formData.append('file', e.target?.files?.[0] as File); + const formData = createFileFormData(e.target?.files?.[0] as File);Also applies to: 57-58
packages/ui/src/components/organisms/Form/DynamicForm/_stories/FileUploadShowcase/FileUploadShowcase.tsx (1)
17-19: Make upload URLs configurable.The localhost URLs should be configurable to support different environments.
+const API_BASE_URL = process.env.REACT_APP_API_URL || 'http://localhost:3000'; + const schema: Array<IFormElement<any, any>> = [ { id: 'FileField:Regular', element: 'filefield', valueDestination: 'file-regular', params: { label: 'Regular Upload', placeholder: 'Select File', uploadSettings: { - url: 'http://localhost:3000/upload', + url: `${API_BASE_URL}/upload`, resultPath: 'filename', method: 'POST', }, }, },Also applies to: 31-35, 49-51, 68-70
packages/ui/src/components/organisms/Form/Validator/validators/document/document-validator.unit.test.ts (2)
17-21: Add more specific error messages.The error messages in the tests should be more descriptive to help identify the failure cause.
- 'Test message', + 'Value must be an array of documents', ); }); it('should throw error when array is empty', () => { expect(() => documentValidator([], mockParams)).toThrow( - 'Test message', + 'Document array cannot be empty', ); });Also applies to: 23-25
51-61: Add edge case tests.The test suite should include edge cases such as:
- Documents with multiple pages
- Invalid page numbers
- Special characters in IDs
Would you like me to generate additional test cases for these scenarios?
packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.tsx (2)
20-26: Consider enhancing type safety for HTTP parameters.The
httpsParamsproperty could benefit from more specific typing for request methods and URL patterns.export interface IEntityFieldGroupParams extends IFieldListParams { httpsParams: { - createEntity: IHttpParams; - deleteEntity: IHttpParams; + createEntity: IHttpParams & { method: 'POST' | 'PUT' }; + deleteEntity: IHttpParams & { method: 'DELETE' }; }; lockText?: string; }
37-37: Consider memoizing callback functions.The
addItemandremoveItemcallbacks fromuseEntityFieldGroupListshould be memoized to prevent unnecessary re-renders.- const { items, isRemovingEntity, addItem, removeItem } = useEntityFieldGroupList({ element }); + const { items, isRemovingEntity, addItem: rawAddItem, removeItem: rawRemoveItem } = useEntityFieldGroupList({ element }); + const addItem = useCallback(rawAddItem, [rawAddItem]); + const removeItem = useCallback(rawRemoveItem, [rawRemoveItem]);packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/hooks/useChildrenDisabledOnLock/useChildrenDisabledOnLock.unit.test.ts (1)
41-46: Consider using a constant for the disable rule.The disable rule is duplicated across test cases and could be extracted to a constant.
+const LOCK_DISABLE_RULE = { + engine: 'json-logic', + value: { + '==': [1, 1], + }, +}; - const expectedDisableRule = { - engine: 'json-logic', - value: { - '==': [1, 1], - }, - }; + const expectedDisableRule = LOCK_DISABLE_RULE;packages/ui/src/common/hooks/useHttp/utils/request.unit.test.ts (1)
75-91: Add more specific error test cases.The error handling test could be expanded to cover specific HTTP error codes and network errors.
+ it('should handle network errors', async () => { + const requestParams = { + url: 'http://api.example.com/test', + method: 'GET', + headers: {}, + } as const; + const error = new Error('Network Error'); + error.name = 'NetworkError'; + + mockFormatString.mockReturnValue('http://api.example.com/test'); + mockFormatHeaders.mockReturnValue({}); + mockAxios.mockRejectedValue(error); + + const consoleSpy = vi.spyOn(console, 'error'); + + await expect(request(requestParams, {})).rejects.toThrow('Network Error'); + expect(consoleSpy).toHaveBeenCalledWith('Failed to perform request.', error); + });packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.stories.tsx (1)
116-122: Enhance story documentation and add variants.The story could benefit from better documentation and additional variants to showcase different states.
export default { component: EntityFieldGroup, + parameters: { + docs: { + description: { + component: 'A dynamic form field group that manages a list of entity fields.', + }, + }, + }, }; export const Default = { render: () => <EntityFieldGroup />, + parameters: { + docs: { + description: { + story: 'Default state with pre-filled data.', + }, + }, + }, }; + +export const Empty = { + render: () => <EntityFieldGroup />, + args: { + initialContext: {}, + }, +};packages/ui/src/common/hooks/useHttp/useHttp.unit.test.ts (3)
92-109: Consider using a shorter timeout for testing loading state.The test uses a 100ms timeout which might slow down the test suite unnecessarily. Consider using a shorter timeout (e.g., 10ms) since we're only testing state transitions.
vi.mocked(request).mockImplementationOnce( () => new Promise(resolve => { - setTimeout(() => resolve(mockResponse), 100); + setTimeout(() => resolve(mockResponse), 10); }), );
78-90: Enhance error test coverage.The error test could be more thorough by:
- Testing different types of errors (network, validation, etc.)
- Verifying error message propagation
- Testing error state reset on subsequent successful requests
9-14: Consider using TypeScript for mock parameters.The mockParams object could benefit from explicit type annotations to ensure type safety in tests.
- const mockParams = { + const mockParams: IHttpParams = { url: 'test-url', resultPath: 'data.items', method: 'GET' as const, headers: {}, };packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/hooks/useEntityFieldGroupList/useEntityFieldGroupList.unit.test.ts (2)
64-67: Avoid using 'any' type in mock implementation.The cryptoSpy mock uses 'any' type which bypasses TypeScript's type checking. Consider using a more specific type.
- cryptoSpy.mockReturnValue(mockUUID as any); + cryptoSpy.mockReturnValue(mockUUID);
95-107: Enhance error case test coverage.The test for handling undefined value could be expanded to cover more edge cases:
- null values
- non-array objects
- invalid array items
packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/FileField.tsx (1)
51-57: Optimize file creation for string values.Creating an empty File object for string values is inefficient. Consider using a separate type or interface to represent file identifiers.
- if (typeof value === 'string') { - return new File([], value); - } + if (typeof value === 'string') { + return { id: value, name: value } as FileIdentifier; + }packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFieldGroupDocument/EntityFieldGroupDocument.tsx (1)
67-71: Fix typo in warning message.The warning message contains a typo: "migging" should be "missing".
- console.warn('Template id is migging in element', element); + console.warn('Template id is missing in element', element);packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/hooks/useFileUpload/useFileUpload.unit.test.ts (2)
100-114: Consider adding assertions for task properties.While the test verifies basic task creation, it would be more robust to assert the specific properties of the added task.
await result.current.handleChange(mockEvent); expect(mockOnChange).toHaveBeenCalledWith(mockFile); - expect(mockAddTask).toHaveBeenCalled(); + expect(mockAddTask).toHaveBeenCalledWith(expect.objectContaining({ + id: mockElement.id, + run: expect.any(Function) + }));
192-202: Consider adding more loading state scenarios.While the basic loading state is tested, consider adding tests for:
- State transitions (idle → loading → complete)
- Multiple concurrent uploads
- Loading state during error scenarios
🛑 Comments failed to post (11)
packages/ui/src/components/organisms/Form/Validator/validators/document/document-validator.ts (1)
5-30: 🛠️ Refactor suggestion
Enhance error handling and validation in document validator.
The validator uses the same error message for different validation failures and lacks bounds checking.
Apply this diff to improve the validator:
export const documentValidator: TValidator< TDocument[], IDocumentValidatorParams, TBaseValidators | 'document' > = (value, params) => { - const { message = 'Document is required' } = params; + const { + message = { + required: 'Document is required', + notFound: 'Document not found', + invalidPage: 'Invalid page number', + missingProperty: 'Required document property not found', + }, + } = params; const { id, pageNumber = 0, pageProperty = 'ballerineFileId' } = params.value; + if (pageNumber < 0) { + throw new Error(message.invalidPage); + } + if (!Array.isArray(value) || !value.length) { - throw new Error(message); + throw new Error(message.required); } const document = value.find(doc => doc.id === id); if (!document) { - throw new Error(message); + throw new Error(message.notFound); } + if (pageNumber >= document.pages.length) { + throw new Error(message.invalidPage); + } + const documentValue = document.pages[pageNumber]?.[pageProperty]; if (!documentValue) { - throw new Error(message); + throw new Error(message.missingProperty); } return true; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const documentValidator: TValidator< TDocument[], IDocumentValidatorParams, TBaseValidators | 'document' > = (value, params) => { const { message = { required: 'Document is required', notFound: 'Document not found', invalidPage: 'Invalid page number', missingProperty: 'Required document property not found', }, } = params; const { id, pageNumber = 0, pageProperty = 'ballerineFileId' } = params.value; if (pageNumber < 0) { throw new Error(message.invalidPage); } if (!Array.isArray(value) || !value.length) { throw new Error(message.required); } const document = value.find(doc => doc.id === id); if (!document) { throw new Error(message.notFound); } if (pageNumber >= document.pages.length) { throw new Error(message.invalidPage); } const documentValue = document.pages[pageNumber]?.[pageProperty]; if (!documentValue) { throw new Error(message.missingProperty); } return true; };packages/ui/src/common/hooks/useHttp/utils/request.ts (1)
7-7:
⚠️ Potential issueFix typo in type name.
The type name
TReuqestParamscontains a typo and should beTRequestParams.-export type TReuqestParams = Omit<IHttpParams, 'resultPath'>; +export type TRequestParams = Omit<IHttpParams, 'resultPath'>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export type TRequestParams = Omit<IHttpParams, 'resultPath'>;packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/hooks/useEntityFieldGroupList/useEntityFieldGroupList.ts (1)
42-43:
⚠️ Potential issueRemove hardcoded condition.
The
if (true)condition is replacing actual entity existence check logic. This could lead to unnecessary API calls and incorrect behavior.- // if (entity?.id) { - if (true) { + if (entity?.id) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (entity?.id) {🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Unexpected constant condition.
(lint/correctness/noConstantCondition)
packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/hooks/useFileUpload/useFileUpload.ts (1)
41-47: 🛠️ Refactor suggestion
Add file validation.
The hook should validate file existence and potentially file type before upload.
+ const file = e.target?.files?.[0]; + if (!file) { + console.error('No file selected'); + return; + } + + if (params.allowedTypes && !params.allowedTypes.includes(file.type)) { + console.error('Invalid file type'); + return; + } if (uploadOn === 'change') { try { - const formData = new FormData(); - formData.append('file', e.target?.files?.[0] as File); + const formData = createFileFormData(file);Also applies to: 56-61
packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.tsx (1)
57-57: 🛠️ Refactor suggestion
Replace non-null assertions with safe alternatives.
Using non-null assertions (
!) can lead to runtime errors if the assumptions are wrong.- entityId={entity.__id!} + entityId={entity.__id ?? throw new Error('Entity ID is required')} - onRemoveClick={() => removeItem(entity.__id!)} + onRemoveClick={() => entity.__id && removeItem(entity.__id)}Also applies to: 61-61
packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/hooks/useEntityFieldGroupList/useEntityFieldGroupList.unit.test.ts (1)
57-74: 🛠️ Refactor suggestion
Use try-finally to ensure spy restoration.
The cryptoSpy should be restored even if the test fails. Wrap the test in a try-finally block.
it('should add new item with generated __id', async () => { const mockOnChange = vi.fn(); vi.mocked(useField).mockReturnValue({ onChange: mockOnChange, value: [], } as unknown as ReturnType<typeof useField>); const mockUUID = '123-456'; const cryptoSpy = vi.spyOn(crypto, 'randomUUID'); cryptoSpy.mockReturnValue(mockUUID); + try { const { result } = renderHook(() => useEntityFieldGroupList({ element: mockElement })); await result.current.addItem(); expect(mockOnChange).toHaveBeenCalledWith([{ __id: mockUUID }]); + } finally { cryptoSpy.mockRestore(); + } });📝 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.it('should add new item with generated __id', async () => { const mockOnChange = vi.fn(); vi.mocked(useField).mockReturnValue({ onChange: mockOnChange, value: [], } as unknown as ReturnType<typeof useField>); const mockUUID = '123-456'; const cryptoSpy = vi.spyOn(crypto, 'randomUUID'); cryptoSpy.mockReturnValue(mockUUID); try { const { result } = renderHook(() => useEntityFieldGroupList({ element: mockElement })); await result.current.addItem(); expect(mockOnChange).toHaveBeenCalledWith([{ __id: mockUUID }]); } finally { cryptoSpy.mockRestore(); } });packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/FileField.tsx (1)
35-37:
⚠️ Potential issueReplace non-null assertion with runtime check.
Using non-null assertion (
!) is risky as it bypasses TypeScript's null checks. Consider adding a runtime check instead.- const { handleChange, isUploading: disabledWhileUploading } = useFileUpload( - element, - element.params!, - ); + if (!element.params) { + throw new Error('FileField requires params to be defined'); + } + const { handleChange, isUploading: disabledWhileUploading } = useFileUpload( + element, + element.params, + );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (!element.params) { throw new Error('FileField requires params to be defined'); } const { handleChange, isUploading: disabledWhileUploading } = useFileUpload( element, element.params, );packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFieldGroupDocument/EntityFieldGroupDocument.tsx (2)
73-76:
⚠️ Potential issueAvoid type assertion for template ID.
Using type assertion for the template ID is risky. Consider adding runtime type checking.
+ const templateId = element.params?.template?.id; + if (typeof templateId !== 'string') { + console.warn('Invalid template ID type', templateId); + return; + } const updatedDocuments = removeDocumentFromListByTemplateId( documentsList, - element.params?.template?.id as string, + templateId, );📝 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.const templateId = element.params?.template?.id; if (typeof templateId !== 'string') { console.warn('Invalid template ID type', templateId); return; } const updatedDocuments = removeDocumentFromListByTemplateId( documentsList, templateId, );
85-96:
⚠️ Potential issueAdd type safety for file handling.
The file type assertion could lead to runtime errors. Consider adding validation and proper error handling.
(e: React.ChangeEvent<HTMLInputElement>) => { const documents = get(documentsList || [], element.valueDestination); + const file = e.target.files?.[0]; + if (!file) { + console.warn('No file selected'); + return; + } const updatedDocuments = createOrUpdateFileIdOrFileInDocuments( documents, element, - e.target.files?.[0] as File, + file, ); onChange(updatedDocuments); },📝 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.const handleChange = useCallback( (e: React.ChangeEvent<HTMLInputElement>) => { const documents = get(documentsList || [], element.valueDestination); const file = e.target.files?.[0]; if (!file) { console.warn('No file selected'); return; } const updatedDocuments = createOrUpdateFileIdOrFileInDocuments( documents, element, file, ); onChange(updatedDocuments); }, [onChange], );packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/hooks/useFileUpload/useFileUpload.unit.test.ts (2)
140-182: 🛠️ Refactor suggestion
Standardize error handling across upload modes.
The error handling differs between immediate and submit-time uploads. Consider standardizing the approach:
- Use error callbacks instead of console.error
- Ensure consistent error object structure
- Add type safety for error objects
- expect(consoleSpy).toHaveBeenCalledWith('Failed to upload file.', expect.any(Error)); + expect(mockOnChange).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ + message: 'Upload failed' + }) + }) + );Committable suggestion skipped: line range outside the PR's diff.
117-138: 🛠️ Refactor suggestion
Consider enhancing error handling.
Using
console.logfor error reporting is not ideal for production code. Consider:
- Throwing a specific error type
- Using a proper error handling mechanism
- Implementing error callbacks
- expect(consoleSpy).toHaveBeenCalledWith('Failed to upload, no upload settings provided'); + expect(mockOnChange).toHaveBeenCalledWith( + expect.objectContaining({ + error: new Error('No upload settings provided') + }) + );Committable suggestion skipped: line range outside the PR's diff.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| describe('formatHeaders', () => { | ||
| it('should format headers with metadata', () => { | ||
| const headers = { | ||
| Authorization: 'Bearer {token}', |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
| const result = formatHeaders(headers, metadata); | ||
|
|
||
| expect(result).toEqual({ | ||
| Authorization: 'Bearer abc123', |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
| url: 'http://api.example.com/{path}', | ||
| method: 'GET', | ||
| headers: { | ||
| Authorization: 'Bearer {token}', |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
| const result = await request(requestParams, metadata); | ||
|
|
||
| expect(mockFormatString).toHaveBeenCalledWith('http://api.example.com/{path}', metadata); | ||
| expect(mockFormatHeaders).toHaveBeenCalledWith({ Authorization: 'Bearer {token}' }, metadata); |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (14)
packages/ui/src/components/organisms/Form/Validator/validators/document/document-validator.unit.test.ts (4)
8-15: Consider improving type safety of mock data.While the mock data structure is appropriate, consider removing the type assertion and explicitly defining the mock data with proper types.
- const mockParams = { + const mockParams: ICommonValidator<IDocumentValidatorParams, TBaseValidators | 'document'> = { message: 'Test message', value: { id: 'test-id', pageNumber: 0, pageProperty: 'ballerineFileId', }, - } as ICommonValidator<IDocumentValidatorParams, TBaseValidators | 'document'>; + };
17-31: Improve type safety in test data and add boundary tests.The test cases for invalid input could be improved:
- Remove type assertions and use proper typing:
- expect(() => documentValidator(null as unknown as TDocument[], mockParams)).toThrow( + expect(() => documentValidator(null as any, mockParams)).toThrow(
- Add boundary tests for array size limits:
it('should handle large document arrays', () => { const largeDocArray = Array(1000).fill({ id: 'test-id', pages: [{ ballerineFileId: 'valid-file-id' }], propertiesSchema: {}, }); expect(documentValidator(largeDocArray, mockParams)).toBe(true); });
33-49: Make test descriptions more specific.The test descriptions could be more descriptive about the exact scenario being tested.
- it('should throw error when document page does not exist', () => { + it('should throw error when document has empty pages array', () => { - it('should throw error when document page property does not exist', () => { + it('should throw error when ballerineFileId is missing from document page', () => {
51-80: Add test cases for edge cases and error messages.The valid document and default parameter tests look good, but consider adding:
- Test for exact error message content
- Test for multiple pages in a document
- Test for multiple documents in the array
Example:
it('should handle multiple pages in a document', () => { const mockDocuments = [{ id: 'test-id', pages: [ { ballerineFileId: 'file-1' }, { ballerineFileId: 'file-2' } ], propertiesSchema: {}, }] as TDocument[]; expect(documentValidator(mockDocuments, { ...mockParams, value: { ...mockParams.value, pageNumber: 1 } })).toBe(true); });packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/hooks/useEntityLock/useEntityLock.ts (3)
7-14: Consider adding TypeScript types for callback parameters.The
onLockandonUnlockcallback parameters would benefit from explicit TypeScript types to ensure type safety across component boundaries.- onLock: (entity: IEntity) => void, - onUnlock: (entity: IEntity) => void, + onLock: (entity: Readonly<IEntity>) => void, + onUnlock: (entity: Readonly<IEntity>) => void,
21-24: Improve comment clarity.The comment appears to have a typo and could be more descriptive about the locking behavior.
- // If the entity has an id, it it means its already created and should't be mutated. + // Check if the entity is locked to prevent mutations
41-58: Optimize performance by avoiding object spread in map.The
unlockEntityfunction creates a new object for each entity in the array, even when not needed.const unlockEntity = useCallback(() => { + if (!entities?.length) return; const updatedArray = entities.map(entity => { if (entity.__id === entityId) { - const newEntity = { - ...entity, - }; - delete newEntity.__isLocked; + const { __isLocked, ...newEntity } = entity; onUnlock(newEntity); return newEntity; } return entity; });🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.tsx (1)
70-74: Add ARIA label for better accessibility.The add button should have an ARIA label for better screen reader support.
- <Button onClick={addItem} disabled={disabled}> + <Button + onClick={addItem} + disabled={disabled} + aria-label={`Add ${addButtonLabel}`}>packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.stories.tsx (1)
106-106: Remove commented-out code.The commented-out onEvent prop should be removed.
- // onEvent={console.log}packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFieldGroupDocument/EntityFieldGroupDocument.tsx (3)
67-69: Fix typo in console warning message.The warning message contains a typo in the word "missing".
- console.warn('Template id is migging in element', element); + console.warn('Template id is missing in element', element);
112-112: Fix typo in UI text.The text "No File Choosen" contains a spelling error.
- <span className="truncate text-sm">{file ? file.name : 'No File Choosen'}</span> + <span className="truncate text-sm">{file ? file.name : 'No File Chosen'}</span>
98-145: Add keyboard navigation support.The component should support keyboard navigation for better accessibility.
Add keyboard event handlers to support Enter and Space key interactions:
<div className={ctw( 'relative flex h-[56px] flex-row items-center gap-3 rounded-[16px] border bg-white px-4', { 'pointer-events-none opacity-50': disabled }, )} onClick={focusInputOnContainerClick} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + focusInputOnContainerClick(); + } + }} + role="button" + tabIndex={0} data-testid={createTestId(element, stack)} >packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/EntityFields.tsx (2)
90-90: Replace hardcoded delay with configuration.The hardcoded delay should be moved to a configuration or environment variable for better flexibility.
-await delay(1000); +await delay(config.entityCreationDelay);
113-118: Enhance error handling with specific error types.The current error handling is too generic. Consider handling specific error types and providing more detailed error messages.
-} catch (error) { - toast.error('Failed to create entity.'); - console.error(error); - - return context; +} catch (error) { + console.error('Entity creation failed:', error); + if (error instanceof NetworkError) { + toast.error('Network error while creating entity. Please try again.'); + } else if (error instanceof ValidationError) { + toast.error('Invalid entity data. Please check your input.'); + } else { + toast.error('Failed to create entity. Please try again later.'); + } + return context;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
packages/ui/package.json(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.stories.tsx(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.tsx(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFieldGroupDocument/EntityFieldGroupDocument.tsx(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFieldGroupDocument/index.ts(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/EntityFields.tsx(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/hooks/useEntityLock/useEntityLock.ts(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/hooks/useEntityFieldGroupList/useEntityFieldGroupList.ts(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/types/index.ts(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/utils/delay.ts(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/providers/TaskRunner/TaskRunner.tsx(2 hunks)packages/ui/src/components/organisms/Form/Validator/types/index.ts(1 hunks)packages/ui/src/components/organisms/Form/Validator/validators/document/document-validator.ts(1 hunks)packages/ui/src/components/organisms/Form/Validator/validators/document/document-validator.unit.test.ts(1 hunks)packages/ui/src/components/organisms/Form/Validator/validators/document/index.ts(1 hunks)packages/ui/src/components/organisms/Form/Validator/validators/document/types.ts(1 hunks)packages/ui/src/components/organisms/Form/Validator/validators/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/utils/delay.ts
- packages/ui/src/components/organisms/Form/Validator/validators/index.ts
- packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/types/index.ts
- packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFieldGroupDocument/index.ts
- packages/ui/package.json
- packages/ui/src/components/organisms/Form/Validator/validators/document/index.ts
- packages/ui/src/components/organisms/Form/Validator/validators/document/types.ts
- packages/ui/src/components/organisms/Form/DynamicForm/providers/TaskRunner/TaskRunner.tsx
- packages/ui/src/components/organisms/Form/Validator/types/index.ts
- packages/ui/src/components/organisms/Form/Validator/validators/document/document-validator.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/hooks/useEntityLock/useEntityLock.ts
[error] 48-48: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/hooks/useEntityFieldGroupList/useEntityFieldGroupList.ts
[error] 43-43: Unexpected constant condition.
(lint/correctness/noConstantCondition)
🔇 Additional comments (5)
packages/ui/src/components/organisms/Form/Validator/validators/document/document-validator.unit.test.ts (2)
1-6: LGTM! Clean and well-organized imports.All necessary dependencies and types are properly imported.
1-80: Overall implementation is solid and well-tested.The test suite provides good coverage of the document validator functionality with clear test cases and consistent structure. While there are some opportunities for improvement in type safety and edge case coverage, the current implementation is robust and maintainable.
packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/EntityFields.tsx (3)
1-30: LGTM! Well-structured imports and interface definition.The imports are well-organized, and the interface is comprehensive with proper type definitions.
44-48: Verify the status of commented HTTP integration code.The commented-out HTTP hook suggests incomplete implementation. Please clarify if this is intended to be implemented in a future update.
Would you like me to help implement the HTTP integration using the commented code as a reference?
153-194: LGTM! Well-implemented UI with proper loading states and test coverage.The UI implementation follows React best practices with:
- Proper conditional rendering
- Well-structured component hierarchy
- Good test coverage with data-testid attributes
| const lockEntity = useCallback(() => { | ||
| const updatedArray = entities.map(entity => { | ||
| if (entity.__id === entityId) { | ||
| const lockedEntity = { ...entity, __isLocked: true }; | ||
| onLock(lockedEntity); | ||
|
|
||
| return lockedEntity; | ||
| } | ||
|
|
||
| return entity; | ||
| }); | ||
|
|
||
| onChange(updatedArray); | ||
| }, [onChange, entityId, entities, onLock]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for undefined entities.
The lockEntity function should handle cases where the entity array is undefined.
const lockEntity = useCallback(() => {
+ if (!entities?.length) return;
const updatedArray = entities.map(entity => {📝 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.
| const lockEntity = useCallback(() => { | |
| const updatedArray = entities.map(entity => { | |
| if (entity.__id === entityId) { | |
| const lockedEntity = { ...entity, __isLocked: true }; | |
| onLock(lockedEntity); | |
| return lockedEntity; | |
| } | |
| return entity; | |
| }); | |
| onChange(updatedArray); | |
| }, [onChange, entityId, entities, onLock]); | |
| const lockEntity = useCallback(() => { | |
| if (!entities?.length) return; | |
| const updatedArray = entities.map(entity => { | |
| if (entity.__id === entityId) { | |
| const lockedEntity = { ...entity, __isLocked: true }; | |
| onLock(lockedEntity); | |
| return lockedEntity; | |
| } | |
| return entity; | |
| }); | |
| onChange(updatedArray); | |
| }, [onChange, entityId, entities, onLock]); |
| // const { run: deleteEntity, isLoading } = useHttp( | ||
| // element.params!.httpsParams?.deleteEntity, | ||
| // metadata, | ||
| // ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code.
The commented HTTP deletion code should either be implemented or removed to maintain code cleanliness.
| // if (entity?.id) { | ||
| if (true) { |
There was a problem hiding this comment.
Fix constant condition in if statement.
The condition if (true) appears to be a placeholder and should be replaced with actual logic.
- // if (entity?.id) {
- if (true) {
+ if (entity?.id) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // if (entity?.id) { | |
| if (true) { | |
| if (entity?.id) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Unexpected constant condition.
(lint/correctness/noConstantCondition)
| setIsRemovingEntity(true); | ||
| await delay(1000); | ||
| setIsRemovingEntity(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove artificial delay in entity removal.
The artificial delay using delay(1000) should be replaced with actual business logic.
- setIsRemovingEntity(true);
- await delay(1000);
- setIsRemovingEntity(false);
+ // Implement actual entity removal logic hereCommittable suggestion skipped: line range outside the PR's diff.
| key={entity.id} | ||
| entityId={entity.__id!} |
There was a problem hiding this comment.
Fix inconsistent key usage.
The component uses entity.id as key but entity.__id for other operations. This could lead to rendering issues.
- key={entity.id}
+ key={entity.__id!}📝 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.
| key={entity.id} | |
| entityId={entity.__id!} | |
| key={entity.__id!} | |
| entityId={entity.__id!} |
| onSubmit={() => { | ||
| console.log('onSubmit'); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log from onSubmit handler.
Production code should not contain console.log statements.
- onSubmit={() => {
- console.log('onSubmit');
- }}
+ onSubmit={() => {
+ // Handle form submission
+ }}📝 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.
| onSubmit={() => { | |
| console.log('onSubmit'); | |
| }} | |
| onSubmit={() => { | |
| // Handle form submission | |
| }} |
| if (entity.__id === lockedEntity.__id) { | ||
| const newEntity = { | ||
| ...entity, | ||
| id: `${entity.__id}-${crypto.randomUUID()}`, |
There was a problem hiding this comment.
Add browser compatibility check for crypto.randomUUID().
The direct usage of crypto.randomUUID() might not be supported in all browsers. Consider adding a fallback.
-id: `${entity.__id}-${crypto.randomUUID()}`,
+id: `${entity.__id}-${crypto.randomUUID?.() || Math.random().toString(36).substr(2, 9)}`,📝 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.
| id: `${entity.__id}-${crypto.randomUUID()}`, | |
| id: `${entity.__id}-${crypto.randomUUID?.() || Math.random().toString(36).substr(2, 9)}`, |
Summary by CodeRabbit
Release Notes
Dependencies
sonnerlibrary for enhanced UI notificationsNew Features
EntityFieldGroupcomponent for dynamic form field managementdocumentValidatorImprovements
useHttpBug Fixes