WIP - Directors individual sanctions #2980
Conversation
…ngular not plural
|
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant DirectorBlock
participant WorkflowService
participant EndUserService
User->>DirectorBlock: Interact with director documents
DirectorBlock->>WorkflowService: Request document update
WorkflowService->>EndUserService: Create/Update end user
WorkflowService-->>DirectorBlock: Return update status
DirectorBlock->>User: Render updated document status
Suggested reviewers
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.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
apps/backoffice-v2/src/lib/blocks/hooks/useDocumentPageImages/useDocumentPageImages.ts (1)
Line range hint
14-21: Enhance null safety handlingWhile the optional chaining for
documents?.reduceis good, there are additional null safety improvements needed:
- The function should return an empty array if
documentsis null- The type casting to
DocumentPageImagesResultmay fail if the reduce operation returns undefined- Unsafe array access with
filesCopy?.shift()Consider this safer implementation:
- const result = documents?.reduce((list: DocumentPageImagesResult, document, documentIndex) => { + if (!documents?.length) return [] as DocumentPageImagesResult; + + const result = documents.reduce((list: DocumentPageImagesResult, document, documentIndex) => { (document?.pages as AnyObject[])?.forEach((_, pageIndex: number) => { if (!list[documentIndex]) { list[documentIndex] = []; } - list[documentIndex][pageIndex] = filesCopy?.shift()?.data as string; + const nextFile = filesCopy.shift(); + list[documentIndex][pageIndex] = nextFile?.data as string ?? ''; }); return list; - }, []) as DocumentPageImagesResult; + }, [] as DocumentPageImagesResult);
🧹 Nitpick comments (13)
apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx (1)
Line range hint
1-372: Consider breaking down the large hook into smaller, focused hooksThe
useDocumentBlockshook is handling multiple concerns:
- Document state management
- UI rendering logic
- Action handlers
- Decision management
This makes it harder to maintain and test. Consider splitting it into smaller, focused hooks:
useDocumentActions- for handling approve/reject/revision actionsuseDocumentDecisions- for decision-related logicuseDocumentRendering- for UI block creationWould you like me to help create a detailed plan for breaking down this hook into smaller, more manageable pieces?
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx (4)
127-127: Remove unnecessary React FragmentAt line 127, the
<React.Fragment>is unnecessary since it wraps a single child. You can simplify the code by removing the fragment.Apply this diff to remove the unnecessary fragment:
-value: <React.Fragment>Pending re-upload</React.Fragment>, +value: 'Pending re-upload',🧰 Tools
🪛 Biome (1.9.4)
[error] 127-127: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
144-151: Simplify JSX by removing redundant FragmentWithin lines 144-151, the
<React.Fragment>wraps multiple elements, but since they are within an array, you can remove the fragment for cleaner code.Apply this diff to remove the unnecessary fragment:
- value: ( - <React.Fragment> - Re-upload needed - <X - className="h-4 w-4 cursor-pointer" - onClick={() => onRemoveDecision(document.id)} - /> - </React.Fragment> - ), + value: [ + 'Re-upload needed', + <X + key="remove-decision-icon" + className="h-4 w-4 cursor-pointer" + onClick={() => onRemoveDecision(document.id)} + />, + ],
311-313: Redundant nullish coalescing operatorThe use of the
??operator on an object literal is unnecessary since the object spread will always result in an object. This means the?? {}will never return the right-hand side.Apply this diff to remove the redundant operator:
data: Object.entries( { ...additionalProperties, ...document.propertiesSchema?.properties, - } ?? {}, + }, )?.map(🧰 Tools
🪛 GitHub Actions: CI
[warning] 311-313: The "??" operator here will always return the left operand
85-420: Consider refactoring for better readability and maintainabilityThe
useMemohook from lines 85-420 contains deeply nested code with complex logic. Refactoring into smaller, reusable components or functions would improve readability and maintainability.🧰 Tools
🪛 Biome (1.9.4)
[error] 127-127: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
🪛 GitHub Actions: CI
[warning] 311-313: The "??" operator here will always return the left operand
apps/backoffice-v2/src/domains/individuals/queries/useEndUserById/useEndUserById.tsx (2)
10-12: Consider strengthening the AML hits schema validation.The schema allows for optional AML hits and vendor, which could lead to undefined behavior if the API response structure changes.
Consider adding more specific validation:
- amlHits: z.array(HitSchema.extend({ vendor: z.string().optional() })).optional(), + amlHits: z.array(HitSchema.extend({ + vendor: z.string().optional().describe('AML vendor name'), + timestamp: z.string().datetime().optional(), + })).default([]),
14-23: Consider adding retry logic for network failures.The API call could fail due to network issues, and the current implementation doesn't handle retries.
Consider adding retry logic for transient failures:
export const getEndUserById = async ({ id }: { id: string }) => { const [endUser, error] = await apiClient({ endpoint: `end-users/${id}`, method: Method.GET, schema: EndUserSchema, timeout: 30_000, + retry: { + attempts: 3, + backoff: 'exponential', + }, }); return handleZodError(error, endUser); };apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/create-directors-blocks.tsx (1)
23-25: Consider adding validation for malformed director data.The early return for empty directors array is good, but consider adding validation for malformed director objects.
if (!directors?.length) { return []; } + +// Filter out malformed director objects +const validDirectors = directors.filter(director => + director && director.id && (director.firstName || director.lastName) +); + +if (!validDirectors.length) { + console.warn('No valid directors found in the input array'); + return []; +}apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/DirectorBlock.tsx (1)
45-49: Add prop types validation for BlocksComponent children.The component should validate the structure of the cells prop.
+import PropTypes from 'prop-types'; return ( <BlocksComponent blocks={directorBlock} cells={cells}> {(Cell, cell) => <Cell {...cell} />} </BlocksComponent> ); +BlocksComponent.propTypes = { + cells: PropTypes.arrayOf( + PropTypes.shape({ + type: PropTypes.string.isRequired, + value: PropTypes.node.isRequired, + }) + ).isRequired, +};services/workflows-service/src/workflow/utils/entities-update.ts (1)
61-63: Add logging for skipped directors.Consider adding logging when skipping director creation to aid in debugging and monitoring.
if ('ballerineEntityId' in director && director.ballerineEntityId) { + console.log(`Skipping director creation - existing ballerineEntityId: ${director.ballerineEntityId}`); return; }services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts (1)
164-182: Consider batch processing for better performance.Creating directors one by one might not be optimal for large datasets. Consider implementing batch creation.
+ // Batch create directors + const directorRecords = event.context.entity.data.additionalInfo.directors?.map(director => ({ + firstName: director.firstName, + lastName: director.lastName, + email: director.email, + projectId: tokenScope.projectId, + })); + + const createdDirectors = await this.endUserService.createMany({ + data: directorRecords, + });packages/workflow-core/src/lib/plugins/external-plugin/individuals-sanctions-v2-plugin/individuals-sanctions-v2-plugin.ts (1)
242-242: Add type checking for additionalInfo destructuring.The default empty object prevents runtime errors, but consider adding type checking.
- const { dateOfBirth } = additionalInfo ?? {}; + const { dateOfBirth } = (additionalInfo ?? {}) as { dateOfBirth?: string };apps/backoffice-v2/src/lib/blocks/variants/KybExampleBlocks/hooks/useKybExampleBlocksLogic/useKybExampleBlocksLogic.tsx (1)
219-235: Consider error handling for missing document schemas.While the code handles the case where no document schema is found for a country code by logging a warning, it might be better to handle this more robustly.
Consider this improvement:
const documentSchemas = useMemo(() => { const issuerCountryCode = extractCountryCodeFromWorkflow(workflow); const documentsSchemas = issuerCountryCode ? getDocumentsByCountry(issuerCountryCode) : []; if (!Array.isArray(documentsSchemas) || !documentsSchemas.length) { - console.warn(`No document schema found for issuer country code of "${issuerCountryCode}".`); + const warning = `No document schema found for issuer country code of "${issuerCountryCode}".`; + console.warn(warning); + toast.warning(warning); } return documentsSchemas; }, [workflow]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/backoffice-v2/src/domains/individuals/queries/useEndUserById/useEndUserById.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/DirectorBlock.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/create-directors-blocks.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/helpers.ts(1 hunks)apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/index.ts(1 hunks)apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/components/DirectorsCallToAction/helpers.ts(2 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useCaseInfoBlock/useCaseInfoBlock.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useDirectorsBlocks/helpers.ts(0 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useDirectorsBlocks/index.ts(0 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useDirectorsBlocks/useDirectorsBlocks.tsx(0 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useDocumentPageImages/useDocumentPageImages.ts(1 hunks)apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx(2 hunks)apps/backoffice-v2/src/lib/blocks/variants/KybExampleBlocks/hooks/useKybExampleBlocksLogic/useKybExampleBlocksLogic.tsx(3 hunks)packages/common/src/schemas/documents/merchant-screening-plugin-schema.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/individuals-sanctions-v2-plugin/individuals-sanctions-v2-plugin.ts(4 hunks)packages/workflow-core/src/lib/workflow-runner.ts(3 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts(4 hunks)services/workflows-service/src/workflow/utils/entities-update.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/backoffice-v2/src/lib/blocks/hooks/useDirectorsBlocks/helpers.ts
- apps/backoffice-v2/src/lib/blocks/hooks/useDirectorsBlocks/index.ts
- apps/backoffice-v2/src/lib/blocks/hooks/useDirectorsBlocks/useDirectorsBlocks.tsx
✅ Files skipped from review due to trivial changes (2)
- services/workflows-service/prisma/data-migrations
- apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/index.ts
🧰 Additional context used
🪛 GitHub Actions: CI
apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx
[warning] 372-374: The "??" operator here will always return the left operand
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx
[warning] 311-313: The "??" operator here will always return the left operand
🪛 Biome (1.9.4)
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx
[error] 127-127: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (13)
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx (1)
373-375: Ensure correct usage ofdirectorAdapterAt lines 373-375, the
directorAdapteris immediately invoked withdirectorsStorageFilesQueryResultinside themapfunction. Verify thatdirectorAdapterreturns a function when called withdirectorsStorageFilesQueryResult.Run the following script to confirm that
directorAdapterreturns a function:✅ Verification successful
Correct usage of
directorAdapterconfirmedThe
directorAdapteris correctly implemented as a curried function and is being used appropriately in the code. The first call withdirectorsStorageFilesQueryResultreturns a function that is then used to process individual director data within the map function.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'directorAdapter' is used correctly. # Test: Search for the definition of 'directorAdapter' to ensure it returns a function. ast-grep --pattern $'const directorAdapter = ($_$) => ($_$) => { $$$ }' --lang ts # Expected Result: Matches the 'directorAdapter' definition that returns a function.Length of output: 82
Script:
#!/bin/bash # Search for any occurrence of directorAdapter rg "directorAdapter" -A 5 -B 5 # Also look specifically in the file where it's being used cat "apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx"Length of output: 23606
apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx (2)
373-375: Verify correct usage ofdirectorAdapterAt lines 373-375, similar to the previous file, ensure that
directorAdapteris being used correctly and returns a function when called.
332-334:⚠️ Potential issueMissing dependency in
useCallbackhookIn the
onMutateApproveTaskByIdDirectorsfunction at lines 332-334,workflow?.idis used but not included in the dependency array. This could lead to stale references ifworkflow?.idchanges.Apply this diff to include
workflow?.idin the dependency array:const onMutateApproveTaskByIdDirectors = useCallback( (documentId: string) => () => mutateApproveTaskById({ documentId, contextUpdateMethod: 'director' }), - [mutateApproveTaskById], + [mutateApproveTaskById, workflow?.id], );Likely invalid or redundant comment.
apps/backoffice-v2/src/lib/blocks/components/DirectorsCallToAction/helpers.ts (1)
Line range hint
5-27: Ensure default revision reasons are providedIn the updated
getRevisionReasonsForDocumentfunction, if none of the specific conditions match, the function returnsrevisionReasons. Ensure thatrevisionReasonsis not undefined to avoid potential runtime errors.If
revisionReasonscan be undefined, consider providing a default value or handling the undefined case.Apply this diff to provide a default value:
return revisionReasons || [];apps/backoffice-v2/src/domains/individuals/queries/useEndUserById/useEndUserById.tsx (1)
32-39: LGTM! Well-structured query hook with proper authentication check.The hook correctly checks for both ID presence and authentication status before enabling the query.
apps/backoffice-v2/src/lib/blocks/hooks/useCaseInfoBlock/useCaseInfoBlock.tsx (1)
19-23: LGTM! Defensive programming improvement.The addition of optional chaining operators makes the code more robust by safely handling potentially undefined properties in the workflow object hierarchy.
services/workflows-service/src/workflow/utils/entities-update.ts (1)
61-63: LGTM! Prevents duplicate EndUser creation.The check for existing
ballerineEntityIdprevents redundant processing and maintains data integrity.packages/workflow-core/src/lib/plugins/external-plugin/individuals-sanctions-v2-plugin/individuals-sanctions-v2-plugin.ts (1)
48-52: Add validation for context assignment.When assigning the entire context, consider adding validation to prevent potential security issues.
✅ Verification successful
Context assignment is properly validated and secure
The context assignment is safe as it's part of a comprehensive validation system:
- The entire payload is validated through strict Zod schemas
- The context data undergoes multiple validation layers before being used
- The transformed data is validated before making API requests
- The plugin implements type-safe KYC information handling
The '*' path pattern is a deliberate design choice for passing the full context when needed, and the implementation includes proper safeguards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for sensitive information patterns in context assignments rg -g '*.{ts,tsx}' "context\s*=\s*\*" -A 5Length of output: 44
Script:
#!/bin/bash # Search for context assignments in plugin files rg -g '*.ts' -A 5 'acc\[.*\]\s*=\s*context' # Look for similar property.type === 'path' patterns ast-grep --pattern 'if ($_ === "path") { $$$ }' # Search for validation patterns around context rg -g '*.ts' -B 3 -A 3 'validateContext|sanitize.*context|context.*validation'Length of output: 1767
Script:
#!/bin/bash # Search for workflow context types and interfaces rg -g '*.ts' -B 2 -A 5 'interface.*Context|type.*Context' # Look for the plugin's class definition and imports ast-grep --pattern 'class IndividualsSanctionsV2Plugin { $$$ }' # Search for plugin validation patterns rg -g '*.ts' -B 2 -A 5 'validate.*Plugin|plugin.*validation'Length of output: 45448
Script:
#!/bin/bash # Search for the plugin's validation and schema rg -g '*.ts' -B 3 -A 5 'IndividualsSanctionsV2Plugin.*schema|IndividualsSanctionsV2Plugin.*validate' # Look for the plugin's property handling ast-grep --pattern 'if (property.type === "path") { $$$ }'Length of output: 165
Script:
#!/bin/bash # Get the complete plugin file content cat packages/workflow-core/src/lib/plugins/external-plugin/individuals-sanctions-v2-plugin/individuals-sanctions-v2-plugin.ts # Search for similar property path handling patterns in other plugins rg -g '*.ts' -B 3 -A 5 'property\.type\s*===\s*.(path|PATH).'Length of output: 12822
packages/common/src/schemas/documents/merchant-screening-plugin-schema.ts (1)
92-99: Verify the impact of making the Country field optional.The change makes the
Countryfield optional in theDriversLicenseSchema. While this provides more flexibility, it's important to ensure that:
- This change aligns with business requirements for driver's license validation.
- Any code that consumes this schema can handle cases where the country code is not provided.
Run the following script to check for direct usage of this field:
✅ Verification successful
Making the Country field optional is safe to proceed
The verification shows that the
Countryfield inDriversLicenseSchemais not directly used in merchant screening validation logic. The merchant screening implementation sources country data from the entity's address information, making this change safe to implement.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for direct usage of the Country field from DriversLicenseSchema # Test: Search for code that might be affected by this field becoming optional # Look for direct property access rg -A 5 "DriversLicense.*Country" # Look for potential validation logic rg -A 5 "Country.*ISO.*3166" # Look for schema validation code ast-grep --pattern 'validate($_, { Country: $_ })'Length of output: 119
Script:
#!/bin/bash # Search for files importing or using the merchant screening plugin schema rg -l "merchant-screening-plugin-schema" # Look for merchant screening related files and validation logic rg -l "merchantScreening|merchant.*screening" # Search for country code validation patterns in TypeScript/JavaScript files fd -e ts -e js --exec rg -l "countryCode|country.*code|ISO.*3166" # Look for type definitions or interfaces using this schema ast-grep --pattern 'import { $_ } from "./merchant-screening-plugin-schema"' ast-grep --pattern 'type $_ = { Country?: string }'Length of output: 3597
Script:
#!/bin/bash # Check how the schema is used in default context cat packages/common/src/schemas/documents/default-context-schema.ts # Look for any direct Country field validation in merchant screening plugin cat packages/workflow-core/src/lib/plugins/external-plugin/mastercard-merchant-screening-plugin.tsLength of output: 7227
apps/backoffice-v2/src/lib/blocks/variants/KybExampleBlocks/hooks/useKybExampleBlocksLogic/useKybExampleBlocksLogic.tsx (2)
171-217: LGTM! Well-structured mutation handlers for director actions.The implementation follows a clean pattern of separating mutation handlers for different director-related actions (revision, approval, decision removal). Each handler correctly uses the 'director' context update method.
236-248: LGTM! Comprehensive director blocks configuration.The
createDirectorsBlockscall is well-structured with all necessary props for handling director-related functionality, including proper loading states and permissions.packages/workflow-core/src/lib/workflow-runner.ts (2)
306-306: LGTM! Improved type safety.The return type has been updated to explicitly omit 'actionPluginName' from IterativePluginParams, which better reflects the actual structure of the returned object.
321-325: LGTM! Added runtime validation for action plugin existence.Good addition of an invariant check to ensure the action plugin exists before proceeding. This provides better error messages and fail-fast behavior when an action plugin is missing.
apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx
Show resolved
Hide resolved
...2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/create-directors-blocks.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/DirectorBlock.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/helpers.ts
Show resolved
Hide resolved
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/helpers.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts
Outdated
Show resolved
Hide resolved
Omri-Levy
changed the title
services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/domains/individuals/queries/useEndUserById/useEndUserById.tsx
Outdated
Show resolved
Hide resolved
...ffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx (1)
373-389: Consider extracting shared transformation logic.The director data transformation and document schema retrieval logic is duplicated between
useKybExampleBlocksLogicanduseDefaultBlocksLogic. Consider extracting this into a shared utility function.Create a new utility file
getDirectorDocumentSchemas.ts:import { TWorkflowById } from '@/domains/workflows/fetchers'; import { getDocumentsByCountry } from '@ballerine/common'; import { extractCountryCodeFromWorkflow } from '@/pages/Entity/hooks/useEntityLogic/utils'; export const getDirectorDocumentSchemas = (workflow: TWorkflowById | undefined) => { const issuerCountryCode = extractCountryCodeFromWorkflow(workflow); const documentsSchemas = issuerCountryCode ? getDocumentsByCountry(issuerCountryCode) : []; if (!Array.isArray(documentsSchemas) || !documentsSchemas.length) { console.warn(`No document schema found for issuer country code of "${issuerCountryCode}".`); } return documentsSchemas; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx(2 hunks)apps/backoffice-v2/src/lib/blocks/variants/KybExampleBlocks/hooks/useKybExampleBlocksLogic/useKybExampleBlocksLogic.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: lint
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
apps/backoffice-v2/src/lib/blocks/variants/KybExampleBlocks/hooks/useKybExampleBlocksLogic/useKybExampleBlocksLogic.tsx (3)
17-17: LGTM! Well-organized imports.The new imports are properly organized and necessary for the enhanced director-related functionality.
Also applies to: 31-35
219-235: LGTM! Clean data transformation with proper error handling.The director data transformation and document schema retrieval are well-implemented with appropriate warning messages for edge cases.
236-248: Address the TODO comment about callToActionLegacy.The code includes a TODO comment about removing
callToActionLegacy. Please create a follow-up task to handle this technical debt.Would you like me to create an issue to track the removal of
callToActionLegacy?apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx (3)
60-65: LGTM! Consistent imports.The imports are consistent with the changes in
useKybExampleBlocksLogic.tsx.
332-371: LGTM! Well-structured task management callbacks.The callbacks are focused and handle their specific tasks (revision, approval, decision removal) without side effects.
390-402: LGTM! Consistent director blocks configuration.The configuration is consistent with the implementation in
useKybExampleBlocksLogic.tsx.
...blocks/variants/KybExampleBlocks/hooks/useKybExampleBlocksLogic/useKybExampleBlocksLogic.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts (2)
154-170:⚠️ Potential issueAdd error handling for individual end user creation
The current implementation might fail silently if creation of a single director or UBO fails.
Also applies to: 174-190
154-170: 🛠️ Refactor suggestionAdd input validation for director and UBO data
The director and UBO object structures are assumed but not validated.
Also applies to: 174-190
🧹 Nitpick comments (4)
apps/backoffice-v2/src/pages/Entity/hooks/useEntityLogic/utils.ts (2)
69-72: Add documentation and improve error handling.While the function logic is correct, consider these improvements for better maintainability and reliability:
- Add JSDoc documentation to clarify the function's purpose, parameters, and return value
- Add input validation for the documents array
- Consider handling the case when no document has an issuer country
Here's a suggested implementation:
+/** + * Extracts the country code from the first document that has an issuer country + * @param documents - Array of documents to search through + * @returns The country code if found, undefined otherwise + */ export const extractCountryCodeFromDocuments = (documents: TDocument[]) => { + if (!Array.isArray(documents)) { + throw new Error('Expected documents to be an array'); + } + return documents?.find(document => { return !!document?.issuer?.country; })?.issuer?.country; };
69-72: Consider adding type safety for the return value.The function could benefit from stronger typing and validation of the country value.
Consider using a type for valid country codes and adding validation:
type CountryCode = string; // Replace with your actual country code type export const extractCountryCodeFromDocuments = (documents: TDocument[]): CountryCode | undefined => { const document = documents?.find(doc => !!doc?.issuer?.country); const country = document?.issuer?.country; // Add validation if you have a specific format for country codes return country; };services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts (2)
154-170: Extract shared logic for creating end usersThe code for creating directors and UBOs contains duplicated logic. Consider extracting this into a shared helper method.
+ private async createEndUser( + data: { firstName: string; lastName: string; email: string }, + projectId: string, + ) { + const { id } = await this.endUserService.create({ + data: { + firstName: data.firstName, + lastName: data.lastName, + email: data.email, + projectId, + }, + }); + + return { + ballerineEntityId: id, + ...data, + }; + } async finalSubmission(@TokenScope() tokenScope: ITokenScope, @common.Body() body: FinishFlowDto) { // ... const directors = await Promise.all( - workflowRuntimeData.context.entity.data.additionalInfo.directors?.map( - async (director: { firstName: string; lastName: string; email: string }) => { - const { id } = await this.endUserService.create({ - data: { - firstName: director.firstName, - lastName: director.lastName, - email: director.email, - projectId: tokenScope.projectId, - }, - }); - - return { - ballerineEntityId: id, - ...director, - }; - }, - ), + workflowRuntimeData.context.entity.data.additionalInfo.directors?.map(director => + this.createEndUser(director, tokenScope.projectId), + ), ); const ubos = await Promise.all( - workflowRuntimeData.context.entity.data.additionalInfo.ubos?.map( - async (ubo: { firstName: string; lastName: string; email: string }) => { - const { id } = await this.endUserService.create({ - data: { - firstName: ubo.firstName, - lastName: ubo.lastName, - email: ubo.email, - projectId: tokenScope.projectId, - }, - }); - - return { - ballerineEntityId: id, - ...ubo, - }; - }, - ), + workflowRuntimeData.context.entity.data.additionalInfo.ubos?.map(ubo => + this.createEndUser(ubo, tokenScope.projectId), + ), );Also applies to: 174-190
154-155: Define proper interfaces for director and UBO typesInstead of using inline type annotations, define proper interfaces for better type safety and reusability.
interface EndUserData { firstName: string; lastName: string; email: string; } interface DirectorData extends EndUserData { // Add director-specific fields if any } interface UboData extends EndUserData { // Add UBO-specific fields if any }Also applies to: 174-175
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/create-directors-blocks.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/helpers.ts(1 hunks)apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx(2 hunks)apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx(2 hunks)apps/backoffice-v2/src/lib/blocks/variants/KybExampleBlocks/hooks/useKybExampleBlocksLogic/useKybExampleBlocksLogic.tsx(3 hunks)apps/backoffice-v2/src/pages/Entity/hooks/useEntityLogic/utils.ts(1 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts(4 hunks)services/workflows-service/src/workflow/utils/entities-update.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/workflows-service/src/workflow/utils/entities-update.ts
- apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx
👮 Files not reviewed due to content moderation or server errors (5)
- apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/helpers.ts
- apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/create-directors-blocks.tsx
- apps/backoffice-v2/src/lib/blocks/variants/KybExampleBlocks/hooks/useKybExampleBlocksLogic/useKybExampleBlocksLogic.tsx
- apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx
- apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx
[error] 130-130: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test_linux
- GitHub Check: test_windows
- GitHub Check: build (windows-latest)
🔇 Additional comments (2)
services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts (2)
22-22: LGTM - Clean dependency injectionThe EndUserService is properly imported and injected following NestJS dependency injection patterns.
Also applies to: 33-33
193-213: LGTM - Proper context update with array replacementThe workflow context update correctly uses the REPLACE array merge option to update the directors and UBOs data.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
apps/backoffice-v2/src/lib/blocks/variants/KybExampleBlocks/hooks/useKybExampleBlocksLogic/useKybExampleBlocksLogic.tsx (1)
221-228: Consider performance and type safety improvements.
- Memoize the directors transformation to prevent unnecessary recalculations on re-renders.
- Add type safety for the revision reasons extraction.
Apply this diff to implement the suggestions:
+ const directors = useMemo( + () => + workflow?.context?.entity?.data?.additionalInfo?.directors?.map( + directorAdapter(directorsDocumentPagesResults), + ), + [workflow?.context?.entity?.data?.additionalInfo?.directors, directorsDocumentPagesResults], + ); + type RevisionReason = { + enum?: string[]; + [key: string]: unknown; + }; const revisionReasons = - workflow?.workflowDefinition?.contextSchema?.schema?.properties?.documents?.items?.properties?.decision?.properties?.revisionReason?.anyOf?.find( - ({ enum: enum_ }) => !!enum_, - )?.enum ?? []; + workflow?.workflowDefinition?.contextSchema?.schema?.properties?.documents?.items?.properties?.decision?.properties?.revisionReason?.anyOf?.find( + ({ enum: enum_ }: RevisionReason) => !!enum_, + )?.enum ?? [];apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx (1)
139-139: Remove unnecessaryReact.FragmentAt line 139, the
React.Fragmentwraps only a single child, making it redundant. You can simplify the code by removing it.Apply this diff to remove the unnecessary
React.Fragment:- value: <React.Fragment>Pending re-upload</React.Fragment>, + value: 'Pending re-upload',🧰 Tools
🪛 Biome (1.9.4)
[error] 139-139: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx (1)
330-332: Add error handling for undefined workflow ID.While the mutation hooks are correctly initialized, consider adding error handling for when
workflow?.idis undefined. Also, consider tracking the loading state formutateRemoveDecisionTaskByIdas you do withmutateApproveTaskById.- const { mutate: mutateRemoveDecisionTaskById } = useRemoveDecisionTaskByIdMutation(workflow?.id); + const { mutate: mutateRemoveDecisionTaskById, isLoading: isLoadingRemoveDecision } = + useRemoveDecisionTaskByIdMutation(workflow?.id); + + if (!workflow?.id) { + throw new Error('Workflow ID is required for director task management'); + }apps/backoffice-v2/src/domains/entities/hooks/mutations/useRevisionTaskByIdMutation/useRevisionTaskByIdMutation.tsx (2)
18-18: Consider enhancing error handlingWhile
directorIdhas been added consistently with other mutation hooks, consider including it in error messages to help with debugging director-specific issues.onError: (_error, variables, context) => { const workflowById = workflowsQueryKeys.byId({ workflowId: variables?.workflowId, filterId }); toast.error( t(`toast:ask_revision_document.error`, { errorMessage: _error.message, + directorId: variables?.directorId, }), ); queryClient.setQueryData(workflowById.queryKey, context.previousWorkflow); },Also applies to: 24-24, 32-32
16-35: Consider implementing a shared type for mutation parametersAll three mutation hooks (
useApproveTaskByIdMutation,useRemoveDecisionTaskByIdMutation, anduseRevisionTaskByIdMutation) share similar parameter structures withdirectorId. Consider creating a shared type to maintain consistency and reduce duplication.Create a new type in a shared location:
type DocumentDecisionMutationParams = { documentId: string; directorId?: string; contextUpdateMethod?: 'base' | 'director'; // Add other common parameters };Then use it in all mutation hooks:
mutationFn: ({ directorId, documentId, contextUpdateMethod, ...rest }: DocumentDecisionMutationParams & { // Add hook-specific parameters }) => { // Mutation logic }apps/backoffice-v2/src/domains/workflows/fetchers.ts (1)
303-303: LGTM! Consider using a type alias for the decision body.The addition of the optional
directorIdparameter is well-placed. To improve type safety and reusability, consider extracting the decision body type into a separate type alias.type WorkflowDecisionBody = { decision: string | null; directorId?: string; reason?: string; comment?: string; };services/workflows-service/src/workflow/workflow.controller.internal.ts (1)
294-294: Consider adding parameter validation.While the
directorIdis properly passed to the service layer, consider adding validation to ensure it's a valid UUID or matches the expected format when present.if (directorId && !isValidUUID(directorId)) { throw new BadRequestException('Invalid director ID format'); }services/workflows-service/src/workflow/workflow.service.ts (2)
1125-1127: Enhance the error message for better debugging.While the validation is good, the error message could be more specific by including the workflow ID and document ID in the message.
if (!directorId) { throw new BadRequestException( `Attempted to update director document without a director id. WorkflowId: ${workflowId}, DocumentId: ${documentId}`, ); }
1144-1156: Add type safety to director filtering.The director filtering logic works correctly but could benefit from proper type definitions. Consider adding interfaces for the director structure.
interface Director { ballerineEntityId: string; additionalInfo?: { documents?: Array<{ id: string; [key: string]: unknown; }>; }; } private getDirectorsDocuments( context: WorkflowRuntimeData['context'], directorId?: string, ): Array<Director['additionalInfo']['documents'][number]> { return ( this.getDirectors(context) .filter((director: Director) => { if (!directorId) return true; return director.ballerineEntityId === directorId; }) .map(director => director.additionalInfo?.documents) .filter(Boolean) .flat() || [] ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/backoffice-v2/src/domains/entities/hooks/mutations/useApproveTaskByIdMutation/useApproveTaskByIdMutation.tsx(2 hunks)apps/backoffice-v2/src/domains/entities/hooks/mutations/useRemoveDecisionTaskByIdMutation/useRemoveDecisionTaskByIdMutation.tsx(1 hunks)apps/backoffice-v2/src/domains/entities/hooks/mutations/useRevisionTaskByIdMutation/useRevisionTaskByIdMutation.tsx(1 hunks)apps/backoffice-v2/src/domains/workflows/fetchers.ts(1 hunks)apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/helpers.ts(1 hunks)apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx(2 hunks)apps/backoffice-v2/src/lib/blocks/variants/KybExampleBlocks/hooks/useKybExampleBlocksLogic/useKybExampleBlocksLogic.tsx(3 hunks)services/workflows-service/src/workflow/dtos/document-decision-update-input.ts(1 hunks)services/workflows-service/src/workflow/workflow.controller.internal.ts(1 hunks)services/workflows-service/src/workflow/workflow.service.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/helpers.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx
[error] 139-139: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
🔇 Additional comments (11)
apps/backoffice-v2/src/lib/blocks/variants/KybExampleBlocks/hooks/useKybExampleBlocksLogic/useKybExampleBlocksLogic.tsx (4)
17-17: LGTM! Well-organized imports.The new imports are logically grouped and properly scoped to their respective functionalities.
Also applies to: 31-35
175-209: Add URL validation before window.open.The
onMutateRevisionTaskByIdDirectorscallback opens a new window without validating the URL. This could lead to errors ifcollectionFlowUrlortokenis undefined.
210-219: LGTM! Well-structured mutation callbacks.The approve and remove decision callbacks are concise, properly typed, and correctly use the 'director' context update method.
229-241: LGTM! Well-structured director blocks creation.The director blocks are created with comprehensive props, including all necessary callbacks, state flags, and configuration options.
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/hooks/useDirectorBlock/useDirectorBlock.tsx (1)
1-460: Consider refactoring to improve maintainabilityThis file is quite long, which may affect readability and maintenance. Consider extracting reusable components or logic into separate files, such as the "Re-upload needed" badge component.
🧰 Tools
🪛 Biome (1.9.4)
[error] 139-139: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx (2)
60-62: LGTM! Clean imports for director document management.The new imports are well-organized and properly scoped to handle director-specific task management.
373-391: Track technical debt: Remove legacy code.The code contains a TODO comment about removing
callToActionLegacy. Consider creating a tracking issue to handle this technical debt.Would you like me to create an issue to track the removal of the
callToActionLegacydependency?Also, verify that all required properties are properly typed in the
createDirectorsBlocksparameters.✅ Verification successful
Technical debt is already being tracked across the codebase
The
callToActionLegacydeprecation is already being tracked with consistent TODO comments across multiple files. The codebase shows evidence of an ongoing migration fromcallToActionLegacyto a newCallToActioncomponent, with the legacy version marked for removal in:
- DirectorBlock component
- DefaultBlocks logic
- KybExampleBlocks logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the type definition of createDirectorsBlocks ast-grep --pattern 'interface $interface { $$$ callToActionLegacy?: $type; $$$ }'Length of output: 89
Script:
#!/bin/bash # Search for callToActionLegacy usage and createDirectorsBlocks definition rg "callToActionLegacy" -A 2 -B 2 ast-grep --pattern 'function createDirectorsBlocks($params) { $$$ }'Length of output: 4558
apps/backoffice-v2/src/domains/entities/hooks/mutations/useRemoveDecisionTaskByIdMutation/useRemoveDecisionTaskByIdMutation.tsx (1)
16-16: Verify if directorId affects UI stateThe
directorIdparameter has been added to the mutation, but the optimistic update inonMutatedoesn't reflect any changes related to it. Please verify ifdirectorIdaffects the UI state and whether it needs to be handled in the optimistic update.Also applies to: 20-20, 27-27
✅ Verification successful
Optimistic update implementation is correct
The
directorIdis only used to track which director performed the action in the API context and doesn't affect the immediate UI state. The optimistic update correctly focuses on nullifying the decision status fields, which are the only UI-relevant changes when removing a decision task.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for UI components or state management code that might be affected by directorId rg -g '*.tsx' -g '*.ts' -l 'directorId.*decision' apps/backoffice-v2/src/Length of output: 75
Script:
#!/bin/bash # Search for directorId usages in the codebase echo "=== DirectorId usages ===" rg -g '*.{tsx,ts}' 'directorId' apps/backoffice-v2/src/ echo -e "\n=== Decision task related files ===" rg -g '*.{tsx,ts}' -l 'DecisionTask' apps/backoffice-v2/src/ echo -e "\n=== Mutation implementation ===" cat apps/backoffice-v2/src/domains/entities/hooks/mutations/useRemoveDecisionTaskByIdMutation/useRemoveDecisionTaskByIdMutation.tsxLength of output: 8217
apps/backoffice-v2/src/domains/entities/hooks/mutations/useApproveTaskByIdMutation/useApproveTaskByIdMutation.tsx (1)
16-16: Consider updating optimistic update logicWhile
directorIdhas been added to the mutation parameters and API call, the optimistic update inonMutatedoesn't handle this new field. IfdirectorIdaffects the UI state (e.g., showing which director approved), the optimistic update should be enhanced to maintain UI consistency.Also applies to: 21-21, 30-30
services/workflows-service/src/workflow/workflow.service.ts (2)
823-830: LGTM! Parameter additions are consistent.The
directorIdparameter is consistently added across the method signatures, maintaining proper type safety with the optional modifier.
908-908: LGTM! Proper parameter propagation.The
directorIdis correctly passed through the document update chain, maintaining consistency in the workflow.Also applies to: 930-930
.../src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx
Show resolved
Hide resolved
services/workflows-service/src/workflow/dtos/document-decision-update-input.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/DirectorBlock.tsx (1)
22-29:⚠️ Potential issueAdd loading and error states for end user data fetching.
The component doesn't handle loading or error states from the useEndUserByIdQuery hook.
🧹 Nitpick comments (3)
apps/backoffice-v2/src/domains/individuals/queries/useEndUserByIdQuery/useEndUserByIdQuery.tsx (1)
8-11: Consider adding staleTime configuration for performance optimization.While the implementation is correct, adding a staleTime configuration could optimize performance by reducing unnecessary refetches of relatively static end-user data.
return useQuery({ ...endUsersQueryKeys.byId({ id }), enabled: !!id && isAuthenticated, + staleTime: 5 * 60 * 1000, // 5 minutes });apps/backoffice-v2/src/domains/individuals/fetchers.ts (1)
11-13: Strengthen the validation for amlHits array.Consider adding more strict validation for the amlHits array to ensure data integrity and type safety.
export const EndUserSchema = z.object({ - amlHits: z.array(HitSchema.extend({ vendor: z.string().optional() })).optional(), + amlHits: z + .array( + HitSchema.extend({ + vendor: z.string().min(1).optional(), + }), + ) + .default([]), });apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/DirectorBlock.tsx (1)
46-48: Consider memoizing the Cell render function.The Cell render function could be memoized to prevent unnecessary re-renders.
+ const renderCell = React.useCallback((Cell, cell) => <Cell {...cell} />, []); return ( <BlocksComponent blocks={directorBlock} cells={cells}> - {(Cell, cell) => <Cell {...cell} />} + {renderCell} </BlocksComponent> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/backoffice-v2/src/domains/individuals/fetchers.ts(1 hunks)apps/backoffice-v2/src/domains/individuals/queries/useEndUserByIdQuery/useEndUserByIdQuery.tsx(1 hunks)apps/backoffice-v2/src/domains/individuals/query-keys.ts(1 hunks)apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/DirectorBlock.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (windows-latest)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: Analyze (javascript)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
🔇 Additional comments (2)
apps/backoffice-v2/src/domains/individuals/query-keys.ts (1)
4-9: LGTM! Well-structured query key factory implementation.The query key structure follows React Query best practices, with proper separation of concerns between key generation and data fetching.
apps/backoffice-v2/src/lib/blocks/components/DirectorBlock/DirectorBlock.tsx (1)
23-29: 🛠️ Refactor suggestionMake aml object construction more robust.
The current implementation could lead to undefined values being passed down. Consider providing default values and type safety.
const directorWithAml = { ...director, aml: { - vendor: endUser?.amlHits?.find(({ vendor }) => !!vendor)?.vendor, - hits: endUser?.amlHits, + vendor: endUser?.amlHits?.find(({ vendor }) => !!vendor)?.vendor ?? null, + hits: endUser?.amlHits ?? [], }, };Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx (1)
360-369:⚠️ Potential issueAdd error handling and loading state feedback.
The approval and removal callbacks need error handling and loading state feedback, as previously suggested.
Apply this diff to improve error handling:
const onMutateApproveTaskByIdDirectors = useCallback( - ({ directorId, documentId }: { directorId: string; documentId: string }) => - mutateApproveTaskById({ directorId, documentId, contextUpdateMethod: 'director' }), + ({ directorId, documentId }: { directorId: string; documentId: string }) => { + if (!directorId || !documentId) { + toast.error('Invalid director or document id'); + return; + } + toast.loading('Approving document...'); + mutateApproveTaskById({ + directorId, + documentId, + contextUpdateMethod: 'director' + }).catch(error => { + toast.error(`Failed to approve document: ${error.message}`); + }); + }, [mutateApproveTaskById], ); const onMutateRemoveDecisionTaskByIdDirectors = useCallback( - ({ directorId, documentId }: { directorId: string; documentId: string }) => - mutateRemoveDecisionTaskById({ directorId, documentId, contextUpdateMethod: 'director' }), + ({ directorId, documentId }: { directorId: string; documentId: string }) => { + if (!directorId || !documentId) { + toast.error('Invalid director or document id'); + return; + } + toast.loading('Removing decision...'); + mutateRemoveDecisionTaskById({ + directorId, + documentId, + contextUpdateMethod: 'director' + }).catch(error => { + toast.error(`Failed to remove decision: ${error.message}`); + }); + }, [mutateRemoveDecisionTaskById], );
🧹 Nitpick comments (3)
packages/workflow-core/src/lib/workflow-runner.ts (1)
321-322: Consider improving type safety.The
@ts-expect-errorcomment could be avoided by properly typing theparamsparameter.- // @ts-expect-error -- params is type unknown, changing it would mean updating multiple places - invariant( + invariant<{actionPluginName?: string}>(apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx (2)
328-331: Consider adding loading state for removeDecision mutation.While
mutateApproveTaskByIdtracks its loading state,mutateRemoveDecisionTaskByIddoesn't. Consider adding loading state tracking for consistency and to prevent potential race conditions.- const { mutate: mutateRemoveDecisionTaskById } = useRemoveDecisionTaskByIdMutation(workflow?.id); + const { mutate: mutateRemoveDecisionTaskById, isLoading: isLoadingRemoveDecisionTaskById } = + useRemoveDecisionTaskByIdMutation(workflow?.id);
374-377: Simplify revision reasons extraction.The nested property access with optional chaining could be simplified using the nullish coalescing operator.
- const revisionReasons = - workflow?.workflowDefinition?.contextSchema?.schema?.properties?.documents?.items?.properties?.decision?.properties?.revisionReason?.anyOf?.find( - ({ enum: enum_ }) => !!enum_, - )?.enum ?? []; + const revisionReasons = workflow?.workflowDefinition?.contextSchema?.schema?.properties?.documents + ?.items?.properties?.decision?.properties?.revisionReason?.anyOf + ?.find(({ enum: enum_ }) => !!enum_) + ?.enum ?? [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx(2 hunks)packages/workflow-core/src/lib/plugins/external-plugin/individuals-sanctions-v2-plugin/individuals-sanctions-v2-plugin.ts(3 hunks)packages/workflow-core/src/lib/workflow-runner.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/workflow-core/src/lib/plugins/external-plugin/individuals-sanctions-v2-plugin/individuals-sanctions-v2-plugin.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: Analyze (javascript)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
- GitHub Check: spell_check
🔇 Additional comments (4)
packages/workflow-core/src/lib/workflow-runner.ts (3)
69-69: LGTM! Good choice of assertion library.The addition of
invariantfrom 'outvariant' is appropriate for runtime assertions, being both lightweight and tree-shakeable.
306-306: LGTM! Improved type safety.The return type modification correctly removes 'actionPluginName' from the return type as it's only used internally to find the action plugin.
321-325: LGTM! Good defensive programming.The invariant check ensures early failure with a descriptive error message when an action plugin is not found, preventing potential runtime errors.
apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx (1)
60-63: LGTM! Well-organized imports.The new imports for director-related functionality are properly structured and follow the project's conventions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/workflow-core/src/lib/plugins/external-plugin/shared/get-payload-properties-value.ts (1)
Line range hint
11-14: Update Documentation to Include New FunctionalityThe JSDoc comment should be updated to document the new '*' special case and its implications.
/** * Get the value of the properties in the payload depending on the type of the property i.e. 'literal' or 'path' + * @param properties - The properties object containing type information and values + * @param properties[key].__type - The type of the property + * @param properties[key].value - The value or path. Use '*' to get the entire context (use with caution) * @param context + * @returns A new object with resolved property values + * @example + * // Get specific property + * getPayloadPropertiesValue({ + * properties: { name: { __type: 'path', value: 'user.name' } }, + * context: { user: { name: 'John' } } + * }) // => { name: 'John' } + * + * // Get entire context + * getPayloadPropertiesValue({ + * properties: { data: { __type: 'path', value: '*' } }, + * context: { user: { name: 'John' } } + * }) // => { data: { user: { name: 'John' } } } */
📜 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 (21)
apps/backoffice-v2/CHANGELOG.md(1 hunks)apps/backoffice-v2/package.json(2 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/workflow-core/CHANGELOG.md(1 hunks)packages/workflow-core/package.json(2 hunks)packages/workflow-core/src/lib/plugins/external-plugin/shared/get-payload-properties-value.ts(1 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)websites/docs/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (16)
- websites/docs/package.json
- packages/common/package.json
- packages/common/CHANGELOG.md
- packages/workflow-core/CHANGELOG.md
- sdks/workflow-node-sdk/package.json
- sdks/workflow-browser-sdk/CHANGELOG.md
- sdks/web-ui-sdk/CHANGELOG.md
- sdks/workflow-node-sdk/CHANGELOG.md
- sdks/workflow-browser-sdk/package.json
- services/workflows-service/package.json
- apps/backoffice-v2/package.json
- apps/kyb-app/CHANGELOG.md
- examples/headless-example/CHANGELOG.md
- examples/headless-example/package.json
- apps/backoffice-v2/CHANGELOG.md
- services/workflows-service/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/prisma/data-migrations
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze (javascript)
- GitHub Check: lint
- GitHub Check: build (ubuntu-latest)
- GitHub Check: spell_check
🔇 Additional comments (3)
sdks/web-ui-sdk/package.json (2)
24-24: LGTM: Version bump follows semantic versioning.The increment from 1.5.69 to 1.5.70 is appropriate for a patch release.
99-99: Verify compatibility with the updated common package.The @ballerine/common dependency has been updated to version 0.9.69.
Let's verify the compatibility and changelog of the updated package:
apps/kyb-app/package.json (1)
4-4: Verify dependency updates and their implications.The version updates look correct and follow semantic versioning:
- Package version:
0.3.114→0.3.115- Dependencies:
@ballerine/common:^0.9.68→^0.9.69@ballerine/workflow-browser-sdk:0.6.87→0.6.88Let's verify the dependency updates and their implications:
Also applies to: 20-21
✅ Verification successful
Version updates are consistent and backward compatible ✅
The dependency updates in kyb-app are properly synchronized:
- All direct dependencies are on their latest versions
- Version increments are patch-level updates, indicating backward compatibility
- Caret versioning (^) ensures compatibility with future patch and minor updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for dependency version consistency and potential breaking changes # Check workflow-browser-sdk versions echo "workflow-browser-sdk versions:" fd package.json | xargs jq -r 'select(.dependencies?."@ballerine/workflow-browser-sdk"?) | "\(.name): \(.dependencies."@ballerine/workflow-browser-sdk")"' # Check if there are any breaking changes in the recent commits echo -e "\nRecent commits in common package:" git log --oneline -n 5 -- packages/common/ echo -e "\nRecent commits in workflow-browser-sdk:" git log --oneline -n 5 -- sdks/workflow-browser-sdk/Length of output: 501
Script:
#!/bin/bash # Check common package versions and recent changes echo "common package versions:" fd package.json | xargs jq -r 'select(.dependencies?."@ballerine/common"?) | "\(.name): \(.dependencies."@ballerine/common")"' echo -e "\nRecent changes mentioning version bumps:" rg -i "bump.*(common|workflow.*sdk)" -g "!package-lock.json" -A 2 echo -e "\nChanges in workflow-browser-sdk package.json:" rg "version" sdks/workflow-browser-sdk/package.json echo -e "\nChanges in common package.json:" rg "version" packages/common/package.jsonLength of output: 1035
| if ('__type' in property && property.value === '*') { | ||
| acc[key] = context; | ||
|
|
||
| return acc; | ||
| } |
There was a problem hiding this comment.
Security and Performance Concerns with Context Exposure
The new condition that assigns the entire context when value === '*' raises several concerns:
- Security: Exposing the entire context object could leak sensitive information
- Performance: Large context objects could impact memory usage
- Type Safety: No validation is performed on the context structure
Consider these alternatives:
- Whitelist specific context properties that can be exposed
- Add runtime type validation
- Document the security implications
Example implementation with whitelisting:
- if ('__type' in property && property.value === '*') {
- acc[key] = context;
-
- return acc;
- }
+ if ('__type' in property && property.value === '*') {
+ // Whitelist safe properties from context
+ const safeProps = ['id', 'name', 'status'];
+ acc[key] = Object.fromEntries(
+ Object.entries(context).filter(([key]) => safeProps.includes(key))
+ );
+
+ return acc;
+ }Committable suggestion skipped: line range outside the PR's diff.
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "@ballerine/workflow-core", | |||
| "author": "Ballerine <[email protected]>", | |||
| "version": "0.6.87", | |||
| "version": "0.6.88", | |||
There was a problem hiding this comment.
💡 Codebase verification
Update @ballerine/common dependency version in affected packages
The following packages are using an older version (^0.9.68) of @ballerine/common and should be updated to 0.9.69:
- packages/workflows-dashboard
- packages/blocks
- packages/ui
🔗 Analysis chain
Verify version consistency across the monorepo.
The version updates look correct and follow semantic versioning:
- Package version:
0.6.87→0.6.88 @ballerine/commondependency:0.9.68→0.9.69
Let's verify version consistency across the monorepo:
Also applies to: 34-34
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for version consistency across the monorepo
# Find all package.json files and extract package versions
echo "Package versions:"
fd package.json | xargs jq -r 'select(.name? | contains("@ballerine")) | "\(.name): \(.version)"'
# Check @ballerine/common dependency versions
echo -e "\n@ballerine/common dependency versions:"
fd package.json | xargs jq -r 'select(.dependencies?."@ballerine/common"?) | "\(.name): \(.dependencies."@ballerine/common")"'
Length of output: 1391
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/workflow-core/src/lib/workflow-runner.test.unit.ts(2 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/prisma/data-migrations
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (windows-latest)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (javascript)
- GitHub Check: lint
🔇 Additional comments (1)
packages/workflow-core/src/lib/workflow-runner.test.unit.ts (1)
357-357: Verify the implementation of action plugin association.The addition of
actionPluginNamesuggests a new feature where common plugins can be associated with specific action plugins. Please ensure that this relationship is properly documented and tested.Run the following script to verify the implementation:
✅ Verification successful
Action plugin association is properly implemented ✓
The feature is well-integrated with:
- Type definitions in common plugin types
- Runtime validation in workflow runner
- Error handling for missing plugins
- Usage in production workflow definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for documentation and implementation details of the action plugin association feature # Search for documentation about action plugin association echo "Searching for documentation..." rg -l "actionPluginName" --type md # Search for implementation details echo "Searching for implementation..." rg "actionPluginName" --type ts -g "!*.test.ts"Length of output: 1962
| const result = workflow.initiateCommonPlugins(commonPluginSchemas, [ | ||
| { | ||
| name: 'actionPluginName1', | ||
| displayName: 'Action Plugin Name 1', | ||
| pluginKind: 'api', | ||
| stateNames: [], | ||
| successAction: 'SUCCESS', | ||
| errorAction: 'ERROR', | ||
| persistResponseDestination: 'pluginsOutput.actionPluginName1', | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Empty stateNames array will prevent plugin execution
The empty stateNames array is problematic as the workflow engine filters plugins based on stateNames.includes(currentState). This plugin won't be triggered in any state, making it effectively non-functional.
- In
packages/workflow-core/src/lib/workflow-runner.ts, plugins are explicitly filtered by their state names:
commonPlugins = commonPlugins?.filter(plugin =>
plugin.stateNames.includes(this.#__currentState)
);Even though this is a test file, it should model realistic scenarios to provide meaningful test coverage.
🔗 Analysis chain
Review empty stateNames array and verify persistence path format.
A few concerns about the action plugin configuration:
- The
stateNamesarray is empty. This might prevent the action plugin from being triggered in any state. - The
persistResponseDestinationfollows a specific format. Ensure this format is documented and consistently used across the codebase.
Run the following script to verify the persistence path format:
Consider adding at least one state name to ensure the action plugin can be triggered:
- stateNames: [],
+ stateNames: ['state1', 'state2'],🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in persistResponseDestination format
# Search for other persistResponseDestination usages
echo "Searching for persistResponseDestination patterns..."
rg "persistResponseDestination.*pluginsOutput\." --type ts
# Search for documentation about the format
echo "Searching for documentation about persistence path format..."
rg "pluginsOutput" --type md
Length of output: 1702
Script:
#!/bin/bash
# Check how stateNames are used and if empty arrays are valid
echo "Searching for stateNames usage in workflow implementation..."
ast-grep --pattern 'stateNames: $_'
echo -e "\nSearching for stateNames handling in workflow core..."
rg "stateNames" -B 2 -A 2 packages/workflow-core/src/lib/workflow-runner.ts
echo -e "\nSearching for empty stateNames arrays..."
rg "stateNames:\s*\[\s*\]" --type ts
Length of output: 6587
Script:
#!/bin/bash
# Check the test context around the empty stateNames
echo "Checking test context..."
rg -B 10 -A 10 "stateNames: \[\]" packages/workflow-core/src/lib/workflow-runner.test.unit.ts
Length of output: 813
Summary by CodeRabbit
New Features
DirectorBlockcomponent for managing director information.useEndUserByIdQueryfor fetching end-user details.endUsersQueryKeysfor managing query keys related to end users.Improvements
Bug Fixes
Refactoring