-
Notifications
You must be signed in to change notification settings - Fork 199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/implement ocr button #2731
Changes from 13 commits
42bc8eb
29fa3e4
456522c
6408f95
d205e97
783bf7e
67b6ce2
9df1bfc
4c4f013
a5a8a1a
96cae03
ca3d26e
8096d8f
3d651de
2ed1587
53f7d3d
8244298
299cbe6
571b9c6
f8a4508
9e89e6a
41c6d15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import { ctw } from '@/common/utils/ctw/ctw'; | ||
import { ComponentProps, FunctionComponent } from 'react'; | ||
import { Loader2, ScanTextIcon } from 'lucide-react'; | ||
|
||
export interface IImageOCR extends ComponentProps<'button'> { | ||
onOcrPressed?: () => void; | ||
isOcrDisabled: boolean; | ||
isLoadingOCR?: boolean; | ||
} | ||
|
||
export const ImageOCR: FunctionComponent<IImageOCR> = ({ | ||
isOcrDisabled, | ||
onOcrPressed, | ||
className, | ||
isLoadingOCR, | ||
...props | ||
}) => ( | ||
<button | ||
{...props} | ||
type="button" | ||
className={ctw( | ||
'disabled: btn btn-circle btn-ghost btn-sm bg-base-300/70 text-[0.688rem] focus:outline-primary disabled:bg-base-300/70', | ||
isLoadingOCR, | ||
className, | ||
)} | ||
onClick={() => onOcrPressed?.()} | ||
disabled={isOcrDisabled || isLoadingOCR} | ||
> | ||
{isLoadingOCR ? ( | ||
<Loader2 className="animate-spin stroke-foreground" /> | ||
) : ( | ||
<ScanTextIcon className={'p-0.5'} /> | ||
)} | ||
</button> | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import { useMutation, useQueryClient } from '@tanstack/react-query'; | ||
import { fetchWorkflowDocumentOCRResult } from '@/domains/workflows/fetchers'; | ||
import { toast } from 'sonner'; | ||
import { t } from 'i18next'; | ||
import { workflowsQueryKeys } from '@/domains/workflows/query-keys'; | ||
import { useFilterId } from '@/common/hooks/useFilterId/useFilterId'; | ||
|
||
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({ | ||
workflowRuntimeId: workflowId, | ||
documentId, | ||
}); | ||
}, | ||
onSuccess: (data, variables) => { | ||
void queryClient.invalidateQueries(workflowsQueryKeys._def); | ||
toast.success(t('toast:document_ocr.success')); | ||
}, | ||
onError: (error, variables) => { | ||
console.error('OCR error:', error, 'for document:', variables.documentId); | ||
void queryClient.invalidateQueries(workflowsQueryKeys._def); | ||
toast.error(t('toast:document_ocr.error')); | ||
}, | ||
}); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -298,3 +298,21 @@ export const createWorkflowRequest = async ({ | |
|
||
return handleZodError(error, workflow); | ||
}; | ||
|
||
export const fetchWorkflowDocumentOCRResult = async ({ | ||
workflowRuntimeId, | ||
documentId, | ||
}: { | ||
workflowRuntimeId: string; | ||
documentId: string; | ||
}) => { | ||
const [workflow, error] = await apiClient({ | ||
method: Method.PATCH, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PATCH? |
||
url: `${getOriginUrl( | ||
env.VITE_API_URL, | ||
)}/api/v1/internal/workflows/${workflowRuntimeId}/documents/${documentId}/run-ocr`, | ||
schema: z.any(), | ||
}); | ||
|
||
return handleZodError(error, workflow); | ||
}; |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,7 +1,7 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { MotionButton } from '@/common/components/molecules/MotionButton/MotionButton'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { checkIsBusiness } from '@/common/utils/check-is-business/check-is-business'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { ctw } from '@/common/utils/ctw/ctw'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { CommonWorkflowStates, StateTag, valueOrNA } from '@ballerine/common'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { CommonWorkflowStates, isObject, StateTag, valueOrNA } from '@ballerine/common'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useApproveTaskByIdMutation } from '@/domains/entities/hooks/mutations/useApproveTaskByIdMutation/useApproveTaskByIdMutation'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useRejectTaskByIdMutation } from '@/domains/entities/hooks/mutations/useRejectTaskByIdMutation/useRejectTaskByIdMutation'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useRemoveDecisionTaskByIdMutation } from '@/domains/entities/hooks/mutations/useRemoveDecisionTaskByIdMutation/useRemoveDecisionTaskByIdMutation'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -29,6 +29,7 @@ import { X } from 'lucide-react'; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
import * as React from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { FunctionComponent, useCallback, useMemo } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { toTitleCase } from 'string-ts'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useDocumentOcr } from '@/domains/entities/hooks/mutations/useDocumentOcr/useDocumentOcr'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export const useDocumentBlocks = ({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
workflow, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -79,6 +80,14 @@ export const useDocumentBlocks = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { mutate: mutateApproveTaskById, isLoading: isLoadingApproveTaskById } = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
useApproveTaskByIdMutation(workflow?.id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
mutate: mutateOCRDocument, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
isLoading: isLoadingOCRDocument, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
data: ocrResult, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} = useDocumentOcr({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
workflowId: workflow?.id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+83
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent Variable Naming: Use CamelCase for The variables Apply this diff to rename the variables: const {
- mutate: mutateOCRDocument,
- isLoading: isLoadingOCRDocument,
+ mutate: mutateOcrDocument,
+ isLoading: isLoadingOcrDocument,
data: ocrResult,
} = useDocumentOcr({
workflowId: workflow?.id,
}); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { isLoading: isLoadingRejectTaskById } = useRejectTaskByIdMutation(workflow?.id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { comment, onClearComment, onCommentChange } = useCommentInputLogic(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -358,6 +367,19 @@ export const useDocumentBlocks = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.cellAt(0, 0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const documentEntries = Object.entries( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...additionalProperties, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...propertiesSchema?.properties, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} ?? {}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
).map(([title, formattedValue]) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (isObject(formattedValue)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
formattedValue.value ||= ocrResult?.parsedData?.[title]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In React its more important not to make mutations. Just pass the value to the right instead of formattedValue. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return [title, formattedValue]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid Mutating The current implementation directly mutates 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 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const detailsCell = createBlocksTyped() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.addBlock() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.addCell({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -370,12 +392,7 @@ export const useDocumentBlocks = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
value: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
title: `${category} - ${docType}`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
data: Object.entries( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...additionalProperties, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...propertiesSchema?.properties, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} ?? {}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
)?.map( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
data: documentEntries?.map( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
([ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
title, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -441,6 +458,7 @@ export const useDocumentBlocks = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
workflowId: workflow?.id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
isSaveDisabled: isLoadingOCRDocument, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
documents: workflow?.context?.documents, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.addCell(decisionCell) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -455,6 +473,9 @@ export const useDocumentBlocks = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
type: 'multiDocuments', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
value: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
isLoading: storageFilesQueryResult?.some(({ isLoading }) => isLoading), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
onOcrPressed: () => mutateOCRDocument({ documentId: id }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent Function Name: Rename The function Apply this diff to update the function name: onOcrPressed: () => mutateOCRDocument({ documentId: id }),
+onOcrPressed: () => mutateOcrDocument({ documentId: id }),
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
isDocumentEditable: caseState.writeEnabled, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
isLoadingOCR: isLoadingOCRDocument, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+482
to
+484
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent Variable Naming: Align The function 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
Suggested change
Add Error Handling for OCR Mutation Currently, 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.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
data: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
documents?.[docIndex]?.pages?.map( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
({ type, fileName, metadata, ballerineFileId }, pageIndex) => ({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import { ImageViewer } from '@/common/components/organisms/ImageViewer/ImageView | |
import { ctw } from '@/common/utils/ctw/ctw'; | ||
import { isPdf } from '@/common/utils/is-pdf/is-pdf'; | ||
import { useDocumentsToolbarLogic } from '@/pages/Entity/components/Case/hooks/useDocumentsToolbarLogic/useDocumentsToolbarLogic'; | ||
import { ImageOCR } from '@/common/components/molecules/ImageOCR/ImageOCR'; | ||
|
||
export const DocumentsToolbar: FunctionComponent<{ | ||
image: { id: string; imageUrl: string; fileType: string; fileName: string }; | ||
|
@@ -13,14 +14,20 @@ export const DocumentsToolbar: FunctionComponent<{ | |
onRotateDocument: () => void; | ||
onOpenDocumentInNewTab: (id: string) => void; | ||
shouldDownload: boolean; | ||
onOcrPressed?: () => void; | ||
isOCREnabled: boolean; | ||
isLoadingOCR: boolean; | ||
Comment on lines
+17
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider making The |
||
fileToDownloadBase64: string; | ||
}> = ({ | ||
image, | ||
isLoading, | ||
hideOpenExternalButton, | ||
onRotateDocument, | ||
onOpenDocumentInNewTab, | ||
onOcrPressed, | ||
shouldDownload, | ||
isLoadingOCR, | ||
isOCREnabled, | ||
fileToDownloadBase64, | ||
}) => { | ||
const { onOpenInNewTabClick } = useDocumentsToolbarLogic({ | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason the condition is not the same as the button below? The isLoading and image id. |
||
<ImageOCR | ||
isOcrDisabled={!isOCREnabled} | ||
onOcrPressed={onOcrPressed} | ||
isLoadingOCR={isLoadingOCR} | ||
/> | ||
)} | ||
{!hideOpenExternalButton && !isLoading && image?.id && ( | ||
<button | ||
type={`button`} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ import { ImageViewer } from '@/common/components/organisms/ImageViewer/ImageView | |
import { ctw } from '@/common/utils/ctw/ctw'; | ||
import { keyFactory } from '@/common/utils/key-factory/key-factory'; | ||
import { DocumentsToolbar } from '@/pages/Entity/components/Case/Case.Documents.Toolbar'; | ||
import { useDocuments } from './hooks/useDocuments/useDocuments'; | ||
import { useDocumentsLogic } from './hooks/useDocuments/useDocumentsLogic'; | ||
import { IDocumentsProps } from './interfaces'; | ||
|
||
/** | ||
|
@@ -24,19 +24,20 @@ import { IDocumentsProps } from './interfaces'; | |
*/ | ||
export const Documents: FunctionComponent<IDocumentsProps> = ({ | ||
documents, | ||
onOcrPressed, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure 'onOcrPressed' is defined in 'IDocumentsProps' You've added |
||
isLoading, | ||
isDocumentEditable, | ||
isLoadingOCR, | ||
hideOpenExternalButton, | ||
}) => { | ||
const { | ||
crop, | ||
onCrop, | ||
onCancelCrop, | ||
isCropping, | ||
onOcr, | ||
selectedImageRef, | ||
initialImage, | ||
skeletons, | ||
isLoadingOCR, | ||
selectedImage, | ||
onSelectImage, | ||
documentRotation, | ||
|
@@ -45,8 +46,9 @@ export const Documents: FunctionComponent<IDocumentsProps> = ({ | |
onTransformed, | ||
isRotatedOrTransformed, | ||
shouldDownload, | ||
isOCREnabled, | ||
fileToDownloadBase64, | ||
} = useDocuments(documents); | ||
} = useDocumentsLogic(documents); | ||
|
||
return ( | ||
<ImageViewer selectedImage={selectedImage} onSelectImage={onSelectImage}> | ||
|
@@ -88,8 +90,10 @@ export const Documents: FunctionComponent<IDocumentsProps> = ({ | |
onOpenDocumentInNewTab={onOpenDocumentInNewTab} | ||
// isRotatedOrTransformed={isRotatedOrTransformed} | ||
shouldDownload={shouldDownload} | ||
isOCREnabled={!!isDocumentEditable && isOCREnabled} | ||
onOcrPressed={onOcrPressed} | ||
isLoadingOCR={!!isLoadingOCR} | ||
// isCropping={isCropping} | ||
// isLoadingOCR={isLoadingOCR} | ||
// onCancelCrop={onCancelCrop} | ||
fileToDownloadBase64={fileToDownloadBase64} | ||
/> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Inconsistent Default Value for
isDocumentOcrEnabled
The property
isDocumentOcrEnabled
is defined with a default value offalse
infetchers.ts
. However, inuseDocumentsLogic.tsx
, it is used with a fallback totrue
:This inconsistency can lead to
shouldOCR
beingtrue
even when it should default tofalse
, potentially causing unintended behavior.Recommendations:
true
inuseDocumentsLogic.tsx
to align with the schema default.🔗 Analysis chain
LGTM: New OCR feature flag added correctly
The new
isDocumentOcrEnabled
property has been added appropriately to thefeatures
object within theCustomerSchema
. It's correctly defined as an optional boolean with a default value offalse
, which aligns with best practices for feature flags.A few points to consider:
false
ensures backwards compatibility with existing data.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:
Length of output: 78
Script:
Length of output: 364
Script:
Length of output: 394