Illiar/feat/kyb v1 cleanup #3004
Conversation
|
WalkthroughThis pull request involves updates across multiple packages, with a primary focus on the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
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.
Actionable comments posted: 2
🔭 Outside diff range comments (5)
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/DocumentField/DocumentField.tsx (2)
Line range hint
37-37: Remove remaining ts-ignore comments and add proper type definitions.Several ts-ignore comments remain in the code, which contradicts the PR objective of removing TypeScript tags. Consider adding proper type definitions instead.
For example, for the props destructuring:
- //@ts-ignore - const { definition, formData, inputIndex, onBlur, ...restProps } = props; + const { definition, formData, inputIndex, onBlur, ...restProps }: { + definition: UIElement<DocumentFieldParams>; + formData: any; + inputIndex: number | null; + onBlur: () => void; + } = props;Also applies to: 49-49
Line range hint
97-146: Add proper TypeScript types to file upload handler.The file upload handler contains multiple ts-ignore comments. Consider adding proper type definitions:
- //@ts-ignore - const fileUploader: UploadFileFn = useCallback( + const fileUploader: UploadFileFn = useCallback<UploadFileFn>( async (file: File) => { - //@ts-ignore const parser = new DocumentValueDestinationParser(definition.valueDestination); // ... rest of the function }, [stateApi, options, inputIndex, definition.valueDestination, toggleElementLoading], );packages/ui/src/components/organisms/DynamicForm/components/RSJVInputAdaters/MultiselectInputAdapter/MultiselectInputAdapter.tsx (1)
Line range hint
28-28: Remove @ts-ignore comment.Per PR objectives to remove TypeScript tags, this @ts-ignore comment should be addressed. Consider properly typing the onBlur callback or handling the type mismatch.
packages/ui/src/components/molecules/inputs/MultiSelect/MultiSelect.tsx (1)
Line range hint
71-77: Remove TypeScript ignore comments.Several TypeScript ignore comments are present in the code. These should be properly typed instead of ignored.
Replace the
@ts-ignorecomments with proper type assertions:onChange( value ? [...value, option.value] : [option.value], - // @ts-ignore - name, + name as string, );packages/ui/src/components/organisms/Form/DynamicForm/fields/MultiselectField/MultiselectField.unit.test.tsx (1)
Line range hint
84-88: Update mock options to use consistent property names.The mock options are using
labelwhile the interface and component expecttitle.Update the mock options to use
title:const mockOptions: MultiselectFieldOption[] = [ - { label: 'Option 1', value: 'opt1' }, - { label: 'Option 2', value: 'opt2' }, - { label: 'Option 3', value: 'opt3' }, + { title: 'Option 1', value: 'opt1' }, + { title: 'Option 2', value: 'opt2' }, + { title: 'Option 3', value: 'opt3' }, ];
🧹 Nitpick comments (5)
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/DocumentField/DocumentField.tsx (2)
Line range hint
91-106: Add cleanup to useLayoutEffect hook.The useLayoutEffect hook should cleanup the fetch operation to prevent memory leaks:
useLayoutEffect(() => { if (!fileId) return; const persistedFile = collectionFlowFileStorage.getFileById(fileId); if (persistedFile) return; - void fetchFile(fileId).then(file => { + let isSubscribed = true; + void fetchFile(fileId).then(file => { + if (!isSubscribed) return; const createdFile = new File([''], file.fileNameInBucket || file.fileNameOnDisk || '', { type: 'text/plain', }); collectionFlowFileStorage.registerFile(fileId, createdFile); }); + return () => { + isSubscribed = false; + }; }, [fileId, toggleElementLoading]);
Line range hint
30-214: Consider breaking down the DocumentField component.The component has grown quite complex with multiple responsibilities (file upload, state management, error handling). Consider breaking it down into smaller, more focused components:
- DocumentUploader (handling file upload logic)
- DocumentStateManager (managing document state)
- DocumentErrorHandler (managing error states)
This would improve maintainability and make it easier to add proper TypeScript types.
packages/ui/src/components/organisms/Form/DynamicForm/fields/MultiselectField/MultiselectFieldSelectedItem.unit.test.tsx (1)
1-1: Consider keeping specific import path.The import path has been made more generic (
@/components/moleculesinstead of.../MultiSelect). While this works, specific imports can make dependencies clearer and help with tree-shaking.-import { MultiSelectOption } from '@/components/molecules'; +import { MultiSelectOption } from '@/components/molecules/inputs/MultiSelect';packages/ui/src/components/organisms/Form/DynamicForm/fields/MultiselectField/MultiselectField.tsx (1)
34-38: Optimize options transformation.The
useMemohook is correctly used to prevent unnecessary re-computations, but the transformation could be simplified if the interface usedtitledirectly.If you apply the previous suggestion to use
titlein the interface, you can simplify this to:const multiselectOptions = useMemo(() => { - return ( - element.params?.options?.map(option => ({ title: option.label, value: option.value })) || [] - ); + return element.params?.options || []; }, [element.params?.options]);packages/ui/src/components/organisms/Form/DynamicForm/fields/MultiselectField/MultiselectField.unit.test.tsx (1)
148-150: Remove unnecessary transformation in test.The options transformation in the test will become unnecessary once the mock options are updated to use
title.After updating the mock options, simplify the assertion:
-expect(multiselect.options).toEqual( - mockOptions.map(option => ({ title: option.label, value: option.value })), -); +expect(multiselect.options).toEqual(mockOptions);
📜 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 (20)
apps/kyb-app/CHANGELOG.md(1 hunks)apps/kyb-app/package.json(2 hunks)apps/kyb-app/src/components/organisms/DynamicUI/Page/hooks/usePageErrors/usePageErrors.ts(0 hunks)apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/CheckboxList/CheckboxList.tsx(0 hunks)apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/DocumentField/DocumentField.tsx(2 hunks)apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/Multiselect/Multiselect.tsx(0 hunks)apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/json-form.fields.ts(0 hunks)apps/kyb-app/src/components/organisms/UIRenderer/elements/SubmitButton/helpers.ts(0 hunks)packages/react-pdf-toolkit/CHANGELOG.md(1 hunks)packages/react-pdf-toolkit/package.json(2 hunks)packages/ui/CHANGELOG.md(1 hunks)packages/ui/package.json(1 hunks)packages/ui/src/components/molecules/inputs/MultiSelect/MultiSelect.stories.tsx(2 hunks)packages/ui/src/components/molecules/inputs/MultiSelect/MultiSelect.tsx(3 hunks)packages/ui/src/components/organisms/DynamicForm/components/RSJVInputAdaters/MultiselectInputAdapter/MultiselectInputAdapter.tsx(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/MultiselectField/MultiselectField.tsx(4 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/MultiselectField/MultiselectField.unit.test.tsx(4 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/MultiselectField/MultiselectFieldSelectedItem.tsx(1 hunks)packages/ui/src/components/organisms/Form/DynamicForm/fields/MultiselectField/MultiselectFieldSelectedItem.unit.test.tsx(1 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
💤 Files with no reviewable changes (5)
- apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/Multiselect/Multiselect.tsx
- apps/kyb-app/src/components/organisms/UIRenderer/elements/SubmitButton/helpers.ts
- apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/CheckboxList/CheckboxList.tsx
- apps/kyb-app/src/components/organisms/DynamicUI/Page/hooks/usePageErrors/usePageErrors.ts
- apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/json-form.fields.ts
✅ Files skipped from review due to trivial changes (6)
- services/workflows-service/prisma/data-migrations
- apps/kyb-app/package.json
- packages/react-pdf-toolkit/package.json
- apps/kyb-app/CHANGELOG.md
- packages/react-pdf-toolkit/CHANGELOG.md
- packages/ui/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/ui/CHANGELOG.md (1)
3-7: LGTM! Version bump and changelog entry are properly documented.The changelog entry correctly documents:
- Version increment to 0.5.69
- Patch change fixing the Multiselect options mapping issue
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/DocumentField/DocumentField.tsx (1)
20-20: LGTM: Clean import addition for document value parsing.The addition of
DocumentValueDestinationParserimport aligns with the enhanced document value parsing functionality.packages/ui/src/components/organisms/Form/DynamicForm/fields/MultiselectField/MultiselectFieldSelectedItem.tsx (1)
16-16: Verify consistent usage of option.title across the codebase.The change from
option.labeltooption.titlealigns with the PR objective to fix multiselect options mapping.Let's verify this change is consistent across the codebase:
✅ Verification successful
The change to
option.titleis consistent within the UI packageThe usage of
option.titleis consistent across all files within thepackages/uiMultiSelect implementation. The instances ofoption.labelfound inbackoffice-v2belong to a separate, independent MultiSelect implementation and do not need to be modified.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of option.label in MultiSelect related files rg "option\.label" --type ts --type tsx # Search for option.title usage to confirm the new pattern rg "option\.title" --type ts --type tsxLength of output: 139
Script:
#!/bin/bash # Search for MultiSelect-related files containing option.label rg -B2 -A2 "option\.label" --glob '*MultiSelect*.{ts,tsx}' # Search for MultiSelect-related files containing option.title rg -B2 -A2 "option\.title" --glob '*MultiSelect*.{ts,tsx}' # List all MultiSelect-related files to ensure we're not missing anything fd "MultiSelect" --extension ts --extension tsxLength of output: 4398
packages/ui/src/components/molecules/inputs/MultiSelect/MultiSelect.stories.tsx (1)
12-12: LGTM! Story file correctly demonstrates the new MultiSelect API.The changes consistently use
titleinstead oflabelacross all examples, which helps document the correct usage of the component.Also applies to: 20-20, 49-49
packages/ui/src/components/organisms/DynamicForm/components/RSJVInputAdaters/MultiselectInputAdapter/MultiselectInputAdapter.tsx (1)
48-48: LGTM! Change is consistent with the new MultiSelect API.The defaultRenderer correctly uses
option.titleinstead ofoption.label.packages/ui/src/components/organisms/Form/DynamicForm/fields/MultiselectField/MultiselectFieldSelectedItem.unit.test.tsx (1)
9-11: LGTM! Test data correctly reflects the new API.The mock option correctly uses the
titleproperty instead oflabel.
...components/organisms/UIRenderer/elements/JSONForm/components/DocumentField/DocumentField.tsx
Show resolved
Hide resolved
...es/ui/src/components/organisms/Form/DynamicForm/fields/MultiselectField/MultiselectField.tsx
Show resolved
Hide resolved
Summary by CodeRabbit
Dependency Updates
@ballerine/uifrom version0.5.68to0.5.69across multiple packageskyb-appandreact-pdf-toolkitCode Quality
Component Improvements
titleinstead oflabelfor option display