Feature/implement ocr button #2731
Conversation
|
WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 22
🧹 Outside diff range and nitpick comments (16)
apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/interfaces.ts (2)
4-4: LGTM! Consider adding a JSDoc comment for clarity.The addition of the
onOcrPressedproperty is well-implemented and aligns with the PR objectives. It provides a clear mechanism for handling OCR button presses.Consider adding a JSDoc comment to provide more context about the
onOcrPressedfunction. This can help other developers understand its purpose and usage. Here's a suggested implementation:+ /** + * Callback function triggered when the OCR button is pressed. + * This function should handle the initiation of the OCR process. + */ onOcrPressed: () => void;
Verify OCR functionality implementation and consider adding error handling states.
While the
onOcrPressedcallback is correctly implemented and the loading state is managed, it's important to ensure that error handling is also addressed to handle potential OCR failures gracefully.
- Action Items:
- Confirm that the
Case.Documentscomponent handles errors during the OCR process.- Consider updating the
IMultiDocumentsPropsinterface to include an error state (e.g.,error: string | null) to manage OCR-related errors.🔗 Analysis chain
Line range hint
1-11: Verify component implementation and consider additional states.While the interface change looks good, it's important to ensure that the corresponding component correctly implements this new OCR functionality.
Please verify the following:
- The component using this interface correctly implements the OCR button and handles the
onOcrPressedcallback.- Consider if additional states (e.g., OCR processing state) need to be added to the interface to handle loading or error states during the OCR process.
Run the following script to find the component implementation:
Review the component implementation to ensure it correctly uses the new
onOcrPressedproperty and consider if additional states are needed in the interface.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find the component implementation using IMultiDocumentsProps # Test: Search for the component using IMultiDocumentsProps rg --type typescript -A 10 "IMultiDocumentsProps" apps/backoffice-v2/src/lib/blocks/components/MultiDocumentsLength of output: 144
Script:
#!/bin/bash # Description: Find the component implementation using IMultiDocumentsProps # Test: Search for the component using IMultiDocumentsProps with the correct file type rg --type ts -A 10 "IMultiDocumentsProps" apps/backoffice-v2/src/lib/blocks/components/MultiDocumentsLength of output: 2708
apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/MultiDocuments.tsx (1)
10-14: LGTM! The OCR functionality has been successfully integrated.The addition of the
onOcrPressedprop to theCase.Documentscomponent aligns well with the PR objectives of implementing OCR functionality. The use of optional chaining (?.) is a good practice to handle cases where the prop might not be provided.Consider the following suggestions to improve code quality and maintainability:
- Add prop type definitions for
onOcrPressedin theIMultiDocumentsPropsinterface:interface IMultiDocumentsProps { value: { data: Array<{ imageUrl: string }>; isLoading: boolean; onOcrPressed?: () => void; // Add this line }; }
- Add a brief comment explaining the purpose of the
onOcrPressedprop:<Case.Documents documents={documents} isLoading={value?.isLoading} // Callback function to handle OCR button press onOcrPressed={value?.onOcrPressed} />These changes will enhance type safety and improve code readability for future maintainers.
apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/utils/check-can-ocr/check-can-ocr.ts (2)
6-18: LGTM: Well-structured function signature with clear typing.The function signature is well-defined with appropriate typing for each parameter. The use of an object parameter with destructuring enhances readability and flexibility.
Consider adding a JSDoc comment to describe the function's purpose and parameters. This would further improve code documentation and IDE support. For example:
/** * Checks if OCR can be performed based on the current workflow state and other conditions. * @param {Object} params - The parameters for checking OCR availability. * @param {ReturnType<typeof useCaseState>} params.caseState - The current state of the case. * @param {boolean} params.noAction - Indicates if no action is possible. * @param {TWorkflowById} params.workflow - The current workflow. * @param {DefaultContextSchema['documents'][number]['decision']} params.decision - The current decision. * @param {boolean} params.isLoadingApprove - Indicates if an approval is currently loading. * @returns {boolean} Whether OCR can be performed. */
19-29: LGTM: Logic is sound and well-implemented.The function's logic is well-structured and covers the necessary conditions for determining if OCR can proceed. Good use of the helper function
checkCanMakeDecisionfor better code organization.To improve readability, consider extracting the conditions in the return statement into named variables. This would make the logic more self-documenting. For example:
const isOcrAllowed = !isLoadingApprove && canMakeDecision; const isWorkflowEligible = isStateManualReview || hasApproveEvent; return isOcrAllowed && isWorkflowEligible;This refactoring makes the conditions more explicit and easier to understand at a glance.
apps/backoffice-v2/src/pages/Entity/components/Case/interfaces.ts (1)
51-51: LGTM! Consider adding error handling.The addition of the
onOcrPressedmethod to theIDocumentsPropsinterface aligns well with the PR objective of implementing an OCR button. The method signature is clear and follows TypeScript best practices.Consider updating the method signature to include error handling:
onOcrPressed: (documentId: string) => Promise<void>;This change would allow for asynchronous OCR processing and provide a way to handle potential errors in the OCR process.
apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.Toolbar.tsx (2)
17-18: LGTM: New props for OCR functionality.The new props
onOcrPressedandshouldOCRare well-named and typed appropriately for their intended use.Consider adding JSDoc comments to these props to provide more context about their usage. For example:
/** * Callback function triggered when OCR action is initiated. */ onOcrPressed?: () => void; /** * Flag to determine if OCR functionality should be enabled. */ shouldOCR: boolean;
39-43: LGTM: Conditional rendering of ImageOCR component.The conditional rendering of the
ImageOCRcomponent is implemented correctly, using theshouldOCRprop and checking for the existence ofimage. The component is properly configured with the required props.Consider extracting the inline styles to a separate CSS class or a styled component for better maintainability. For example:
<div className="ocr-wrapper"> <ImageOCR isOcrDisabled={!shouldOCR} onOcrPressed={onOcrPressed} /> </div>Then define the
ocr-wrapperclass in your CSS file:.ocr-wrapper { margin-bottom: 2.5rem; display: flex; height: 100%; flex-direction: column; align-items: center; gap: 12.5rem; }This separation of concerns can make the code more maintainable and easier to read.
apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsx (1)
Line range hint
1-89: Summary of changes and areas for attentionThe changes in this file align with the PR objectives and the AI summary. The renaming of the hook to
useDocumentsLogicand the simplification of OCR logic are positive changes. However, there are a few areas that require attention:
- Verify the usage and necessity of the newly added
useCustomerQuery.- Ensure that all references to this hook have been updated across the codebase due to the name change.
- Consider making the
shouldOCRproperty more dynamic to accommodate different use cases.- Confirm that all previous OCR-related logic has been properly removed or relocated.
Please address these points to ensure the changes are fully integrated and optimized.
apps/backoffice-v2/public/locales/en/toast.json (1)
84-87: LGTM! Consider enhancing the error message.The new entries for document OCR are well-structured and align with the PR objectives. They provide clear feedback for both successful and failed OCR operations.
Consider enhancing the error message to be more specific and include a placeholder for additional error details. This could help users understand what went wrong and assist in debugging. For example:
"document_ocr": { "success": "OCR performed successfully.", - "error": "Failed to perform OCR on the document." + "error": "Failed to perform OCR on the document. Error: {{errorMessage}}" },This change would make the error message consistent with other error messages in the file that include
{{errorMessage}}placeholders.apps/backoffice-v2/src/domains/workflows/fetchers.ts (1)
302-318: Function implementation looks good, with some suggestions for improvement.The new
fetchWorkflowDocumentOCRResultfunction is well-implemented and consistent with the existing code style. However, I have a couple of suggestions to enhance its clarity and type safety:
Consider renaming the function to better reflect its action. The current name suggests it's fetching a result, but it's actually triggering an OCR run. A more accurate name could be
triggerWorkflowDocumentOCRorrunWorkflowDocumentOCR.Instead of using
z.any()for schema validation, consider defining a more specific schema for the expected response. This will provide better type safety and documentation of the expected response structure.Here's a suggested refactor incorporating these changes:
export const triggerWorkflowDocumentOCR = async ({ workflowDefinitionId, documentId, }: { workflowDefinitionId: string; documentId: string; }) => { const [workflow, error] = await apiClient({ method: Method.PATCH, url: `${getOriginUrl( env.VITE_API_URL, )}/api/v1/internal/workflows/${workflowDefinitionId}/documents/${documentId}/run-ocr`, schema: z.object({ // Define the expected response structure here // For example: success: z.boolean(), message: z.string(), result: z.any(), // Replace with a more specific schema if possible }), }); return handleZodError(error, workflow); };This refactor improves the function name clarity and adds a basic schema for better type safety. Adjust the schema definition based on the actual expected response structure from the API.
services/workflows-service/src/workflow/workflow.controller.internal.ts (1)
341-357: LGTM with suggestions for improvementThe implementation of the
runDocumentOcrmethod looks good overall. It follows the established patterns in the controller and uses appropriate decorators for API documentation and access control. However, there are a few areas where it could be improved:
Error handling: Consider adding explicit error handling to catch and properly handle potential exceptions from the service call.
Input validation: It might be beneficial to add input validation for the
paramsobject to ensure all required fields are present and valid before processing.Return type specification: The method could benefit from a more specific return type annotation to clearly indicate what kind of data is being returned from the OCR operation.
Here's a suggested refactoring that addresses these points:
@common.Patch(':id/documents/:documentId/run-ocr') @swagger.ApiOkResponse({ type: YourOCRResultType }) // Replace with actual type @swagger.ApiNotFoundResponse({ type: errors.NotFoundException }) @swagger.ApiForbiddenResponse({ type: errors.ForbiddenException }) @swagger.ApiBadRequestResponse({ type: errors.BadRequestException }) @UseGuards(WorkflowAssigneeGuard) async runDocumentOcr( @common.Param() params: DocumentUpdateParamsInput, @CurrentProject() currentProjectId: TProjectId, ): Promise<YourOCRResultType> { // Replace with actual return type if (!params.id || !params.documentId) { throw new errors.BadRequestException('Missing required parameters'); } try { const ocrResult = await this.service.runOCROnDocument({ workflowRuntimeId: params.id, documentId: params.documentId, projectId: currentProjectId, }); return ocrResult; } catch (error) { if (isRecordNotFoundError(error)) { throw new errors.NotFoundException(`No resource was found for ${JSON.stringify(params)}`); } // Log the error here throw new errors.InternalServerErrorException('An error occurred while processing the OCR request'); } }This refactored version includes input validation, error handling, and assumes a specific return type (
YourOCRResultType) which should be replaced with the actual type returned by your OCR operation.apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.tsx (1)
48-48: Align variable naming for consistencyThe variable
shouldOCRuses uppercase letters for "OCR", whileonOcrPresseduses "Ocr" in camel case. For consistency with other variables likeisLoadingandonOcrPressed, consider renamingshouldOCRtoshouldOcr.Apply the following changes:
At line 48:
- shouldOCR, + shouldOcr,At line 92:
- shouldOCR={shouldOCR} + shouldOcr={shouldOcr}Also applies to: 92-92
services/workflows-service/src/storage/storage.service.ts (1)
143-149: Add error handling in 'fileToBase64' methodThe method
fileToBase64lacks error handling for scenarios where the file might not exist or cannot be read. This could lead to unhandled exceptions.Apply this diff to add try-catch error handling:
fileToBase64(filepath: string): string { - const fileBuffer = readFileSync(filepath); - - const base64String = fileBuffer.toString('base64'); - - return base64String; + try { + const fileBuffer = readFileSync(filepath); + return fileBuffer.toString('base64'); + } catch (error) { + this.logger.error('Error reading file:', error); + throw new Error('Failed to convert file to Base64.'); + } }apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx (1)
84-86: Naming Convention: Use camelCase for Variable NamesThe variables
mutateOCRDocumentandisLoadingOCRDocumentuse uppercase letters in the middle of the identifier. For consistency with camelCase naming conventions in JavaScript/TypeScript, consider renaming them tomutateOcrDocumentandisLoadingOcrDocument.Apply this diff to adjust the variable names:
const { - mutate: mutateOCRDocument, - isLoading: isLoadingOCRDocument, + mutate: mutateOcrDocument, + isLoading: isLoadingOcrDocument,services/workflows-service/src/workflow/workflow.service.ts (1)
2615-2616: Evaluate transaction timeout settingThe transaction timeout is set to 180,000 milliseconds (3 minutes). If the OCR process takes longer, this could cause the transaction to fail.
Consider handling long-running OCR operations outside of a database transaction or adjusting the timeout if necessary.
📜 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 (20)
- apps/backoffice-v2/package.json (1 hunks)
- apps/backoffice-v2/public/locales/en/toast.json (1 hunks)
- apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsx (1 hunks)
- apps/backoffice-v2/src/domains/entities/hooks/mutations/useDocumentOcr/useDocumentOcr.ts (1 hunks)
- apps/backoffice-v2/src/domains/workflows/fetchers.ts (1 hunks)
- apps/backoffice-v2/src/lib/blocks/components/CallToActionLegacy/hooks/useCallToActionLegacyLogic/useCallToActionLegacyLogic.tsx (0 hunks)
- apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/MultiDocuments.tsx (1 hunks)
- apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/interfaces.ts (1 hunks)
- apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx (6 hunks)
- apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/utils/check-can-ocr/check-can-ocr.ts (1 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.Toolbar.tsx (3 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.tsx (4 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsx (2 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/interfaces.ts (1 hunks)
- services/workflows-service/prisma/data-migrations (1 hunks)
- services/workflows-service/src/common/utils/unified-api-client/unified-api-client.ts (3 hunks)
- services/workflows-service/src/storage/storage.controller.internal.ts (2 hunks)
- services/workflows-service/src/storage/storage.service.ts (2 hunks)
- services/workflows-service/src/workflow/workflow.controller.internal.ts (1 hunks)
- services/workflows-service/src/workflow/workflow.service.ts (5 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- apps/backoffice-v2/src/lib/blocks/components/CallToActionLegacy/hooks/useCallToActionLegacyLogic/useCallToActionLegacyLogic.tsx
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
🔇 Additional comments not posted (22)
apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsx (4)
1-3: LGTM: Imports are appropriate and well-organized.The imports are relevant to the component's functionality and follow a logical order. No unused imports are observed.
1-30: Overall, the ImageOCR component is well-implemented and aligns with PR objectives.The new
ImageOCRcomponent successfully implements the OCR button functionality as described in the PR objectives. The code is well-structured, follows React best practices, and uses appropriate styling.A few minor optimizations were suggested to improve code quality:
- Removing unnecessary fragment and template literals.
- Addressing the unused
documentIdprop.These changes will enhance the code without altering the component's functionality. Great job on implementing this feature!
11-30: 🛠️ Refactor suggestionComponent implementation can be optimized.
The
ImageOCRcomponent is well-implemented, but there are a few minor optimizations that can be made:
- The empty fragment (
<>...</>) is unnecessary as there's only one child element.- The
type="button"attribute uses template literals unnecessarily.- The
documentIdprop is destructured but not used in the component.Consider applying the following changes:
-export const ImageOCR: FunctionComponent<IImageOCR> = ({ +export const ImageOCR: FunctionComponent<Omit<IImageOCR, 'documentId'>> = ({ isOcrDisabled, onOcrPressed, - documentId, className, ...props -}) => ( - <> - <button - type={`button`} +}) => ( + <button + type="button" className={ctw( `btn btn-circle btn-ghost btn-sm bg-base-300/70 text-[0.688rem] focus:outline-primary`, )} onClick={() => onOcrPressed?.()} disabled={isOcrDisabled} > <ScanTextIcon /> - </button> - </> + </button> );These changes will:
- Remove the unnecessary fragment.
- Simplify the button type attribute.
- Remove the unused
documentIdprop from the component's props type.To ensure
documentIdis not used elsewhere, run:#!/bin/bash # Description: Check usage of documentId in ImageOCR component and related files # Test: Search for documentId usage in the component and any files that import it rg --type typescript 'documentId' apps/backoffice-v2/src/common/components/molecules/ImageOCR/ rg --type typescript 'from.*ImageOCR' apps/backoffice-v2/src/ | xargs rg --type typescript 'documentId'
5-9: Interface definition looks good, butdocumentIdis unused.The
IImageOCRinterface is well-defined and includes all necessary properties for the component. However, thedocumentIdproperty is not used in the current implementation of theImageOCRcomponent.If
documentIdis intended for future use, consider adding a TODO comment. If it's not needed, you may want to remove it. Here's a script to check its usage:✅ Verification successful
Interface definition looks good, but
documentIdis unused in theImageOCRcomponent.The
IImageOCRinterface includes all necessary properties for the component. However, thedocumentIdproperty is not utilized in the current implementation of theImageOCRcomponent.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of documentId in ImageOCR component # Test: Search for documentId usage in the component rg --type typescript 'documentId' apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsxLength of output: 303
Script:
#!/bin/bash # Description: Use ast-grep to find usage of documentId in ImageOCR.tsx ast-grep --lang typescript --pattern $'Identifier documentId' apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsxLength of output: 579
Script:
#!/bin/bash # Description: Check usage of documentId in ImageOCR.tsx using the correct TypeScript type identifier # Test: Search for documentId usage in the component rg --type ts 'documentId' apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsxLength of output: 134
apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/utils/check-can-ocr/check-can-ocr.ts (2)
1-4: LGTM: Imports are well-organized and relevant.The imports are logically organized and all seem to be utilized within the function. Good job on keeping the imports clean and relevant.
1-29: Overall: Well-implemented utility function for OCR availability check.This new utility function
checkCanOcris well-aligned with the PR objectives of implementing OCR-related functionality. The code is well-structured, properly typed, and logically sound. The minor suggestions for improvement (adding JSDoc and refactoring for readability) will further enhance the code quality and maintainability.Great job on implementing this crucial piece of the OCR feature!
apps/backoffice-v2/src/domains/entities/hooks/mutations/useDocumentOcr/useDocumentOcr.ts (3)
1-6: LGTM: Imports are appropriate and well-organized.The imports cover all necessary dependencies for the hook's functionality, including React Query for state management, toast notifications, internationalization, and internal utilities. The import structure follows a consistent pattern, which is good for maintainability.
20-23: LGTM: Proper success handlingThe success handling is well-implemented:
- It invalidates the relevant queries to ensure fresh data.
- It displays a localized success message using the toast notification system.
This approach ensures that the UI is updated with the latest data and provides good user feedback.
1-30: Overall assessment: Good implementation with minor improvements neededThe
useDocumentOcrhook is well-structured and makes good use of React Query for managing the OCR mutation. Here's a summary of the main points:
- The hook name has a typo that needs to be fixed (
useDocumentOrc->useDocumentOcr).- There's an inconsistency in parameter naming (
workflowIdvsworkflowDefinitionId) that should be addressed.- Success handling is well-implemented with proper query invalidation and user feedback.
- Error handling could be refined by removing unnecessary query invalidation and potentially including more context in the error message.
Addressing these points will improve the overall quality and consistency of the implementation. Great job on implementing this OCR functionality!
apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.Toolbar.tsx (3)
8-8: LGTM: New import for ImageOCR component.The import for the
ImageOCRcomponent is correctly placed and follows the existing import structure. This maintains code organization and readability.
26-26: LGTM: Destructured onOcrPressed prop.The
onOcrPressedprop is correctly destructured from the component props, maintaining consistency with its earlier definition.
Line range hint
1-85: Overall assessment: Changes look good and align with PR objectives.The modifications to the
DocumentsToolbarcomponent successfully integrate OCR functionality as intended. The new props, imports, and conditional rendering are implemented correctly and follow React best practices. The changes are consistent with the PR objectives of implementing an OCR button and related functionality.To ensure that the
ImageOCRcomponent is implemented correctly and that there are no unintended side effects, please run the following verification script:This script will help verify the correct implementation and usage of the
ImageOCRcomponent across the codebase.✅ Verification successful
Verification Successful:
ImageOCRcomponent is correctly implemented and integrated.The
ImageOCRcomponent exists at./apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsxand is implemented with the correct props. Its integration withinCase.Documents.Toolbar.tsxaligns with the PR objectives, ensuring the OCR functionality is properly incorporated without introducing issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of ImageOCR component and its usage # Test 1: Check if ImageOCR component exists echo "Checking for ImageOCR component..." ast-grep --lang typescript --pattern $'export const ImageOCR: FunctionComponent<{$$$}> = ({$$$}) => {$$$}' # Test 2: Verify ImageOCR props echo "Verifying ImageOCR props..." ast-grep --lang typescript --pattern $'export const ImageOCR: FunctionComponent<{ isOcrDisabled: boolean; onOcrPressed: () => void; $$$ }>' # Test 3: Check for any other usages of ImageOCR in the codebase echo "Checking for other usages of ImageOCR..." rg --type typescript "import.*ImageOCR" -A 10Length of output: 572
Script:
#!/bin/bash # Description: Re-verify the implementation of ImageOCR component and its usage without specifying file type # Test 1: Check if ImageOCR component exists echo "Checking for ImageOCR component..." ast-grep --lang typescript --pattern $'export const ImageOCR: FunctionComponent<{$$$}> = ({$$$}) => {$$$}' # Test 2: Verify ImageOCR props echo "Verifying ImageOCR props..." ast-grep --lang typescript --pattern $'export const ImageOCR: FunctionComponent<{ isOcrDisabled: boolean; onOcrPressed: () => void; $$$ }>' # Test 3: Check for any other usages of ImageOCR in the codebase without specifying file type echo "Checking for other usages of ImageOCR..." rg "import.*ImageOCR" -A 10Length of output: 1812
Script:
#!/bin/bash # Description: Verify the existence and implementation of the ImageOCR component # Test 1: Check if the ImageOCR component file exists echo "Checking if ImageOCR component file exists..." fd "ImageOCR.tsx" ./apps/backoffice-v2/src/common/components/molecules/ImageOCR/ --hidden # Test 2: If the file exists, display its contents if [ -f "./apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsx" ]; then echo "Displaying contents of ImageOCR.tsx..." cat ./apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsx else echo "ImageOCR.tsx does not exist." fiLength of output: 1235
apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsx (3)
73-73: 🛠️ Refactor suggestionConsider making
shouldOCRmore dynamic and verify removal of previous OCR logic.The addition of
shouldOCR: truesimplifies the OCR logic as mentioned in the AI summary. However, setting it to a statictruevalue might not be flexible for all use cases.Consider making
shouldOCRmore dynamic, perhaps based on document type or user permissions:shouldOCR: determineIfShouldOCR(selectedImage, userPermissions),Also, please verify that the previous OCR-related logic has been properly removed or relocated. Run the following script to check for any remaining OCR-related code:
#!/bin/bash # Description: Check for remaining OCR-related code # Test: Search for OCR-related terms rg --type typescript -i 'ocr|optical character recognition' apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsx
14-14: Approve name change and verify usage across the codebase.The renaming of
useDocumentstouseDocumentsLogicis more descriptive and aligns with the file name. This is a good change for clarity.However, ensure that all references to this hook have been updated across the codebase. Run the following script to check for any remaining references to
useDocuments:✅ Verification successful
Name change verified and approved.
All references to
useDocumentshave been successfully updated touseDocumentsLogicacross the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for remaining references to useDocuments # Test: Search for useDocuments usage rg --type typescript 'useDocuments'Length of output: 189
Script:
#!/bin/bash # Description: Check for remaining references to useDocuments in TypeScript files # Search for 'useDocuments' in .ts and .tsx files rg 'useDocuments' --glob '*.ts' --glob '*.tsx'Length of output: 978
12-12: Verify the usage ofuseCustomerQuery.The addition of
useCustomerQuerysuggests that customer data is now being used in this hook. Please ensure that this query is necessary and properly utilized within the component.To confirm the usage of
useCustomerQuery, run the following script:✅ Verification successful
useCustomerQueryis properly utilized within the component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of useCustomerQuery in the component # Test: Search for useCustomerQuery usage rg --type typescript -A 10 'useCustomerQuery' apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsxLength of output: 173
Script:
#!/bin/bash # Description: Verify the usage of useCustomerQuery in the component with the correct file type # Test: Search for useCustomerQuery usage in .tsx files rg --type tsx -A 10 'useCustomerQuery' apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsxLength of output: 159
Script:
#!/bin/bash # Description: Verify the usage of useCustomerQuery in .tsx files using glob pattern # Test: Search for useCustomerQuery usage in .tsx files rg --glob '*.tsx' -A 10 'useCustomerQuery' apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsxLength of output: 830
apps/backoffice-v2/package.json (1)
100-100: Significant version update for lucide-react package.The
lucide-reactpackage has been updated from version^0.239.0to^0.445.0, which is a substantial version jump. While this update may introduce new features or improvements that could benefit the OCR button implementation, it's important to consider the following:
- Verify the changelog for any breaking changes that might affect the existing codebase.
- Check for new features or improvements in the latest version that could be leveraged for the OCR functionality.
- Ensure thorough testing of the application, particularly areas that use Lucide icons, to confirm compatibility with this new version.
To assist in verifying the impact of this change, you can run the following script:
services/workflows-service/src/storage/storage.controller.internal.ts (1)
11-11: ImportmanageFileByProvideradded appropriatelyThe addition of the import statement for
manageFileByProviderensures that the file storage management is correctly utilized within theuploadFilemethod.apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.tsx (3)
10-10: Import statement updated correctlyThe import of
useDocumentsLogicreflects the updated document management logic and replaces the previous hook appropriately.
48-50: Destructured values from 'useDocumentsLogic' are correctly updatedIncluding
shouldOCRandfileToDownloadBase64fromuseDocumentsLogicaligns with the new OCR functionality and appears to be implemented correctly.
92-93: Passing OCR props to 'DocumentsToolbar'Passing
shouldOCRandonOcrPressedtoDocumentsToolbarenables OCR actions within the toolbar. Ensure that theDocumentsToolbarcomponent is updated to accept these new props to avoid any undefined prop issues.services/workflows-service/src/storage/storage.service.ts (2)
129-137: Validate 'persistedFile.uri' before downloadingIn the method
__downloadFileFromRemote, ensure thatpersistedFile.uriis validated to prevent downloading from malicious or invalid URLs.[security]
Consider adding additional validation or sanitization steps for the URI.
24-28: Dependency Injection: Verify the addition of new dependenciesThe constructor now includes
HttpServiceandAppLoggerService. Ensure that these services are properly provided in the module to avoid runtime errors.Run the following script to check if
HttpServiceandAppLoggerServiceare imported and provided:#!/bin/bash # Description: Verify that HttpService and AppLoggerService are properly injected. # Test: Search for 'HttpModule' import. Expect: 'HttpModule' is imported in the module. rg "import\s+\{.*HttpModule.*\}\s+from.*" services/workflows-service/src/**/*.module.ts # Test: Search for 'AppLoggerService' provider. Expect: 'AppLoggerService' is provided in the module. rg "providers\s*:\s*\[.*AppLoggerService.*\]" services/workflows-service/src/**/*.module.ts
| export const useDocumentOrc = ({ workflowId }: { workflowId: string }) => { | ||
| const filterId = useFilterId(); | ||
| const workflowById = workflowsQueryKeys.byId({ workflowId, filterId }); | ||
| const queryClient = useQueryClient(); |
There was a problem hiding this comment.
Fix typo in hook name: useDocumentOrc should be useDocumentOcr
There's a typo in the hook name. OCR stands for Optical Character Recognition, so the correct spelling should be "Ocr" instead of "Orc".
Please apply the following change:
-export const useDocumentOrc = ({ workflowId }: { workflowId: string }) => {
+export const useDocumentOcr = ({ workflowId }: { workflowId: string }) => {This will ensure consistency with the file name and prevent potential confusion.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const useDocumentOrc = ({ workflowId }: { workflowId: string }) => { | |
| const filterId = useFilterId(); | |
| const workflowById = workflowsQueryKeys.byId({ workflowId, filterId }); | |
| const queryClient = useQueryClient(); | |
| export const useDocumentOcr = ({ workflowId }: { workflowId: string }) => { | |
| const filterId = useFilterId(); | |
| const workflowById = workflowsQueryKeys.byId({ workflowId, filterId }); | |
| const queryClient = useQueryClient(); |
| return useMutation({ | ||
| mutationFn: ({ documentId }: { documentId: string }) => { | ||
| return fetchWorkflowDocumentOCRResult({ | ||
| workflowDefinitionId: workflowId, | ||
| documentId, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Ensure consistency in parameter naming
The mutation function uses workflowDefinitionId, but the hook receives workflowId as a parameter. This inconsistency could lead to confusion.
Consider one of the following options:
- Rename the parameter to match the usage:
-export const useDocumentOcr = ({ workflowId }: { workflowId: string }) => {
+export const useDocumentOcr = ({ workflowDefinitionId }: { workflowDefinitionId: string }) => {
// ... (update all occurrences of workflowId to workflowDefinitionId)- Or, if
workflowIdis the correct term, update the fetch function call:
return fetchWorkflowDocumentOCRResult({
- workflowDefinitionId: workflowId,
+ workflowId,
documentId,
});Choose the option that best aligns with your project's terminology and the actual purpose of this ID.
Committable suggestion was skipped due to low confidence.
| onError: (_error, _variables) => { | ||
| console.error(_error); | ||
| void queryClient.invalidateQueries(workflowsQueryKeys._def); | ||
| toast.error(t('toast:document_ocr.error')); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine error handling approach
While the current error handling provides user feedback and logs the error, there are a couple of points to consider:
- Invalidating queries on error might not be necessary and could potentially cause issues by triggering unnecessary refetches.
- The error details are not utilized in the user-facing error message.
Consider the following improvements:
- onError: (_error, _variables) => {
- console.error(_error);
- void queryClient.invalidateQueries(workflowsQueryKeys._def);
- toast.error(t('toast:document_ocr.error'));
+ onError: (error, variables) => {
+ console.error('OCR error:', error, 'for document:', variables.documentId);
+ toast.error(t('toast:document_ocr.error', { documentId: variables.documentId }));
},This change:
- Removes the query invalidation on error, as it's likely unnecessary.
- Improves error logging by including context (document ID).
- Potentially enriches the error message with the document ID (update your i18n file accordingly).
Also, consider adding more specific error handling if the fetchWorkflowDocumentOCRResult function provides structured error responses.
Committable suggestion was skipped due to low confidence.
| async runDocumentOcr({ | ||
| images, | ||
| supportedCountries, | ||
| overrideSchemas, | ||
| }: { | ||
| images: TOcrImages; | ||
| supportedCountries: string[]; | ||
| overrideSchemas: { | ||
| overrideSchemas: Array<{ | ||
| countryCode: string; | ||
| documentType: string; | ||
| documentCategory: string; | ||
| schema: TSchema; | ||
| }>; | ||
| }; | ||
| }) { | ||
| return await this.axiosInstance.post('/v1/document/smart-ocr', { | ||
| images, | ||
| supportedCountries, | ||
| overrideSchemas, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add error handling in runDocumentOcr method for consistency
The runDocumentOcr method lacks error handling. To maintain consistency with the existing postBatchBusinessReport method and improve reliability, consider adding a try/catch block to handle potential errors and log them appropriately.
Apply this diff:
async runDocumentOcr({
images,
supportedCountries,
overrideSchemas,
}: {
images: TOcrImages;
supportedCountries: string[];
overrideSchemas: {
overrideSchemas: Array<{
countryCode: string;
documentType: string;
documentCategory: string;
schema: TSchema;
}>;
};
}) {
+ try {
return await this.axiosInstance.post('/v1/document/smart-ocr', {
images,
supportedCountries,
overrideSchemas,
});
+ } catch (error) {
+ this.logger.error('Error in runDocumentOcr', error);
+ throw error;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async runDocumentOcr({ | |
| images, | |
| supportedCountries, | |
| overrideSchemas, | |
| }: { | |
| images: TOcrImages; | |
| supportedCountries: string[]; | |
| overrideSchemas: { | |
| overrideSchemas: Array<{ | |
| countryCode: string; | |
| documentType: string; | |
| documentCategory: string; | |
| schema: TSchema; | |
| }>; | |
| }; | |
| }) { | |
| return await this.axiosInstance.post('/v1/document/smart-ocr', { | |
| images, | |
| supportedCountries, | |
| overrideSchemas, | |
| }); | |
| } | |
| async runDocumentOcr({ | |
| images, | |
| supportedCountries, | |
| overrideSchemas, | |
| }: { | |
| images: TOcrImages; | |
| supportedCountries: string[]; | |
| overrideSchemas: { | |
| overrideSchemas: Array<{ | |
| countryCode: string; | |
| documentType: string; | |
| documentCategory: string; | |
| schema: TSchema; | |
| }>; | |
| }; | |
| }) { | |
| try { | |
| return await this.axiosInstance.post('/v1/document/smart-ocr', { | |
| images, | |
| supportedCountries, | |
| overrideSchemas, | |
| }); | |
| } catch (error) { | |
| this.logger.error('Error in runDocumentOcr', error); | |
| throw error; | |
| } | |
| } |
🛠️ Refactor suggestion
Simplify overrideSchemas parameter structure in runDocumentOcr
The overrideSchemas parameter includes an extra overrideSchemas property, which may be redundant. Consider simplifying the structure by passing the array directly if the API allows it. This can improve code readability and reduce unnecessary nesting.
Apply this diff:
In the method signature:
async runDocumentOcr({
images,
supportedCountries,
overrideSchemas,
}: {
images: TOcrImages;
supportedCountries: string[];
- overrideSchemas: {
- overrideSchemas: Array<{
+ overrideSchemas: Array<{
countryCode: string;
documentType: string;
documentCategory: string;
schema: TSchema;
- }>;
- };
+ }>;
}) {And in the Axios request:
return await this.axiosInstance.post('/v1/document/smart-ocr', {
images,
supportedCountries,
- overrideSchemas,
+ overrideSchemas,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async runDocumentOcr({ | |
| images, | |
| supportedCountries, | |
| overrideSchemas, | |
| }: { | |
| images: TOcrImages; | |
| supportedCountries: string[]; | |
| overrideSchemas: { | |
| overrideSchemas: Array<{ | |
| countryCode: string; | |
| documentType: string; | |
| documentCategory: string; | |
| schema: TSchema; | |
| }>; | |
| }; | |
| }) { | |
| return await this.axiosInstance.post('/v1/document/smart-ocr', { | |
| images, | |
| supportedCountries, | |
| overrideSchemas, | |
| }); | |
| } | |
| async runDocumentOcr({ | |
| images, | |
| supportedCountries, | |
| overrideSchemas, | |
| }: { | |
| images: TOcrImages; | |
| supportedCountries: string[]; | |
| overrideSchemas: Array<{ | |
| countryCode: string; | |
| documentType: string; | |
| documentCategory: string; | |
| schema: TSchema; | |
| }>; | |
| }) { | |
| return await this.axiosInstance.post('/v1/document/smart-ocr', { | |
| images, | |
| supportedCountries, | |
| overrideSchemas, | |
| }); | |
| } |
| interface TOcrImage { | ||
| runOcr({ | ||
| images, | ||
| schema, | ||
| }: { | ||
| images: Array< | ||
| | { | ||
| remote: { | ||
| imageUri: string; | ||
| mimeType: string; | ||
| }; | ||
| } | ||
| | { | ||
| base64: string; | ||
| } | ||
| >; | ||
| schema: TSchema; | ||
| }): Promise<axios.AxiosResponse<any>>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reuse 'TOcrImages' type in interface definition for consistency
To maintain consistency and reduce redundancy, consider using the previously defined TOcrImages type for the images parameter in the runOcr method within the TOcrImage interface, instead of redefining the type inline.
Apply the following diff:
interface TOcrImage {
runOcr({
images,
schema,
}: {
- images: Array<
- | {
- remote: {
- imageUri: string;
- mimeType: string;
- };
- }
- | {
- base64: string;
- }
- >;
+ images: TOcrImages;
schema: TSchema;
}): Promise<axios.AxiosResponse<any>>;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface TOcrImage { | |
| runOcr({ | |
| images, | |
| schema, | |
| }: { | |
| images: Array< | |
| | { | |
| remote: { | |
| imageUri: string; | |
| mimeType: string; | |
| }; | |
| } | |
| | { | |
| base64: string; | |
| } | |
| >; | |
| schema: TSchema; | |
| }): Promise<axios.AxiosResponse<any>>; | |
| } | |
| interface TOcrImage { | |
| runOcr({ | |
| images, | |
| schema, | |
| }: { | |
| images: TOcrImages; | |
| schema: TSchema; | |
| }): Promise<axios.AxiosResponse<any>>; | |
| } |
| const { | ||
| mutate: mutateOCRDocument, | ||
| isLoading: isLoadingOCRDocument, | ||
| data: ocrResult, | ||
| } = useDocumentOrc({ | ||
| workflowId: workflow?.id, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Inconsistent Hook Usage: Rename useDocumentOrc to useDocumentOcr
The hook is referred to as useDocumentOrc, but it should be useDocumentOcr to accurately represent the OCR functionality. This inconsistency may cause runtime errors or confusion.
Apply this diff to correct the hook usage:
const {
- mutate: mutateOCRDocument,
- isLoading: isLoadingOCRDocument,
- data: ocrResult,
-} = useDocumentOrc({
+ mutate: mutateOcrDocument,
+ isLoading: isLoadingOcrDocument,
+ data: ocrResult,
+} = useDocumentOcr({
workflowId: workflow?.id,
});Committable suggestion was skipped due to low confidence.
| const document = runtimeData?.context?.documents?.find( | ||
| (document: DefaultContextSchema['documents'][number]) => document.id === documentId, | ||
| ); | ||
|
|
||
| const documentSchema = addPropertiesSchemaToDocument(document, workflowDef.documentsSchema); |
There was a problem hiding this comment.
Add null check for document existence
In the findDocumentById method, there's no check to handle the case where the document is not found. This could lead to a TypeError when document is undefined in subsequent operations.
Apply this diff to handle the case when the document is not found:
const document = runtimeData?.context?.documents?.find(
(document: DefaultContextSchema['documents'][number]) => document.id === documentId,
);
+ if (!document) {
+ throw new NotFoundException(`Document with ID ${documentId} not found in workflow ${workflowId}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const document = runtimeData?.context?.documents?.find( | |
| (document: DefaultContextSchema['documents'][number]) => document.id === documentId, | |
| ); | |
| const documentSchema = addPropertiesSchemaToDocument(document, workflowDef.documentsSchema); | |
| const document = runtimeData?.context?.documents?.find( | |
| (document: DefaultContextSchema['documents'][number]) => document.id === documentId, | |
| ); | |
| if (!document) { | |
| throw new NotFoundException(`Document with ID ${documentId} not found in workflow ${workflowId}`); | |
| } | |
| const documentSchema = addPropertiesSchemaToDocument(document, workflowDef.documentsSchema); |
| const base64String = this.storageService.fileToBase64(filePath!); | ||
|
|
There was a problem hiding this comment.
Handle potential undefined filePath
In cases where signedUrl is not available, the code assumes filePath is defined. If filePath is undefined, calling fileToBase64 will result in a runtime error.
Add a check to ensure that filePath is defined before using it:
+ if (!filePath) {
+ throw new InternalServerErrorException('File path is undefined');
+ }
const base64String = this.storageService.fileToBase64(filePath!);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const base64String = this.storageService.fileToBase64(filePath!); | |
| if (!filePath) { | |
| throw new InternalServerErrorException('File path is undefined'); | |
| } | |
| const base64String = this.storageService.fileToBase64(filePath!); |
| id: page.ballerineFileId!, | ||
| format: 'signed-url', | ||
| projectIds: [projectId], | ||
| }); | ||
|
|
There was a problem hiding this comment.
Ensure ballerineFileId is defined
When accessing page.ballerineFileId!, the non-null assertion operator is used. If ballerineFileId is undefined or null, this will cause a runtime error.
Consider adding a check to ensure ballerineFileId is defined before using it, or handle the case where it might be missing:
const { signedUrl, mimeType, filePath } = await this.storageService.fetchFileContent({
- id: page.ballerineFileId!,
+ id: page.ballerineFileId,
format: 'signed-url',
projectIds: [projectId],
});
+ if (!page.ballerineFileId) {
+ throw new BadRequestException('Page is missing ballerineFileId');
+ }Committable suggestion was skipped due to low confidence.
| await new UnifiedApiClient().runOcr({ | ||
| images, | ||
| schema: documentWithSchema.propertiesSchema as unknown as TSchema, | ||
| }) | ||
| )?.data; | ||
| }, |
There was a problem hiding this comment.
Add error handling for OCR service call
The call to runOcr may fail or throw an exception, but there is no error handling in place. This could result in unhandled exceptions.
Consider wrapping the OCR service call in a try-catch block to handle potential errors gracefully:
+ try {
return (
await new UnifiedApiClient().runOcr({
images,
schema: documentWithSchema.propertiesSchema as unknown as TSchema,
})
)?.data;
+ } catch (error) {
+ this.logger.error('OCR service failed', { error });
+ throw new InternalServerErrorException('Failed to process OCR');
+ }Committable suggestion was skipped due to low confidence.
| documentId: string; | ||
| }) => { | ||
| const [workflow, error] = await apiClient({ | ||
| method: Method.PATCH, |
| mutate: mutateOCRDocument, | ||
| isLoading: isLoadingOCRDocument, | ||
| data: ocrResult, | ||
| } = useDocumentOrc({ |
| import { CommonWorkflowEvent, CommonWorkflowStates, DefaultContextSchema } from '@ballerine/common'; | ||
| import { checkCanMakeDecision } from '@/lib/blocks/hooks/useDocumentBlocks/utils/check-can-make-decision/check-can-make-decision'; | ||
|
|
||
| export const checkCanOcr = ({ |
There was a problem hiding this comment.
@alonp99, I agree that the function name checkCanOcr can be more descriptive. Perhaps renaming it to canPerformOcr, isOcrAllowed, or shouldEnableOcr would enhance readability and better convey its purpose.
✏️ Learnings added
Learnt from: alonp99
PR: ballerine-io/ballerine#2731
File: apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/utils/check-can-ocr/check-can-ocr.ts:6-6
Timestamp: 2024-09-25T21:48:10.975Z
Learning: When naming functions, use clear and descriptive names that accurately reflect their purpose to improve code readability.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| workflow: TWorkflowById; | ||
| decision: DefaultContextSchema['documents'][number]['decision']; | ||
| isLoadingApprove: boolean; | ||
| }) => { |
There was a problem hiding this comment.
can you explain why this function is needed?
| (document: DefaultContextSchema['documents'][number]) => document.id === documentId, | ||
| ); | ||
|
|
||
| const documentSchema = addPropertiesSchemaToDocument(document, workflowDef.documentsSchema); |
There was a problem hiding this comment.
This is not documentSchema, you can call it documentWithSchema or just document - its very confusing in the rest of the code
| throw new BadRequestException('Cannot run document OCR on document without pages'); | ||
| } | ||
|
|
||
| const documentWithSchema = addPropertiesSchemaToDocument( |
There was a problem hiding this comment.
its seems like youre adding the properties twice, its also done infindDocumentById
| const documentPagesContent = documentWithSchema.pages.map(async page => { | ||
| const { signedUrl, mimeType, filePath } = await this.storageService.fetchFileContent({ | ||
| id: page.ballerineFileId!, | ||
| format: 'signed-url', |
There was a problem hiding this comment.
Its weird your stating the format and get something else (like base64). i guess you could split it and check If it file is on s3, if so, use signed URL otherwise continue formats
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apps/backoffice-v2/src/domains/entities/hooks/mutations/useDocumentOcr/useDocumentOcr.ts (1 hunks)
- apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx (6 hunks)
- services/workflows-service/prisma/data-migrations (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/backoffice-v2/src/domains/entities/hooks/mutations/useDocumentOcr/useDocumentOcr.ts
- services/workflows-service/prisma/data-migrations
🔇 Additional comments (4)
apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx (4)
4-4: LGTM: Import statement updated correctlyThe addition of
isObjectto the import statement from '@ballerine/common' is appropriate and consistent with its usage in the file.
32-32: LGTM: New import for OCR functionality addedThe import of
useDocumentOcris correctly added and aligns with the new OCR functionality being implemented in the hook.
Line range hint
1-570: Summary of OCR Implementation ReviewThe implementation of OCR functionality in the
useDocumentBlockshook is generally well-done. Here's a summary of the key points:
- The necessary imports and hook usage for OCR have been correctly added.
- The integration of OCR results into the document processing logic is implemented.
- An OCR button functionality has been added to the UI.
However, there are a few minor issues to address:
- Consistency in naming conventions (e.g.,
mutateOCRDocumentvsmutateOcrDocument).- Potential object mutation in the
documentEntrieslogic.- An unused variable (
isLoadingOCRDocument) that should be removed if not needed.Please address these issues to improve the overall quality and consistency of the implementation.
470-470:⚠️ Potential issueFix typo in function name
The function name
mutateOCRDocumentshould bemutateOcrDocumentfor consistency with camelCase naming conventions.Apply this diff to fix the typo:
- onOcrPressed: () => mutateOCRDocument({ documentId: id }), + onOcrPressed: () => mutateOcrDocument({ documentId: id }),Likely invalid or redundant comment.
…o/ballerine into feature/implement_ocr_button
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/interfaces.ts (1)
4-5: LGTM! Consider minor adjustment for consistency.The new properties
onOcrPressedandisLoadingOCRare well-defined and align with the PR objectives. They follow TypeScript best practices and have clear, descriptive names.For consistency with the existing
isLoadingproperty, consider movingisLoadingOCRright after it. This groups related properties together, improving readability:export interface IMultiDocumentsProps { value: { isLoading: boolean; isLoadingOCR: boolean; onOcrPressed: () => void; data: Array<{ imageUrl: string; title: string; fileType: string; }>; }; }apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/MultiDocuments.tsx (1)
10-15: LGTM! Consider adding type assertions for improved type safety.The changes align well with the PR objectives by adding OCR-related props to the
Case.Documentscomponent. This implementation allows for OCR functionality and loading state management.For improved type safety, consider adding type assertions to the new props:
<Case.Documents documents={documents} isLoading={value?.isLoading} - onOcrPressed={value?.onOcrPressed} - isLoadingOCR={value?.isLoadingOCR} + onOcrPressed={value?.onOcrPressed as (() => void) | undefined} + isLoadingOCR={value?.isLoadingOCR as boolean | undefined} />This change ensures that the types of
onOcrPressedandisLoadingOCRare explicitly defined, which can help catch potential type-related issues earlier in the development process.apps/backoffice-v2/src/lib/blocks/components/EditableDetails/interfaces.ts (1)
28-28: LGTM! Consider adding a JSDoc comment for clarity.The addition of the
isSaveDisabledproperty is well-implemented. It's optional, which maintains backward compatibility, and its boolean type is appropriate for representing a disabled state.Consider adding a JSDoc comment to provide more context about when and why this property might be used. For example:
/** * Indicates whether the save functionality should be disabled. * This can be used to prevent saving when certain conditions are not met. */ isSaveDisabled?: boolean;apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsx (1)
12-33: LGTM: Well-implemented component with a minor suggestion.The
ImageOCRcomponent is well-implemented, following React best practices. The conditional rendering, styling, and prop usage are all handled effectively.However, there's a minor optimization opportunity in the
onClickhandler:Consider memoizing the
onClickhandler to avoid creating a new function on each render:- onClick={() => !isLoadingOCR && onOcrPressed?.()} + onClick={React.useCallback(() => !isLoadingOCR && onOcrPressed?.(), [isLoadingOCR, onOcrPressed])}This change would require adding
Reactto the import statement:-import { ComponentProps, FunctionComponent } from 'react'; +import React, { ComponentProps, FunctionComponent } from 'react';apps/backoffice-v2/src/lib/blocks/components/Details/Details.tsx (1)
16-16: LGTM! Consider adding type annotation forisSaveDisabled.The addition of the
isSaveDisabledprop and its usage in theEditableDetailscomponent is well-implemented. It extends the component's functionality in a clear and consistent manner.For improved type safety and code clarity, consider adding a type annotation for the
isSaveDisabledprop. You can update the component's props type or add an inline annotation:isSaveDisabled?: boolean,This will ensure that only boolean values are passed for this prop and improve the component's API documentation.
Also applies to: 42-42
apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.Toolbar.tsx (3)
17-19: LGTM: New props for OCR functionality.The new props
onOcrPressed,shouldOCR, andisLoadingOCRare well-defined and properly typed. They align with the OCR functionality being added to the component.Consider grouping related props together for better readability. You could move
shouldOCRnext toonOcrPressedlike this:onOcrPressed?: () => void; shouldOCR: boolean; isLoadingOCR: boolean;
27-30: Align prop order with type definition.The destructuring of the new props is correct, but the order doesn't match the type definition.
Consider reordering the destructured props to match the order in the type definition for consistency and improved readability:
onOcrPressed, shouldOCR, isLoadingOCR,
41-49: LGTM: New OCR functionality implementation.The implementation of the OCR functionality is well-structured and correctly uses the new
ImageOCRcomponent with appropriate props. The conditional rendering ensures the OCR button is only shown when necessary.Consider using the
ctwutility for className concatenation to maintain consistency with the rest of the file:<div className={ctw('gap-y-50 mb-10 flex h-full flex-col items-center')}>apps/backoffice-v2/src/lib/blocks/create-blocks-typed/types.ts (1)
118-118: LGTM! Consider adding a comment for clarity.The addition of the
isSaveDisabledoptional boolean property to theTDetailsCelltype is a good enhancement. It provides more granular control over the save functionality in the UI, which can be useful in various scenarios such as data validation or permission checks.Consider adding a brief comment to explain the purpose of this property, for example:
/** When true, disables the save functionality for this details cell */ isSaveDisabled?: boolean;This will help other developers understand the intended use of this property at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsx (1 hunks)
- apps/backoffice-v2/src/domains/customer/fetchers.ts (1 hunks)
- apps/backoffice-v2/src/lib/blocks/components/Details/Details.tsx (2 hunks)
- apps/backoffice-v2/src/lib/blocks/components/EditableDetails/EditableDetails.tsx (2 hunks)
- apps/backoffice-v2/src/lib/blocks/components/EditableDetails/interfaces.ts (1 hunks)
- apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/MultiDocuments.tsx (1 hunks)
- apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/interfaces.ts (1 hunks)
- apps/backoffice-v2/src/lib/blocks/create-blocks-typed/types.ts (1 hunks)
- apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx (7 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.Toolbar.tsx (3 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.tsx (4 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsx (2 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/interfaces.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.tsx
- apps/backoffice-v2/src/pages/Entity/components/Case/interfaces.ts
🔇 Additional comments (17)
apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsx (3)
1-3: LGTM: Imports are well-organized and necessary.The imports are logically organized and include all the necessary dependencies for the component's functionality.
1-33: Overall, excellent implementation with minor suggestions for improvement.The
ImageOCRcomponent is well-designed and implemented. It provides a reusable OCR button with appropriate props for controlling its state and behavior. The code is clean, follows React best practices, and uses TypeScript effectively for type safety.Consider the minor suggestions provided in the previous comments to further optimize the component. Great job on this implementation!
5-10: LGTM: Well-defined interface, but review documentId usage.The
IImageOCRinterface is well-structured and includes all necessary props for the component. However, thedocumentIdprop is defined but not used in the component implementation.Please verify if the
documentIdprop is necessary or if it's used in a parent component:apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.Toolbar.tsx (2)
8-8: LGTM: New import for OCR functionality.The import of the
ImageOCRcomponent is correctly placed and aligns with the new OCR functionality being added to theDocumentsToolbar.
Line range hint
1-92: Overall: Well-implemented OCR functionality.The changes in this file successfully implement the OCR button functionality as described in the PR objectives. The code is well-structured, and the new OCR-related components and props are integrated seamlessly into the existing
DocumentsToolbarcomponent.A few minor suggestions have been made to improve consistency and readability:
- Grouping related props in the type definition.
- Aligning the order of destructured props with the type definition.
- Using the
ctwutility for className concatenation in the new OCR section.These small adjustments will enhance the overall quality and maintainability of the code.
apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsx (5)
11-11: LGTM: Import changes align with new functionality.The addition of
useCustomerQueryand removal ofcopyToClipboardare consistent with the described changes in the PR objectives and AI summary.
13-13: LGTM: Function name change improves clarity.Renaming
useDocumentstouseDocumentsLogicbetter reflects the hook's purpose and aligns with the changes described in the AI summary.
15-15: LGTM: Customer data query added.The addition of
useCustomerQueryhook usage is consistent with the new import and likely supports the OCR-related features mentioned in the PR objectives.
Line range hint
1-85: Overall, the changes align well with the PR objectives.The refactoring of
useDocumentsLogichook successfully implements the OCR-related changes described in the PR objectives. The code is cleaner, with unnecessary OCR logic removed and new customer-related functionality added.However, please ensure to address the TODO comment regarding the default value of
shouldOCRbefore finalizing this PR.
71-71:⚠️ Potential issueAddress TODO comment and review default value for
shouldOCR.The addition of
shouldOCRaligns with the PR objectives. However, please address the following:
- Remove the default
truevalue as indicated by the TODO comment.- Ensure that the
isDocumentOcrEnabledfeature flag is properly set for customers.- Consider adding a comment explaining the purpose of this flag for future maintainers.
To verify the usage of
isDocumentOcrEnabled, run the following script:✅ Verification successful
To accurately locate usages of
isDocumentOcrEnabled, please run the following script:
TODO Comment Addressed: Remove default
trueforshouldOCR.The
isDocumentOcrEnabledfeature flag is properly defined with a default value offalseand is consistently used within the codebase. You can safely remove the defaulttruevalue as indicated by the TODO comment.
- Action Items:
- Remove
|| truefrom theshouldOCRassignment.- Verify that
isDocumentOcrEnabledis correctly configured for all customers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of isDocumentOcrEnabled in the codebase # Test: Search for isDocumentOcrEnabled usage rg --type typescript 'isDocumentOcrEnabled'Length of output: 78
Script:
# !/bin/bash # Description: Check for other occurrences of isDocumentOcrEnabled in TypeScript files rg 'isDocumentOcrEnabled' --glob '*.ts' --glob '*.tsx'Length of output: 372
apps/backoffice-v2/src/lib/blocks/components/EditableDetails/EditableDetails.tsx (2)
106-106: LGTM: New prop enhances component flexibilityThe addition of the
isSaveDisabledprop is a good enhancement. It allows for external control of the "Save" button's disabled state, improving the component's flexibility and reusability.
431-435: LGTM: Improved "Save" button accessibility and visual feedbackThe changes to the "Save" button rendering logic are well-implemented:
- The new classes provide clear visual feedback when the button is disabled.
- The
aria-disabledattribute improves accessibility by properly indicating the button's state to assistive technologies.- The conditional rendering ensures the button only appears when relevant.
These changes enhance both the user experience and accessibility of the component.
apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx (5)
4-4: LGTM: Import statement updated correctlyThe addition of
isObjectto the import statement is appropriate for the new OCR functionality.
32-32: LGTM: New OCR hook imported correctlyThe import of
useDocumentOcris correctly added and is essential for the new OCR functionality.
83-90:⚠️ Potential issueFix typo and consider removing unused variable
- The variable name
mutateOCRDocumentshould bemutateOcrDocumentfor consistency with camelCase naming conventions.- The
isLoadingOCRDocumentvariable is not used in the visible code. Consider removing it if it's not used elsewhere in the component.Apply this diff to fix the typo and remove the unused variable:
const { - mutate: mutateOCRDocument, - isLoading: isLoadingOCRDocument, + mutate: mutateOcrDocument, data: ocrResult, } = useDocumentOcr({ workflowId: workflow?.id, });Likely invalid or redundant comment.
370-381:⚠️ Potential issueAvoid mutating
formattedValuedirectlyThe current implementation directly mutates
formattedValue, which could lead to unintended side effects if these objects are used elsewhere. Consider using an immutable approach to update the value.Apply this diff to use an immutable approach:
const documentEntries = Object.entries( { ...additionalProperties, ...propertiesSchema?.properties, } ?? {}, ).map(([title, formattedValue]) => { if (isObject(formattedValue)) { - formattedValue.value ||= ocrResult?.parsedData?.[title]; + return [title, { + ...formattedValue, + value: formattedValue.value || ocrResult?.parsedData?.[title], + }]; } - return [title, formattedValue]; + return [title, formattedValue]; });This change ensures that we're not modifying the original
formattedValueobject, but instead creating a new object with the updated value.Likely invalid or redundant comment.
456-456: LGTM: Save disabled during OCR processingThe
isSaveDisabledproperty is correctly set toisLoadingOCRDocument, which prevents saving while OCR is in progress. This is a good practice to avoid inconsistent states.Consider if there are other conditions that should disable saving. You may want to combine this with existing conditions if they exist. To verify, you can run the following command to check for other usages of
isSaveDisabled:
| @@ -27,6 +27,7 @@ const CustomerSchema = z.object({ | |||
| createBusinessReportBatch: z | |||
| .object({ enabled: z.boolean().default(false), options: createBusinessReportOptions }) | |||
| .optional(), | |||
| isDocumentOcrEnabled: z.boolean().default(false).optional(), | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Default Value for isDocumentOcrEnabled
The property isDocumentOcrEnabled is defined with a default value of false in fetchers.ts. However, in useDocumentsLogic.tsx, it is used with a fallback to true:
shouldOCR: customer?.features?.isDocumentOcrEnabled || true, // TODO remove default true after reviewThis inconsistency can lead to shouldOCR being true even when it should default to false, potentially causing unintended behavior.
Recommendations:
- Remove the fallback to
trueinuseDocumentsLogic.tsxto align with the schema default. - Address the TODO comment to ensure consistency across the codebase.
🔗 Analysis chain
LGTM: New OCR feature flag added correctly
The new isDocumentOcrEnabled property has been added appropriately to the features object within the CustomerSchema. It's correctly defined as an optional boolean with a default value of false, which aligns with best practices for feature flags.
A few points to consider:
- The naming is clear and follows the existing naming conventions in the schema.
- The default value of
falseensures backwards compatibility with existing data. - The property is optional, which is appropriate for a feature flag.
To ensure this change doesn't introduce any unexpected behavior, let's verify its usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing usage of isDocumentOcrEnabled
# Expected: No results if this is a new feature
rg --type typescript 'isDocumentOcrEnabled'
Length of output: 78
Script:
#!/bin/bash
# Description: Check for any existing usage of isDocumentOcrEnabled in TypeScript files
# Expected: No results if this is a new feature
rg 'isDocumentOcrEnabled' -g '*.ts' -g '*.tsx'
Length of output: 364
Script:
#!/bin/bash
# Description: Extract surrounding lines for the usage of isDocumentOcrEnabled in useDocumentsLogic.tsx
# Expected: Contextual code to assess the implementation and TODO comment
rg 'isDocumentOcrEnabled' -A 5 -B 5 apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsx
Length of output: 394
| onOcrPressed: () => mutateOCRDocument({ documentId: id }), | ||
| isLoadingOCR: isLoadingOCRDocument, |
There was a problem hiding this comment.
Fix typo in OCR mutation function name
The OCR functionality is correctly implemented, but there's a typo in the function name.
Apply this diff to fix the typo:
- onOcrPressed: () => mutateOCRDocument({ documentId: id }),
+ onOcrPressed: () => mutateOcrDocument({ documentId: id }),
isLoadingOCR: isLoadingOCRDocument,The rest of the changes look good. The isLoadingOCR property is correctly set to isLoadingOCRDocument.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onOcrPressed: () => mutateOCRDocument({ documentId: id }), | |
| isLoadingOCR: isLoadingOCRDocument, | |
| onOcrPressed: () => mutateOcrDocument({ documentId: id }), | |
| isLoadingOCR: isLoadingOCRDocument, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
apps/backoffice-v2/src/pages/Entity/components/Case/interfaces.ts (2)
53-53: LGTM: OCR loading state addedThe addition of
isLoadingOCRproperty aligns with the PR objective and complements theonOcrPressedmethod. It's correctly defined as an optional boolean.Consider using a more specific name like
isOcrProcessingto clearly indicate that it represents the processing state of OCR, not just loading.
51-54: Summary of changes toIDocumentsPropsThe changes to the
IDocumentsPropsinterface align well with the PR objectives of implementing OCR functionality:
- The
onOcrPressedmethod provides a callback for the OCR button.- The
isLoadingOCRproperty allows for indicating the OCR processing state.However, the purpose of
isDocumentEditablein relation to OCR is not clear from the PR objectives.To ensure comprehensive documentation:
Consider updating the PR description to include the rationale for adding the
isDocumentEditableproperty and how it relates to the OCR functionality. This will help maintain clear documentation of the interface's evolution and its relationship to the OCR feature.apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.Toolbar.tsx (3)
17-19: LGTM: New props for OCR functionality.The new props
onOcrPressed,isOCREnabled, andisLoadingOCRare well-typed and align with the PR objectives. Their naming follows React conventions.Consider adding JSDoc comments to these new props to improve code documentation. For example:
/** * Callback function triggered when OCR is initiated. */ onOcrPressed?: () => void; /** * Flag indicating whether OCR functionality is enabled. */ isOCREnabled: boolean; /** * Flag indicating whether OCR processing is in progress. */ isLoadingOCR: boolean;
27-30: Consider grouping related props together.The new OCR-related props have been correctly added to the component's parameter list. However, for better readability and maintainability, consider grouping related props together.
Suggest reordering the props as follows:
}) => ({ image, isLoading, hideOpenExternalButton, onRotateDocument, onOpenDocumentInNewTab, onOcrPressed, isOCREnabled, isLoadingOCR, shouldDownload, fileToDownloadBase64, }) => {This groups the OCR-related props together, making the code more organized.
41-47: LGTM: ImageOCR component integration.The
ImageOCRcomponent is correctly integrated and conditionally rendered based on the presence of an image. The OCR-related props are properly passed to the component.For improved clarity, consider renaming the
isOcrDisabledprop in theImageOCRcomponent toisOcrEnabledand passing the value directly:<ImageOCR isOcrEnabled={isOCREnabled} onOcrPressed={onOcrPressed} isLoadingOCR={isLoadingOCR} />This change would make the prop naming consistent across components and eliminate the need for a negation operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsx (1 hunks)
- apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/MultiDocuments.tsx (1 hunks)
- apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/interfaces.ts (1 hunks)
- apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx (7 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.Toolbar.tsx (3 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.tsx (4 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsx (2 hunks)
- apps/backoffice-v2/src/pages/Entity/components/Case/interfaces.ts (1 hunks)
- services/workflows-service/prisma/data-migrations (1 hunks)
- services/workflows-service/src/workflow/workflow.service.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/backoffice-v2/src/common/components/molecules/ImageOCR/ImageOCR.tsx
- apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/MultiDocuments.tsx
- apps/backoffice-v2/src/lib/blocks/components/MultiDocuments/interfaces.ts
- apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx
- apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.tsx
- apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsx
- services/workflows-service/prisma/data-migrations
- services/workflows-service/src/workflow/workflow.service.ts
🔇 Additional comments (4)
apps/backoffice-v2/src/pages/Entity/components/Case/interfaces.ts (2)
51-51: LGTM: OCR button callback addedThe addition of
onOcrPressedmethod aligns with the PR objective of implementing an OCR button. The method signature is correct for a callback, and it follows React's naming convention for event handlers.
54-54: Please clarify the purpose ofisDocumentEditableThe addition of
isDocumentEditableproperty is not directly mentioned in the PR objectives. Could you please clarify:
- How does this property relate to the OCR functionality being implemented?
- In what scenarios would a document be editable or non-editable in the context of OCR?
- How is this property intended to be used in the UI or logic related to OCR?
This information will help ensure that the property aligns with the overall objectives of the PR and is implemented correctly.
apps/backoffice-v2/src/pages/Entity/components/Case/Case.Documents.Toolbar.tsx (2)
8-8: LGTM: New import for OCR functionality.The import for the
ImageOCRcomponent is correctly placed and follows the project's import conventions. This addition aligns with the PR objective of implementing OCR functionality.
Line range hint
1-89: Overall assessment: Implementation looks good with minor suggestions for improvement.The changes to the
DocumentsToolbarcomponent successfully integrate OCR functionality as per the PR objectives. The code is well-structured, follows React best practices, and maintains consistency with the existing codebase. The suggestions provided are minor and aimed at improving code clarity and organization.Great job on implementing this feature! Once the suggested minor improvements are addressed, this code will be ready for merging.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- services/workflows-service/src/workflow/workflow.service.ts (4 hunks)
- services/workflows-service/src/workflow/workflow.service.unit.test.ts (1 hunks)
🔇 Additional comments (1)
services/workflows-service/src/workflow/workflow.service.unit.test.ts (1)
Line range hint
1-238: Ensure comprehensive updates throughout the test fileWhile the change to the WorkflowService constructor is isolated, it's important to ensure that this modification is reflected consistently throughout the test file. Consider the following points:
Review all instances where WorkflowService is instantiated in this file and update them accordingly.
Check if there are any test cases that might need to be modified or added to cover scenarios related to the new parameter.
Verify that the mock data and setup in the
beforeEachblock are still sufficient for all test cases, given the new parameter.If there are any descriptions or documentation within the test file about the WorkflowService structure, make sure they are updated to include information about the new parameter.
To help with this review, you can run the following command to find all occurrences of WorkflowService instantiation in the file:
This will help identify any other places where the WorkflowService constructor is called and ensure they are all updated consistently.
✅ Verification successful
Correct the shell script to search for WorkflowService instantiations
The previous command failed because
typescriptis not a recognized file type inripgrep. Please use the corrected script below to search for all instances ofnew WorkflowService(in the specified test file:
Verification Successful: WorkflowService instantiations are consistent
All instances of
WorkflowServiceinstantiation in the test file have been reviewed and are consistent with the expected changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for WorkflowService instantiations rg --type typescript -A 5 'new WorkflowService\(' services/workflows-service/src/workflow/workflow.service.unit.test.tsLength of output: 156
Script:
#!/bin/bash # Search for WorkflowService instantiations in the specified test file rg 'new WorkflowService\(' services/workflows-service/src/workflow/workflow.service.unit.test.ts -A 5Length of output: 271
| @@ -207,6 +207,7 @@ describe('WorkflowService', () => { | |||
| ruleEngineService, | |||
| {} as any, | |||
| {} as any, | |||
| {} as any, | |||
There was a problem hiding this comment.
Consider improving the test setup for the new WorkflowService parameter
The addition of a new parameter to the WorkflowService constructor indicates a change in its implementation. However, there are a few concerns:
-
There's no documentation or comments explaining what this new parameter represents. Consider adding a comment to clarify its purpose.
-
Using
{} as anyis generally not a good testing practice. It bypasses TypeScript's type checking and doesn't properly mock the expected interface, which could lead to missed issues during testing. -
Instead of
{} as any, consider creating a proper mock object that represents the expected interface. This will make the test more robust and easier to maintain. -
If there are other test cases for WorkflowService in this file or others, make sure to update them to include this new parameter as well.
Here's a suggestion to improve the code:
// Define an interface for the new parameter (replace NewDependency with the actual type)
interface NewDependency {
// Add the expected properties and methods
}
// Create a mock object
const mockNewDependency: NewDependency = {
// Implement the necessary properties and methods
};
// Use the mock object in the constructor
service = new WorkflowService(
// ... other parameters ...
mockNewDependency,
);This approach will provide better type safety and make the test more representative of the actual usage.
| const document = await this.findDocumentById({ | ||
| workflowId: workflowRuntimeId, | ||
| projectId, | ||
| documentId, | ||
| transaction, | ||
| }); | ||
|
|
||
| if (!('pages' in document)) { | ||
| throw new BadRequestException('Cannot run document OCR on document without pages'); |
There was a problem hiding this comment.
Add null check after retrieving the document
In the runOCROnDocument method, there's no check to handle the case where findDocumentById returns undefined if the document is not found. Accessing properties on an undefined value will result in a runtime error.
Apply this diff to handle the case when the document is not found:
const document = await this.findDocumentById({
workflowId: workflowRuntimeId,
projectId,
documentId,
transaction,
});
+ if (!document) {
+ throw new NotFoundException(`Document with ID ${documentId} not found in workflow ${workflowRuntimeId}`);
+ }
if (!('pages' in document)) {
throw new BadRequestException('Cannot run document OCR on document without pages');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const document = await this.findDocumentById({ | |
| workflowId: workflowRuntimeId, | |
| projectId, | |
| documentId, | |
| transaction, | |
| }); | |
| if (!('pages' in document)) { | |
| throw new BadRequestException('Cannot run document OCR on document without pages'); | |
| const document = await this.findDocumentById({ | |
| workflowId: workflowRuntimeId, | |
| projectId, | |
| documentId, | |
| transaction, | |
| }); | |
| if (!document) { | |
| throw new NotFoundException(`Document with ID ${documentId} not found in workflow ${workflowRuntimeId}`); | |
| } | |
| if (!('pages' in document)) { | |
| throw new BadRequestException('Cannot run document OCR on document without pages'); |
| const documentPagesContent = document.pages.map(async page => { | ||
| const { signedUrl, mimeType, filePath } = await this.storageService.fetchFileContent({ | ||
| id: page.ballerineFileId!, | ||
| format: 'signed-url', | ||
| projectIds: [projectId], | ||
| }); | ||
|
|
||
| if (signedUrl) { | ||
| return { | ||
| remote: { | ||
| imageUri: signedUrl, | ||
| mimeType, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| const base64String = this.storageService.fileToBase64(filePath!); | ||
|
|
||
| return { base64: `data:${mimeType};base64,${base64String}` }; | ||
| }); | ||
|
|
||
| const images = (await Promise.all(documentPagesContent)) satisfies TOcrImages; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor asynchronous processing of document pages
Consider refactoring the asynchronous mapping over document.pages to improve readability and error handling. Using a for...of loop can make the code clearer and facilitate better error control.
Apply this diff to refactor the code:
- const documentPagesContent = document.pages.map(async page => {
- const { signedUrl, mimeType, filePath } = await this.storageService.fetchFileContent({
- id: page.ballerineFileId!,
- format: 'signed-url',
- projectIds: [projectId],
- });
-
- if (signedUrl) {
- return {
- remote: {
- imageUri: signedUrl,
- mimeType,
- },
- };
- }
-
- const base64String = this.storageService.fileToBase64(filePath!);
-
- return { base64: `data:${mimeType};base64,${base64String}` };
- });
-
- const images = (await Promise.all(documentPagesContent)) satisfies TOcrImages;
+ const images: TOcrImages = [];
+ for (const page of document.pages) {
+ const { signedUrl, mimeType, filePath } = await this.storageService.fetchFileContent({
+ id: page.ballerineFileId!,
+ format: 'signed-url',
+ projectIds: [projectId],
+ });
+
+ if (signedUrl) {
+ images.push({
+ remote: {
+ imageUri: signedUrl,
+ mimeType,
+ },
+ });
+ } else {
+ const base64String = this.storageService.fileToBase64(filePath!);
+ images.push({
+ base64: `data:${mimeType};base64,${base64String}`,
+ });
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const documentPagesContent = document.pages.map(async page => { | |
| const { signedUrl, mimeType, filePath } = await this.storageService.fetchFileContent({ | |
| id: page.ballerineFileId!, | |
| format: 'signed-url', | |
| projectIds: [projectId], | |
| }); | |
| if (signedUrl) { | |
| return { | |
| remote: { | |
| imageUri: signedUrl, | |
| mimeType, | |
| }, | |
| }; | |
| } | |
| const base64String = this.storageService.fileToBase64(filePath!); | |
| return { base64: `data:${mimeType};base64,${base64String}` }; | |
| }); | |
| const images = (await Promise.all(documentPagesContent)) satisfies TOcrImages; | |
| const images: TOcrImages = []; | |
| for (const page of document.pages) { | |
| const { signedUrl, mimeType, filePath } = await this.storageService.fetchFileContent({ | |
| id: page.ballerineFileId!, | |
| format: 'signed-url', | |
| projectIds: [projectId], | |
| }); | |
| if (signedUrl) { | |
| images.push({ | |
| remote: { | |
| imageUri: signedUrl, | |
| mimeType, | |
| }, | |
| }); | |
| } else { | |
| const base64String = this.storageService.fileToBase64(filePath!); | |
| images.push({ | |
| base64: `data:${mimeType};base64,${base64String}`, | |
| }); | |
| } | |
| } |
# Conflicts: # pnpm-lock.yaml # services/workflows-service/prisma/data-migrations # services/workflows-service/src/workflow/workflow.service.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
services/workflows-service/src/workflow/workflow.service.ts (2)
2549-2577: Specify the return type offindDocumentByIdConsider specifying the return type of the
findDocumentByIdmethod to enhance type safety and code readability. Explicit return types help prevent unintended type-related issues and improve maintainability.
2578-2634: Specify the return type ofrunOCROnDocumentConsider specifying the return type of the
runOCROnDocumentmethod to enhance type safety and code clarity. This practice ensures that the expected return value is clear to other developers and assists with TypeScript's static analysis.
📜 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 (3)
- apps/backoffice-v2/package.json (1 hunks)
- apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx (7 hunks)
- services/workflows-service/src/workflow/workflow.service.ts (4 hunks)
🔇 Additional comments (1)
apps/backoffice-v2/package.json (1)
101-101: Significant version update forlucide-react. Verify compatibility and test thoroughly.The
lucide-reactpackage has been updated from version^0.239.0to^0.445.0, which is a substantial version jump. This update may introduce breaking changes or new features that could affect the application.Please ensure the following:
- This update was intentional and not accidental.
- The application has been thoroughly tested with the new version.
- Check the changelog for any breaking changes or deprecations between these versions.
- Verify if any other dependencies relying on
lucide-reactneed to be updated as well.To assist with verification, you can run the following script to check for any usage of deprecated or changed APIs:
This script will help identify areas of the codebase that might be affected by the update. Review the results carefully and update the code as necessary.
| const documentEntries = Object.entries( | ||
| { | ||
| ...additionalProperties, | ||
| ...propertiesSchema?.properties, | ||
| } ?? {}, | ||
| ).map(([title, formattedValue]) => { | ||
| if (isObject(formattedValue)) { | ||
| formattedValue.value ||= ocrResult?.parsedData?.[title]; | ||
| } | ||
|
|
||
| return [title, formattedValue]; | ||
| }); |
There was a problem hiding this comment.
Avoid Mutating formattedValue Directly
The current implementation directly mutates formattedValue by setting formattedValue.value. This could lead to unintended side effects if formattedValue is used elsewhere. Instead, consider using an immutable approach to create a new object with the updated value.
Apply this diff to prevent direct mutation:
).map(([title, formattedValue]) => {
if (isObject(formattedValue)) {
- formattedValue.value ||= ocrResult?.parsedData?.[title];
+ formattedValue = {
+ ...formattedValue,
+ value: formattedValue.value || ocrResult?.parsedData?.[title],
+ };
}
return [title, formattedValue];
});This change ensures that you are not modifying the original formattedValue object but creating a new one with the updated value.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const documentEntries = Object.entries( | |
| { | |
| ...additionalProperties, | |
| ...propertiesSchema?.properties, | |
| } ?? {}, | |
| ).map(([title, formattedValue]) => { | |
| if (isObject(formattedValue)) { | |
| formattedValue.value ||= ocrResult?.parsedData?.[title]; | |
| } | |
| return [title, formattedValue]; | |
| }); | |
| const documentEntries = Object.entries( | |
| { | |
| ...additionalProperties, | |
| ...propertiesSchema?.properties, | |
| } ?? {}, | |
| ).map(([title, formattedValue]) => { | |
| if (isObject(formattedValue)) { | |
| formattedValue = { | |
| ...formattedValue, | |
| value: formattedValue.value || ocrResult?.parsedData?.[title], | |
| }; | |
| } | |
| return [title, formattedValue]; | |
| }); |
| const { | ||
| mutate: mutateOCRDocument, | ||
| isLoading: isLoadingOCRDocument, | ||
| data: ocrResult, | ||
| } = useDocumentOcr({ | ||
| workflowId: workflow?.id, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Inconsistent Variable Naming: Use CamelCase for mutateOCRDocument and isLoadingOCRDocument
The variables mutateOCRDocument and isLoadingOCRDocument use an uppercase OCR in the middle of their names, which is inconsistent with camelCase conventions used elsewhere in the codebase (e.g., useDocumentOcr). For consistency and readability, consider renaming these variables to mutateOcrDocument and isLoadingOcrDocument.
Apply this diff to rename the variables:
const {
- mutate: mutateOCRDocument,
- isLoading: isLoadingOCRDocument,
+ mutate: mutateOcrDocument,
+ isLoading: isLoadingOcrDocument,
data: ocrResult,
} = useDocumentOcr({
workflowId: workflow?.id,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { | |
| mutate: mutateOCRDocument, | |
| isLoading: isLoadingOCRDocument, | |
| data: ocrResult, | |
| } = useDocumentOcr({ | |
| workflowId: workflow?.id, | |
| }); | |
| const { | |
| mutate: mutateOcrDocument, | |
| isLoading: isLoadingOcrDocument, | |
| data: ocrResult, | |
| } = useDocumentOcr({ | |
| workflowId: workflow?.id, | |
| }); |
| onOcrPressed: () => mutateOCRDocument({ documentId: id }), | ||
| isDocumentEditable: caseState.writeEnabled, | ||
| isLoadingOCR: isLoadingOCRDocument, |
There was a problem hiding this comment.
Inconsistent Variable Naming: Align mutateOCRDocument with CamelCase Convention
The function mutateOCRDocument is called here, but its naming is inconsistent with camelCase conventions and other variables like onOcrPressed. Similarly, isLoadingOCRDocument should be isLoadingOcrDocument. Renaming these variables enhances readability and maintains consistency across the codebase.
Apply this diff to update the variable names:
onOcrPressed: () => mutateOCRDocument({ documentId: id }),
+onOcrPressed: () => mutateOcrDocument({ documentId: id }),
isDocumentEditable: caseState.writeEnabled,
-isLoadingOCR: isLoadingOCRDocument,
+isLoadingOCR: isLoadingOcrDocument,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onOcrPressed: () => mutateOCRDocument({ documentId: id }), | |
| isDocumentEditable: caseState.writeEnabled, | |
| isLoadingOCR: isLoadingOCRDocument, | |
| onOcrPressed: () => mutateOcrDocument({ documentId: id }), | |
| isDocumentEditable: caseState.writeEnabled, | |
| isLoadingOCR: isLoadingOcrDocument, |
Add Error Handling for OCR Mutation
Currently, onOcrPressed triggers the OCR mutation without handling potential errors. To improve user experience and robustness, consider adding error handling to provide feedback in case the mutation fails.
Example implementation:
onOcrPressed: () => {
- mutateOCRDocument({ documentId: id }),
+ mutateOcrDocument(
+ { documentId: id },
+ {
+ onError: (error) => {
+ // Handle error, e.g., display notification
+ console.error('OCR mutation failed:', error);
+ },
+ },
+ );
},This addition ensures that errors during the mutation are caught and can be appropriately addressed.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- services/workflows-service/src/storage/storage.service.ts (2 hunks)
- services/workflows-service/src/workflow/workflow.service.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/workflow/workflow.service.ts
| private __getAbsoluteFilePAth(filePath: string) { | ||
| if (!path.isAbsolute(filePath)) return filePath; | ||
|
|
||
| const rootDir = path.parse(os.homedir()).root; | ||
|
|
||
| return path.join(rootDir, filePath); | ||
| } | ||
|
|
||
| private async __downloadFileFromRemote(persistedFile: File) { | ||
| const localeFilePath = `${os.tmpdir()}/${persistedFile.id}`; | ||
| const downloadedFilePath = await new HttpFileService({ | ||
| client: this.httpService, | ||
| logger: this.logger, | ||
| }).download(persistedFile.uri, localeFilePath); | ||
|
|
||
| return downloadedFilePath; | ||
| } | ||
|
|
||
| _isUri(persistedFile: File) { | ||
| return z.string().url().safeParse(persistedFile.uri).success; | ||
| } | ||
|
|
||
| fileToBase64(filepath: string): string { | ||
| const fileBuffer = readFileSync(filepath); | ||
|
|
||
| const base64String = fileBuffer.toString('base64'); | ||
|
|
||
| return base64String; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent method naming conventions
The methods in the StorageService class use inconsistent naming conventions for private methods:
- Double underscores:
__getAbsoluteFilePath,__downloadFileFromRemote - Single underscore:
_isUri - No underscore:
fileToBase64
Consider standardizing the naming convention for private methods. A common practice is to use a single underscore prefix for private or protected methods in TypeScript. This will enhance code consistency and maintainability.
| return { filePath: filePath, mimeType }; | ||
| } | ||
|
|
||
| private __getAbsoluteFilePAth(filePath: string) { |
There was a problem hiding this comment.
Fix the typo in the method name '__getAbsoluteFilePAth'
The method name __getAbsoluteFilePAth contains a typographical error with an uppercase 'A' in "PAth". It should be corrected to __getAbsoluteFilePath to maintain proper naming conventions and improve readability.
Apply this diff to correct the method name:
-private __getAbsoluteFilePAth(filePath: string) {
+private __getAbsoluteFilePath(filePath: string) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private __getAbsoluteFilePAth(filePath: string) { | |
| private __getAbsoluteFilePath(filePath: string) { |
| } ?? {}, | ||
| ).map(([title, formattedValue]) => { | ||
| if (isObject(formattedValue)) { | ||
| formattedValue.value ||= ocrResult?.parsedData?.[title]; |
There was a problem hiding this comment.
In React its more important not to make mutations. Just pass the value to the right instead of formattedValue.
| @@ -31,6 +38,13 @@ export const DocumentsToolbar: FunctionComponent<{ | |||
|
|
|||
| return ( | |||
| <div className={`absolute z-50 flex space-x-2 bottom-right-6`}> | |||
| {image && ( | |||
There was a problem hiding this comment.
Any reason the condition is not the same as the button below? The isLoading and image id.
| interface TOcrImage { | ||
| runOcr({ | ||
| images, | ||
| schema, | ||
| }: { | ||
| images: TOcrImages; | ||
| schema: TSchema; | ||
| }): Promise<axios.AxiosResponse<any>>; | ||
| } |
There was a problem hiding this comment.
What is the goal of creating and using this interface over implementing runOcr directly?
Summary by CodeRabbit
Release Notes
New Features
ImageOCRcomponent for triggering OCR actions.Case.Documentscomponent.DocumentsToolbarto include OCR functionality.Bug Fixes
Documentation
Refactor