-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Create base search record action #10135
base: main
Are you sure you want to change the base?
Conversation
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.
PR Summary
This PR introduces a basic 'Find Records' workflow action that enables record searching with simple configuration options. Here's a focused analysis of the key changes:
- Added new
WorkflowEditActionFormFindRecords
component with object selection and record limit fields in/workflow-steps/workflow-actions/components/
- Integrated
FIND_RECORDS
action type inWorkflowStepDetail.tsx
with proper type definitions and server-side support - Added 'Search Records' entry to
RECORD_ACTIONS
with 'IconSearch' icon and appropriate type mapping - Implemented Storybook tests covering both editable and read-only states
- Added server-side support in
workflow-version-step.workspace-service.ts
with default limit of 1 record
The implementation provides a solid foundation for basic record searching, though currently lacks filtering and sorting capabilities as noted in the PR description.
7 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
|
||
return null; |
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.
logic: Default return null could silently fail for unhandled action types. Consider throwing an error instead.
return null; | |
return assertUnreachable( | |
stepDefinition.definition, | |
`Expected the action to have a handler; ${JSON.stringify(stepDefinition.definition)}`, | |
); |
id: getWorkflowNodeIdMock(), | ||
name: 'Search Records', | ||
type: 'FIND_RECORDS', | ||
valid: false, |
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.
logic: Default action is set to invalid (valid: false) but there's no validation logic shown to determine when it becomes valid
<FormNumberFieldInput | ||
label="Limit" | ||
defaultValue={formData.limit} | ||
placeholder="Enter limit" | ||
onPersist={() => {}} | ||
readonly | ||
/> |
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.
logic: FormNumberFieldInput is marked as readonly but has an empty onPersist handler. Either make it editable with proper persistence or remove the onPersist prop if it's meant to be readonly.
<FormNumberFieldInput | |
label="Limit" | |
defaultValue={formData.limit} | |
placeholder="Enter limit" | |
onPersist={() => {}} | |
readonly | |
/> | |
<FormNumberFieldInput | |
label="Limit" | |
defaultValue={formData.limit} | |
placeholder="Enter limit" | |
readonly | |
/> |
if (!isDefined(selectedObjectMetadataItem)) { | ||
throw new Error('Should have found the metadata item'); | ||
} |
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.
logic: Error thrown when metadata item not found could crash the UI. Consider graceful fallback or error boundary.
const newFormData: FindRecordsFormData = { | ||
objectName, | ||
limit: 1, | ||
}; |
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.
style: Hard-coding limit to 1 when changing object type may be unexpected for users. Consider preserving existing limit value.
const newFormData: FindRecordsFormData = { | |
objectName, | |
limit: 1, | |
}; | |
const newFormData: FindRecordsFormData = { | |
objectName, | |
limit: formData.limit ?? 1, | |
}; |
case WorkflowActionType.FIND_RECORDS: { | ||
const activeObjectMetadataItem = | ||
await this.objectMetadataRepository.findOne({ | ||
where: { workspaceId, isActive: true, isSystem: false }, | ||
}); | ||
|
||
return { | ||
id: newStepId, | ||
name: 'Search Records', | ||
type: WorkflowActionType.FIND_RECORDS, | ||
valid: false, | ||
settings: { | ||
...BASE_STEP_DEFINITION, | ||
input: { | ||
objectName: activeObjectMetadataItem?.nameSingular || '', | ||
limit: 1, | ||
}, | ||
}, | ||
}; | ||
} |
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.
logic: No error handling if activeObjectMetadataItem is undefined. Consider throwing a WorkflowVersionStepException if no active object metadata is found.
case WorkflowActionType.FIND_RECORDS: { | |
const activeObjectMetadataItem = | |
await this.objectMetadataRepository.findOne({ | |
where: { workspaceId, isActive: true, isSystem: false }, | |
}); | |
return { | |
id: newStepId, | |
name: 'Search Records', | |
type: WorkflowActionType.FIND_RECORDS, | |
valid: false, | |
settings: { | |
...BASE_STEP_DEFINITION, | |
input: { | |
objectName: activeObjectMetadataItem?.nameSingular || '', | |
limit: 1, | |
}, | |
}, | |
}; | |
} | |
case WorkflowActionType.FIND_RECORDS: { | |
const activeObjectMetadataItem = | |
await this.objectMetadataRepository.findOne({ | |
where: { workspaceId, isActive: true, isSystem: false }, | |
}); | |
if (!isDefined(activeObjectMetadataItem)) { | |
throw new WorkflowVersionStepException( | |
'No active object metadata found', | |
WorkflowVersionStepExceptionCode.FAILURE, | |
); | |
} | |
return { | |
id: newStepId, | |
name: 'Search Records', | |
type: WorkflowActionType.FIND_RECORDS, | |
valid: false, | |
settings: { | |
...BASE_STEP_DEFINITION, | |
input: { | |
objectName: activeObjectMetadataItem.nameSingular, | |
limit: 1, | |
}, | |
}, | |
}; | |
} |
objectName: activeObjectMetadataItem?.nameSingular || '', | ||
limit: 1, | ||
}, |
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.
style: Consider adding validation for limit > 0 and setting a reasonable upper bound to prevent performance issues with large result sets.
Create Search Record action without filters neither sorting