Editable entity data #2875
Editable entity data #2875
Conversation
|
WalkthroughThe pull request introduces several enhancements to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/is-path-match.ts
Dismissed
Show dismissed
Hide dismissed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (24)
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx (1)
17-64: RefactorfilterValuefunction for improved readabilityThe
filterValuefunction contains nested logic that may impact maintainability. Refactoring or adding comments can enhance readability and make the code easier to understand.apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2.tsx (1)
155-162: Improve datetime parsing for robustnessAppending
':00'tofield.valuemay not handle all datetime formats correctly. Consider using a more robust date parsing method to accommodate various datetime inputs without manual manipulation.apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx (1)
385-385: Handle potential errors inonSubmitThe
onSubmitfunction callsmutateUpdateContextAndSyncEntity(values)without error handling. Consider adding error handling to manage any exceptions that may occur during the mutation.apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/get-input-type.ts (1)
5-13: Consider using TypeScript enums/constants for input types.To improve type safety and maintainability, consider defining an enum or constant object for the possible input types.
export enum InputType { TEXT = 'text', NUMBER = 'number', DATE = 'date', DATETIME_LOCAL = 'datetime-local', }apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/get-property-path.ts (2)
14-20: Consider memory implications of recursive proxy.The current implementation creates a new proxy for each nested property access. This could lead to memory issues with deeply nested objects.
Consider implementing a depth limit or using an alternative approach for very deep objects:
const MAX_DEPTH = 10; // Add depth tracking to proxy get(target: TObj, prop: PropertyKey, depth = 0) { if (depth >= MAX_DEPTH) { throw new Error('Maximum object depth exceeded'); } path.push(String(prop)); return new Proxy({}, { ...this, depth: depth + 1 }); }
28-30: Enhance error handling specificity.The current error message could be more descriptive to help debugging.
- throw new Error('Property path is empty'); + throw new Error(`Property path is empty. Accessor function: ${accessor.toString()}`);apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/ReadOnlyDetailV2.tsx (2)
10-22: Enhance accessibility implementation.While basic accessibility attributes are present, consider these improvements:
- Add
aria-labelfor better screen reader context- Consider using semantic HTML elements instead of div
<TextWithNAFallback as={'div'} tabIndex={0} role={'textbox'} aria-readonly + aria-label={`Read-only field: ${typeof children === 'string' ? children : ''}`} {...props} className={ctw( 'flex h-9 w-full max-w-[30ch] items-center break-all rounded-md border border-transparent p-1 pt-1.5 text-sm', className, )} >
4-24: Add unit tests for the component.Consider adding tests to verify:
- Rendering with different prop combinations
- Accessibility attributes
- Fallback behavior
Would you like me to help create a test suite for this component?
apps/backoffice-v2/src/domains/workflows/hooks/mutations/useUpdateContextAndSyncEntity/useUpdateContextAndSyncEntity.tsx (2)
12-16: Consider adding type validation for the data parameterThe mutation accepts any partial context data without validation. Consider adding runtime type validation using a schema validator like Zod to ensure data integrity before making the API call.
22-24: Enhance error handling with more specific error messagesThe current error handling uses a generic message. Consider:
- Passing the error details to the toast message
- Logging the error for debugging
- Handling specific error types differently
- onError: () => { + onError: (error) => { + console.error('Failed to update context:', error); toast.error(t('toast:update_details.error')); },apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailsV2Options.tsx (1)
11-13: Consider adding loading state handlingThe component could benefit from a loading state prop to disable the button during edit operations.
export const EditableDetailsV2Options: FunctionComponent<{ onEnableIsEditable: () => void; + isLoading?: boolean; }> = ({ onEnableIsEditable }) => {apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/types.ts (2)
8-19: Enhance type safety for field propertiesSeveral improvements could make the field properties more robust:
valueAliasis typed asanytypeandformatcould be string literals of allowed valuesoptionsarray could benefit from genericsprops: { - valueAlias?: any; + valueAlias?: string | number | boolean | Date; - type: string | undefined; + type?: 'string' | 'number' | 'boolean' | 'date' | 'select'; - format: string | undefined; + format?: 'date' | 'datetime' | 'email' | 'url'; isEditable: boolean; pattern?: string; minimum?: number; maximum?: number; options?: Array<{ label: string; - value: string; + value: string | number; }>; };
23-30: Consider consolidating parse optionsThe parse options object contains multiple boolean flags that could be simplified into a single discriminated union type for better type safety and maintainability.
- parse?: { - date?: boolean; - isoDate?: boolean; - datetime?: boolean; - boolean?: boolean; - url?: boolean; - nullish?: boolean; - }; + parse?: { + type: 'date' | 'isoDate' | 'datetime' | 'boolean' | 'url'; + nullish?: boolean; + };apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/generate-editable-details-v2-fields.ts (1)
26-31: Consider providing default field propertiesThe props object sets undefined values for type and format. Consider providing meaningful defaults or making these optional.
props: { - type: undefined, - format: undefined, + type: 'string', isEditable: true, },apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/is-path-match.ts (1)
19-24: Improve documentation for path matching rulesThe current documentation only provides a single example. Consider expanding it to cover all matching scenarios.
/** + * Determines if a path matches a given pattern using various matching rules: + * 1. Exact match: pattern matches path exactly + * 2. Root-level match: matches fields directly under root + * 3. Wildcard match: '*' matches any single path segment + * 4. Deep match: '*.field' matches field at any depth + * * @example pattern: 'id', path: 'entity.id' where root is 'entity' + * @example pattern: '*.name', path: 'entity.user.name' matches + * @example pattern: 'user.*', path: 'entity.user.email' matches */apps/backoffice-v2/src/pages/Root/Root.page.tsx (1)
Line range hint
42-44: Clean up commented codeThe ReactQueryDevtools component is commented out. Either remove it completely or document why it's temporarily disabled.
packages/common/src/schemas/documents/schemas/entity-schema.ts (1)
8-11: Standardize null handling for contact informationContact information fields (email, phone) allow null values while name fields don't. Consider standardizing the null handling approach across similar field types.
- firstName: Type.Optional(Type.String()), - lastName: Type.Optional(Type.String()), + firstName: Type.Optional(Type.Union([Type.String(), Type.Null()])), + lastName: Type.Optional(Type.Union([Type.String(), Type.Null()])),apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/EditableDetailsV2.tsx (1)
46-46: Consider responsive grid layoutThe current grid layout uses a fixed 3-column grid. Consider making it responsive for smaller screens.
-<div className={'grid grid-cols-3 gap-x-4 gap-y-6'}> +<div className={'grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-x-4 gap-y-6'}>apps/backoffice-v2/public/locales/en/toast.json (1)
104-106: Consider adding error details to the messageConsider adding an error message interpolation like other toast messages in the file.
"update_details": { "success": "Details updated successfully.", - "error": "Error occurred while updating details." + "error": "Error occurred while updating details.\n\n{{errorMessage}}" }apps/backoffice-v2/src/domains/workflows/fetchers.ts (1)
16-31: LGTM! Well-implemented API client function.The function follows the established pattern, includes proper typing, and handles errors appropriately.
Consider adding retry mechanism for network resilience.
For better reliability, consider implementing a retry mechanism for network failures, especially important for sync operations.
export const updateContextAndSyncEntity = async ({ workflowId, data, }: { workflowId: string; data: Partial<TWorkflowById['context']>; }) => { + const MAX_RETRIES = 3; + const RETRY_DELAY = 1000; + + for (let attempt = 1; attempt <= MAX_RETRIES; attempt++) { + try { const [workflow, error] = await apiClient({ endpoint: `../external/workflows/${workflowId}/sync-entity`, method: Method.PATCH, body: data, schema: WorkflowByIdSchema, }); return handleZodError(error, workflow); + } catch (error) { + if (attempt === MAX_RETRIES) throw error; + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY * attempt)); + } + } };services/workflows-service/src/workflow/workflow.controller.external.ts (1)
559-600: LGTM! Well-implemented controller endpoint.The endpoint is well-documented, includes proper validation, and handles errors appropriately.
Consider adding rate limiting for sync operations.
To prevent potential abuse and ensure system stability, consider implementing rate limiting for this sync endpoint.
Consider using the
@nestjs/throttlerpackage to add rate limiting:import { Throttle } from '@nestjs/throttler'; @Throttle({ default: { ttl: 60000, limit: 10 } }) // 10 requests per minute async updateContextAndSyncEntity( @common.Param('workflowRuntimeDataId') workflowRuntimeDataId: string, @common.Body() body: PartialDeep<DefaultContextSchema>, @CurrentProject() projectId: TProjectId, ) { // ... existing implementation }services/workflows-service/src/workflow/workflow.service.ts (3)
2664-2678: Consider adding error handling for the event emission.The transaction block handles the event emission but doesn't have explicit error handling. Consider catching and handling potential errors that might occur during the event emission.
await this.prismaService.$transaction(async transaction => { + try { await this.event( { id: workflowRuntimeDataId, name: BUILT_IN_EVENT.DEEP_MERGE_CONTEXT, payload: { newContext: context, arrayMergeOption: ARRAY_MERGE_OPTION.REPLACE, }, }, [projectId], projectId, transaction, ); + } catch (error) { + this.logger.error('Failed to emit workflow context update event', { + workflowRuntimeDataId, + error, + }); + throw error; + }
2686-2696: Consider adding validation for required fields in adapters.The entity adapters are well-typed but don't validate if the required fields exist in the data. Consider adding validation or using a validation library like
class-validator.const endUserContextToEntityAdapter = (data: Static<typeof IndividualDataSchema>) => { + if (!data.firstName || !data.lastName) { + throw new BadRequestException('First name and last name are required for end user entity update'); + } return { firstName: data.firstName, lastName: data.lastName, } satisfies Parameters<typeof this.entityRepository.endUser.updateById>[1]['data']; }; const businessContextToEntityAdapter = (data: Static<typeof BusinessDataSchema>) => { + if (!data.companyName) { + throw new BadRequestException('Company name is required for business entity update'); + } return { companyName: data.companyName, } satisfies Parameters<typeof this.entityRepository.business.updateById>[1]['data']; };
2654-2711: Consider adding unit tests for the new method.The new method
updateContextAndSyncEntityhandles critical business logic but there's a TODO comment about missing tests at line 4. Consider adding comprehensive unit tests.Would you like me to help generate unit tests for this method? The tests should cover:
- Successful context update and entity sync
- Error handling for event emission
- Error handling for entity updates
- Validation of entity data
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
apps/backoffice-v2/package.json(2 hunks)apps/backoffice-v2/public/locales/en/toast.json(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/EditableDetailsV2.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailsV2Options.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/ReadOnlyDetailV2.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/constants.ts(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/types.ts(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/generate-editable-details-v2-fields.ts(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/get-display-value.ts(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/get-input-type.ts(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/get-property-path.ts(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/is-path-match.ts(1 hunks)apps/backoffice-v2/src/domains/workflows/fetchers.ts(1 hunks)apps/backoffice-v2/src/domains/workflows/hooks/mutations/useUpdateContextAndSyncEntity/useUpdateContextAndSyncEntity.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/components/EditableDetailsV2Cell/EditableDetailsV2Cell.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx(3 hunks)apps/backoffice-v2/src/lib/blocks/create-blocks-typed/create-blocks-typed.ts(2 hunks)apps/backoffice-v2/src/lib/blocks/create-blocks-typed/types.ts(3 hunks)apps/backoffice-v2/src/pages/Root/Root.page.tsx(1 hunks)packages/common/src/schemas/documents/schemas/entity-schema.ts(1 hunks)packages/common/src/schemas/index.ts(1 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/workflow/workflow.controller.external.ts(2 hunks)services/workflows-service/src/workflow/workflow.service.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/constants.ts
- services/workflows-service/prisma/data-migrations
🔇 Additional comments (16)
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2.tsx (1)
63-66: Confirm handling of input value 'N/A'
In the onInputChange handler, when the input value is 'N/A', it is converted to an empty string ''. Verify that this conversion aligns with the intended behavior and that 'N/A' should represent an empty value.
apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx (1)
381-381: Verify the destructuring of useToggle hook
Ensure that the useToggle hook returns four values as destructured. If it only provides two values, adjust the destructuring to match the hook's return signature.
apps/backoffice-v2/src/lib/blocks/components/EditableDetailsV2Cell/EditableDetailsV2Cell.tsx (1)
1-10: Well-structured and efficient component
The EditableDetailsV2Cell component is cleanly implemented, encapsulating the editable details functionality effectively.
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/get-display-value.ts (1)
1-21: Utility function implemented correctly
The getDisplayValue function is well-written and handles value display logic appropriately based on the isEditable flag.
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/get-input-type.ts (1)
30-32: Review date validation strategy.
The current implementation uses multiple date validation methods which could lead to false positives. Consider:
- Documenting the expected date formats
- Adding test cases for edge cases
- Potentially consolidating the validation logic
packages/common/src/schemas/index.ts (1)
22-22: Verify the existence and implementation of the exported schemas
The new exports look good, following the established pattern. Let's verify the referenced schemas exist and are properly implemented.
✅ Verification successful
Both schemas are properly implemented and exported
Both BusinessDataSchema and IndividualDataSchema are well-defined TypeScript schemas in the entity-schema.ts file:
IndividualDataSchemais a comprehensive schema for individual data with fields like firstName, lastName, email, etc.BusinessDataSchemais a detailed schema for business entities with fields like companyName, registrationNumber, etc.
The export statement in index.ts correctly references these existing schemas.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence and implementation of the schemas
ast-grep --pattern 'export const BusinessDataSchema = $$$' ./packages/common/src/schemas/documents/schemas/entity-schema.ts
ast-grep --pattern 'export const IndividualDataSchema = $$$' ./packages/common/src/schemas/documents/schemas/entity-schema.ts
Length of output: 7631
apps/backoffice-v2/src/pages/Root/Root.page.tsx (1)
Line range hint 17-33: LGTM: ChatbotLayout implementation is well-structured
The implementation properly:
- Handles loading states
- Uses feature flags for conditional rendering
- Manages webchat state cleanly
apps/backoffice-v2/src/lib/blocks/create-blocks-typed/create-blocks-typed.ts (1)
25-25: LGTM: Clean integration of EditableDetailsV2Cell
The new editable details cell is properly integrated into the cells map, following the established pattern.
Also applies to: 60-60
packages/common/src/schemas/documents/schemas/entity-schema.ts (1)
73-73: Verify type discrimination logic
The EntitySchema uses a discriminator field 'type' to determine the shape of 'data'. Ensure that runtime type checking properly validates this relationship.
✅ Verification successful
Let me analyze the type discrimination logic further by checking the schema implementation and its usage.
Let me check one more aspect to verify the schema validation implementation.
Type discrimination is properly implemented and validated
The verification shows that type discrimination is correctly implemented with:
- A strict type enum validation:
type: Type.String({ enum: ['individual', 'business'] }) - A properly defined union type for data validation:
data: Type.Union([IndividualDataSchema, BusinessDataSchema]) - Comprehensive schema definitions for both individual and business data types with proper type checking
- Runtime type validation through schema enforcement in the workflow service
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for instances where entity type checking might be bypassed
rg -g '!{test,spec}*' -A 3 'type.*individual|type.*business'
Length of output: 49866
Script:
#!/bin/bash
# Check the schema implementation and type validation
ast-grep --pattern 'Type.Union([IndividualDataSchema, BusinessDataSchema])'
# Check for type validation in runtime
ast-grep --pattern 'entity.type === $type'
Length of output: 254
Script:
#!/bin/bash
# Check for the schema definitions and their usage
rg -g '*.ts' -A 5 'IndividualDataSchema|BusinessDataSchema'
# Check for type discriminator validation
rg -g '*.ts' "type.*enum.*individual.*business"
Length of output: 4932
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/EditableDetailsV2.tsx (4)
15-29: Props structure looks clean and well-organized
Good implementation of mutually exclusive validation for blacklist/whitelist props. The props interface provides a clear contract for component usage.
46-47: Great accessibility implementation with legend
The use of legend with sr-only class improves screen reader accessibility.
81-96: Well-implemented form actions section
Good implementation of:
- Proper button ordering (Cancel before Save)
- Accessibility with aria-disabled
- Conditional rendering based on editability
31-36: Verify the implementation of useNewEditableDetailsLogic
The hook name suggests this is a new version. Let's verify its implementation and usage.
✅ Verification successful
Based on the search results, I can now generate the final response:
Implementation of useNewEditableDetailsLogic is a new component, not a replacement
The hook appears to be a new implementation rather than a version upgrade of an existing hook. The search results show:
- No previous versions of the hook exist in the codebase
- The hook is only used in EditableDetailsV2.tsx
- The implementation is self-contained in useEditableDetailsV2Logic.tsx with proper form handling and field filtering logic
The "new" in the name likely indicates this is a fresh implementation for the V2 version of the EditableDetails component, rather than a replacement of an existing hook. The implementation is clean and follows React best practices with proper use of hooks (useCallback, useMemo, useForm).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other versions of the hook and its usages
echo "Searching for related hooks..."
rg -l "useEditableDetails" --type ts --type tsx
echo "Searching for usages of the new hook..."
rg "useNewEditableDetailsLogic" --type ts --type tsx -A 5
Length of output: 321
Script:
#!/bin/bash
# Search for related hooks and usages with correct file type flags
echo "Searching for related hooks..."
rg -t typescript -t tsx -l "useEditableDetails"
echo "Searching for usages of the new hook..."
rg -t typescript -t tsx "useNewEditableDetailsLogic" -A 5
# Search for the hook implementation
echo "Searching for hook implementation..."
ast-grep --pattern 'const useNewEditableDetailsLogic = $_'
Length of output: 36869
Script:
#!/bin/bash
# Search for any older versions or related hooks
echo "Searching for related editable details hooks..."
fd -e tsx -e ts "useEditableDetails" --exec rg -l "useEditableDetails"
# Search for components using either version
echo "Searching for components using editable details..."
rg -t tsx -t ts "useEditableDetails" -B 2 -A 2
Length of output: 359
Script:
#!/bin/bash
# Search for files containing editable details hooks
echo "Searching for hook files..."
fd "useEditableDetails.*\.(tsx?|jsx?)"
# Search for usages with ripgrep without file type flags
echo "Searching for hook usages..."
rg "useEditableDetails" -B 2 -A 2
# Search for any older versions using ast-grep
echo "Searching for hook implementations..."
ast-grep --pattern 'const useEditableDetails$_ = $_'
Length of output: 17366
apps/backoffice-v2/package.json (1)
120-120: Good choice of lodash-es
The addition of lodash-es and its types is appropriate:
- Uses the ESM version for better tree-shaking
- Includes TypeScript types for better development experience
Also applies to: 171-171
apps/backoffice-v2/src/lib/blocks/create-blocks-typed/types.ts (1)
252-256: LGTM! Well-structured type definition.
The new TEditableDetailsV2Cell type is well-defined and properly integrated into the existing type system. It follows the established pattern of other cell types and provides proper typing for the editable details feature.
services/workflows-service/src/workflow/workflow.service.ts (1)
2655-2663: LGTM! Method signature is well-typed and documented.
The method signature uses PartialDeep from type-fest to allow partial updates of the context structure, which is a good practice for flexibility.
...ts/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx
Show resolved
Hide resolved
...ts/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx
Show resolved
Hide resolved
...v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailsV2Options.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/types.ts
Show resolved
Hide resolved
...c/common/components/organisms/EditableDetailsV2/utils/generate-editable-details-v2-fields.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
apps/backoffice-v2/src/domains/workflow-definitions/fetchers.ts (1)
50-57: LGTM! Consider adding JSDoc documentation.The
editableContextschema addition is well-structured and properly typed. To improve maintainability, consider adding JSDoc documentation explaining the purpose and usage of this configuration option.+ /** + * Configuration for editable fields in different contexts + * @property {Object} kyc - KYC-specific editable field settings + * @property {boolean} kyc.entity - Whether the KYC entity data is editable + */ editableContext: z .object({ kyc: z.object({apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx (3)
366-366: Remove unused variable_toggleIsEditableThe variable
_toggleIsEditablereturned byuseToggleis assigned but not used. Removing it will clean up the code.
370-371: Add error handling for the mutationWhen calling
mutateUpdateContextAndSyncEntity(values);, consider handling potential errors and success cases by providingonSuccessandonErrorcallbacks or wrapping the call in atry...catchblock to improve robustness.
375-449: OptimizegetEntityDataBlockwithuseCallbackThe
getEntityDataBlockfunction is recreated on every render, which may impact performance. Wrap it withuseCallbackto memoize the function, reducing unnecessary re-creations.Apply this change:
- const getEntityDataBlock = () => { + const getEntityDataBlock = useCallback(() => { // function body remains the same }, [dependencies]);Ensure to list all relevant dependencies in the dependency array.
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/types.ts (2)
35-38: Use inclusive terminology instead of 'blacklist' and 'whitelist'Consider replacing
blacklistwithblocklistandwhitelistwithallowlistto use more inclusive language.Also applies to: 40-43
57-58: Simplify optional property declarationsIt's more idiomatic in TypeScript to declare optional properties using the
?syntax rather than includingundefinedin the type.Apply this diff:
- type: string | undefined; + type?: string; - format: string | undefined; + format?: string;Also applies to: 60-62
apps/backoffice-v2/src/lib/blocks/utils/sort-data.ts (1)
12-12: Handle potentialundefinedvalues inorderedDataWhen mapping
predefinedOrder, if an item is not found indata,findreturnsundefined. This can lead toorderedDatacontainingundefinedvalues, which are later filtered out. Consider handling this case explicitly to avoid unexpected gaps in the ordered list.Apply this diff to filter out
undefinedvalues immediately:const orderedData = predefinedOrder .map(key => data.find(item => item.title === key)) + .filter((item): item is TObj => item !== undefined);apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/EditableDetailsV2.tsx (1)
23-24: Use inclusive terminology instead of 'blacklist' and 'whitelist'Consider replacing
blacklistwithblocklistandwhitelistwithallowlistto promote inclusive language.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/EditableDetailsV2.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailsV2Options.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/types.ts(1 hunks)apps/backoffice-v2/src/domains/workflow-definitions/fetchers.ts(1 hunks)apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx(3 hunks)apps/backoffice-v2/src/lib/blocks/utils/sort-data.ts(1 hunks)services/workflows-service/src/workflow/schemas/zod-schemas.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2.tsx
🔇 Additional comments (8)
services/workflows-service/src/workflow/schemas/zod-schemas.ts (1)
76-83: Verify schema synchronization mechanism.
The editableContext schema matches its counterpart in the backoffice. However, we should ensure these schemas stay synchronized.
Note that this schema uses strict() while the backoffice version uses passthrough(). This difference is acceptable as it enforces stricter validation in the service layer while allowing flexibility in the UI layer.
Let's verify no other workflow definition schemas exist that might need this update:
✅ Verification successful
Schema synchronization is properly maintained
The verification shows that editableContext schema is only defined in two locations:
services/workflows-service/src/workflow/schemas/zod-schemas.ts(service layer)apps/backoffice-v2/src/domains/workflow-definitions/fetchers.ts(UI layer)
This matches the expected architecture where we have corresponding schemas in both layers. The usage in useKycBlock.tsx is a proper consumer of this schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other workflow definition schemas that might need synchronization
rg -l "WorkflowDefinition.*Schema" --type typescript
# Search for uses of editableContext to ensure consistent usage
rg "editableContext" --type typescript
Length of output: 163
Script:
#!/bin/bash
# Search for workflow definition schemas without file type restriction
rg -l "WorkflowDefinition.*Schema"
# Search for editableContext usage without file type restriction
rg "editableContext" -A 2
Length of output: 2040
apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx (3)
399-412: Review action button disabled logic for consistency
The disabled property for action buttons in the config.actions object uses different logic:
- Line 399:
options.disabledis set to!caseState.writeEnabled. - Line 402:
enableEditing.disabledis set toisEditable. - Line 405:
editing.disabledis set to!isEditable || !caseState.writeEnabled. - Line 411:
save.disabledis set to!caseState.writeEnabled.
Verify that these conditions correctly reflect the intended enablement states of the buttons, ensuring users have the appropriate access based on isEditable and caseState.writeEnabled.
498-498: Check for correct usage of getEntityDataBlock
In the value prop:
value: getEntityDataBlock(),Ensure that getEntityDataBlock does not have side effects and that its return value is consistent between renders. If getEntityDataBlock depends on props or state, verify that it updates correctly when those dependencies change.
435-442: 🛠️ Refactor suggestion
Ensure entity.data is defined before mapping
When mapping over childWorkflow?.context?.entity?.data, ensure that entity.data is defined to prevent runtime errors. If entity.data can be undefined, consider providing a default empty object or adding a null check.
Apply this change:
- Object.entries(childWorkflow?.context?.entity?.data ?? {}).map(
+ Object.entries(childWorkflow?.context?.entity?.data || {}).map(Likely invalid or redundant comment.
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/types.ts (1)
54-54: Consider using a more specific type for value
Using any reduces type safety. Consider specifying a more precise type or using a generic to enhance type checking.
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailsV2Options.tsx (1)
25-31: Improve accessibility by adding an aria-label to the trigger button
To enhance accessibility, add an aria-label to the trigger button to provide a textual description for screen readers.
Apply this diff:
<Button
variant="outline"
className={'px-2 py-0 text-xs aria-disabled:pointer-events-none aria-disabled:opacity-50'}
aria-disabled={actions.options.disabled}
+ aria-label="Open options menu"
>
Options
</Button>apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx (2)
117-119: Use deep equality check for value comparison
Using === may not accurately compare complex objects or arrays. Consider using a deep equality check like isEqual from lodash-es.
Apply this diff:
- if (value === defaultValue) {
+ if (isEqual(value, defaultValue)) {
Also, import isEqual at the top of the file:
+ import { isEqual } from 'lodash-es';121-125: Ensure pathToObject is correctly derived
If curr.path does not contain a dot, pathToObject becomes an empty string, which means setting .id at the root level. Verify if this is the intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
packages/common/CHANGELOG.md (2)
3-8: Enhance the changelog entry with more specific details.The current changelog entry "Updated merchant screening schema" is too vague. Consider providing more specific details about what was changed, why it was changed, and any potential impact. Based on the AI summary, this could be improved to:
## 0.9.57 ### Patch Changes -Updated merchant screening schema +- Made PhoneNumber field optional in PrincipalSchema and MerchantSchema within the merchant screening schemaThis helps other developers understand:
- What exactly changed (PhoneNumber field)
- How it changed (made optional)
- Where it changed (in PrincipalSchema and MerchantSchema)
Line range hint
1-999: Consider implementing more detailed changelog practices.The changelog shows a pattern of frequent version bumps with minimal descriptions. This makes it difficult to:
- Track meaningful changes
- Understand the impact of updates
- Plan for upgrades
- Debug issues
Consider adopting a more structured changelog format like Keep a Changelog, which categorizes changes as:
- Added
- Changed
- Deprecated
- Removed
- Fixed
- Security
Each entry should describe:
- What changed
- Why it changed (if relevant)
- Migration steps (if needed)
apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx (4)
359-369: Add error handling feedback for failed mutations.While the success case is handled by toggling off edit mode, there's no user feedback for failed mutations. Consider adding error handling to improve user experience.
const { mutate: mutateUpdateContextAndSyncEntity } = useUpdateContextAndSyncEntityMutation({ workflowId: childWorkflow?.id, onSuccess: () => { toggleOffIsEditable(); }, + onError: (error) => { + // Show error notification/toast to user + console.error('Failed to update entity:', error); + }, });
371-376: Consider using a more specific type for form values.The current type
Record<PropertyKey, any>is very generic. Consider creating a specific type that matches your entity data structure for better type safety.- (values: Record<PropertyKey, any>) => { + (values: { + entity: { + data: { + firstName?: string; + lastName?: string; + // ... other expected fields + }; + }; + }) => {
378-452: Consider splitting the entity data block logic into smaller functions.The
getEntityDataBlockfunction is quite large and handles multiple responsibilities. Consider breaking it down into smaller, more focused functions for better maintainability.+ const getEditableEntityDataBlock = () => { + return createBlocksTyped() + .addBlock() + .addCell({ + type: 'editableDetails', + value: fields, + props: { + // ... editable props + }, + }) + .build() + .flat(1); + }; + + const getReadOnlyEntityDataBlock = () => { + return createBlocksTyped() + .addBlock() + // ... readonly implementation + .build() + .flat(1); + }; + const getEntityDataBlock = () => { if (parentWorkflow?.workflowDefinition?.config?.editableContext?.kyc?.entity) { - return createBlocksTyped()... + return getEditableEntityDataBlock(); } - return createBlocksTyped()... + return getReadOnlyEntityDataBlock(); };
390-400: Document parse configuration and remove empty blacklist.The parse configuration is comprehensive but lacks documentation. Also, the empty blacklist array could be removed if not used.
config: { parse: { + // Convert string inputs to their respective types date: true, // Convert date strings to Date objects isoDate: true, // Handle ISO date format datetime: true, // Handle datetime strings boolean: true, // Convert 'true'/'false' strings to boolean url: true, // Validate URL format nullish: true, // Handle null/undefined values }, - blacklist: [], // Remove if not used actions: {services/workflows-service/src/workflow/workflow.controller.external.ts (1)
594-605: Add error handling and success response documentationThe endpoint implementation could be improved with:
- Explicit error handling for service calls
- API documentation for successful responses
+ @ApiResponse({ + status: 200, + description: 'Context updated and entity synced successfully', + schema: Type.Object({ + // Add expected response properties based on workflowService.updateContextAndSyncEntity return type + }), + }) async updateContextAndSyncEntity( @common.Param('workflowRuntimeDataId') workflowRuntimeDataId: string, @common.Body() body: PartialDeep<DefaultContextSchema>, @CurrentProject() projectId: TProjectId, ) { + try { return await this.workflowService.updateContextAndSyncEntity({ workflowRuntimeDataId, context: body, projectId, }); + } catch (error) { + if (isRecordNotFoundError(error)) { + throw new errors.NotFoundException( + `No workflow runtime data found for ${workflowRuntimeDataId}`, + ); + } + throw error; + } }services/workflows-service/src/workflow/workflow.service.ts (1)
2686-2698: Consider extracting adapter functions for reusabilityThe
endUserContextToEntityAdapterandbusinessContextToEntityAdapterfunctions are defined within theupdateContextAndSyncEntitymethod. If these adapters are needed elsewhere, consider moving them to a utility module to improve maintainability and reusability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
apps/backoffice-v2/CHANGELOG.md(1 hunks)apps/backoffice-v2/package.json(4 hunks)apps/backoffice-v2/src/domains/workflows/fetchers.ts(1 hunks)apps/backoffice-v2/src/domains/workflows/hooks/mutations/useUpdateContextAndSyncEntity/useUpdateContextAndSyncEntity.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx(3 hunks)apps/kyb-app/CHANGELOG.md(1 hunks)apps/kyb-app/package.json(2 hunks)examples/headless-example/CHANGELOG.md(1 hunks)examples/headless-example/package.json(2 hunks)packages/common/CHANGELOG.md(1 hunks)packages/common/package.json(1 hunks)packages/common/src/schemas/documents/merchant-screening-plugin-schema.ts(2 hunks)packages/workflow-core/CHANGELOG.md(1 hunks)packages/workflow-core/package.json(2 hunks)sdks/web-ui-sdk/CHANGELOG.md(1 hunks)sdks/web-ui-sdk/package.json(2 hunks)sdks/workflow-browser-sdk/CHANGELOG.md(1 hunks)sdks/workflow-browser-sdk/package.json(2 hunks)sdks/workflow-node-sdk/CHANGELOG.md(1 hunks)sdks/workflow-node-sdk/package.json(2 hunks)services/workflows-service/CHANGELOG.md(1 hunks)services/workflows-service/package.json(2 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/workflow/workflow.controller.external.ts(2 hunks)services/workflows-service/src/workflow/workflow.service.ts(3 hunks)websites/docs/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- packages/common/package.json
- sdks/workflow-node-sdk/package.json
- packages/workflow-core/CHANGELOG.md
- examples/headless-example/package.json
- sdks/web-ui-sdk/CHANGELOG.md
- sdks/workflow-browser-sdk/CHANGELOG.md
- apps/kyb-app/CHANGELOG.md
- examples/headless-example/CHANGELOG.md
- sdks/workflow-node-sdk/CHANGELOG.md
- services/workflows-service/CHANGELOG.md
- apps/backoffice-v2/CHANGELOG.md
- services/workflows-service/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/backoffice-v2/src/domains/workflows/fetchers.ts
- apps/backoffice-v2/src/domains/workflows/hooks/mutations/useUpdateContextAndSyncEntity/useUpdateContextAndSyncEntity.tsx
- services/workflows-service/prisma/data-migrations
- apps/backoffice-v2/package.json
🔇 Additional comments (11)
websites/docs/package.json (1)
20-20: LGTM! Verify version compatibility.
The version bump of @ballerine/common from ^0.9.56 to ^0.9.57 follows semantic versioning and aligns with other packages in the monorepo.
Let's verify the version and check for any breaking changes:
apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx (2)
24-26: LGTM: New imports are well-organized and purposeful.
The imports are correctly placed and follow the existing import pattern, bringing in necessary functionality for the editable entity data feature.
501-501: LGTM: Clean integration of the entity data block.
The new entity data block is well-integrated into the existing container structure.
apps/kyb-app/package.json (2)
4-4: Version bump follows semantic versioning
The patch version increment (0.3.90 -> 0.3.91) is appropriate for these dependency updates as they represent backward-compatible changes.
4-4: Verify dependency versions support editable entity functionality
The version updates appear to be part of a coordinated update across multiple Ballerine projects. Let's verify these versions support the new editable entity feature.
Also applies to: 19-19, 21-21
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Version updates align with editable entity functionality
The version updates in the kyb-app's package.json are verified to support editable entity functionality:
- The implementation in
useKycBlockshows entity editing capabilities with proper configuration checks (editableContext.kyc.entity) - The common package (0.9.57) and workflow-browser-sdk (0.6.75) versions are consistently updated across related packages
- The EntityData interface in the kyb-app confirms the editable fields structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the updated dependencies contain the necessary changes for editable entity functionality
# Check for editable entity related changes in @ballerine/common
echo "Checking @ballerine/common for editable entity changes..."
rg -l "editableEntity|EntityEditor"
# Check for workflow changes related to entity editing
echo "Checking workflow-browser-sdk for entity editing capabilities..."
rg -l "updateEntity|editEntity"
Length of output: 335
Script:
#!/bin/bash
# Let's try a different approach to find entity editing related changes
# Search for entity-related changes in the codebase
echo "Searching for entity editing patterns..."
rg -l -e "entity.*edit" -e "edit.*entity" -e "updateEntityData" -e "modifyEntity" -e "entityModification"
# Look for workflow configuration files that might contain entity editing settings
echo "Checking workflow configurations..."
fd -e json -e yaml -e yml | xargs rg -l "entity"
# Check for recent changes in relevant files
echo "Checking package changes..."
cat apps/kyb-app/package.json
# Look for documentation or configuration mentioning entity editing
echo "Checking documentation..."
fd README.md CHANGELOG.md | xargs rg -l "edit.*entity|entity.*edit"
Length of output: 4162
Script:
#!/bin/bash
# Let's examine the KycBlock component that contains entity editing functionality
echo "Checking KycBlock implementation..."
cat apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx
# Let's also check the recent changes in the common package and workflow-browser-sdk
echo "Checking package versions in related projects..."
fd package.json -t f | xargs rg "@ballerine/(common|workflow-browser-sdk)"
# Check for any configuration or types related to entity editing
echo "Checking for entity-related types and configurations..."
rg -A 5 "interface.*Entity"
Length of output: 23370
packages/common/src/schemas/documents/merchant-screening-plugin-schema.ts (2)
218-222: Breaking Change: MerchantSchema.PhoneNumber is now optional
Similar to the PrincipalSchema change, this makes the merchant's phone number optional. The change maintains consistency with the PrincipalSchema modification.
Let's verify the impact:
Recommendations:
- Ensure all validation logic is updated to handle optional phone numbers
- Update API documentation to reflect this change
- Consider adding runtime validation to gracefully handle cases where phone numbers are expected by external systems
116-122: Breaking Change: PrincipalSchema.PhoneNumber is now optional
This change makes the phone number field optional in the PrincipalSchema. While this provides more flexibility, it's a breaking change that could affect existing implementations.
Let's verify the impact:
Consider:
- Adding a migration guide for this breaking change
- Documenting this change in the changelog
- Incrementing the major version number according to semver
services/workflows-service/src/workflow/workflow.controller.external.ts (2)
43-43: LGTM! Import statements are correctly added
The new imports provide necessary types for the endpoint's type safety:
DefaultContextSchemafor workflow context validationPartialDeepfor handling partial updates of nested objects
Also applies to: 48-48
564-564: Verify endpoint usage consistency
Let's verify the integration of this new endpoint with other workflow-related endpoints and its usage patterns.
✅ Verification successful
Endpoint pattern is consistent with existing workflow endpoints
Based on the verification results:
- The new
/:workflowRuntimeDataId/sync-entityPATCH endpoint follows similar patterns as other workflow-related PATCH endpoints in the codebase - The
updateContextAndSyncEntitymethod is properly integrated in both the service layer (workflow.service.ts) and the frontend (backoffice-v2) - The endpoint follows the established pattern of workflow context updates seen in other controllers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar endpoint patterns and usage
# Check for similar PATCH endpoints in workflow controllers
echo "Checking for similar PATCH endpoints..."
rg -A 2 "@common\.Patch\(" --type ts
# Check for updateContextAndSyncEntity usage
echo "Checking for updateContextAndSyncEntity usage..."
rg "updateContextAndSyncEntity" --type ts
# Check for similar context update patterns
echo "Checking for similar context update patterns..."
ast-grep --pattern 'workflowService.updateContext$$$'
Length of output: 15100
services/workflows-service/src/workflow/workflow.service.ts (2)
2705-2711: Add error handling for business entity update
The call to entityRepository.business.updateById lacks error handling. Consider wrapping this call in a try-catch block to handle potential database errors.
if (workflowRuntimeData.businessId && context.entity?.data) {
+ try {
await this.entityRepository.business.updateById(workflowRuntimeData.businessId, {
data: businessContextToEntityAdapter(
context.entity.data as Static<typeof BusinessDataSchema>,
),
});
+ } catch (error) {
+ this.logger.error('Failed to update business entity', {
+ businessId: workflowRuntimeData.businessId,
+ error,
+ });
+ throw error;
+ }
}2713-2717: Add error handling for end user entity update
Similarly, the call to entityRepository.endUser.updateById lacks error handling. Wrap this call in a try-catch block to handle potential errors.
if (workflowRuntimeData.endUserId && context.entity?.data) {
+ try {
await this.entityRepository.endUser.updateById(workflowRuntimeData.endUserId, {
data: endUserContextToEntityAdapter(context.entity.data),
});
+ } catch (error) {
+ this.logger.error('Failed to update end user entity', {
+ endUserId: workflowRuntimeData.endUserId,
+ error,
+ });
+ throw error;
+ }
}
Summary by CodeRabbit
New Features
EditableDetailsV2) with enhanced user interaction.EditableDetailsV2Cell) for integrating editable details within a cell context.useKycBlockhook to dynamically manage editable fields and submission handling.editableContextin workflow definitions to manage editable KYC contexts.Bug Fixes
Chores