Implement contextual actions for the workflows #8814
Conversation
…extual-actions-for-the-workflows
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR implements comprehensive workflow management functionality through contextual actions in the action menu system. Here's a summary of the key changes:
- Added workflow-specific actions (test, activate/deactivate, draft management) with proper state handling and user feedback
- Implemented workflow version management actions (use as draft, view history, executions) with navigation support
- Created new hooks for workflow operations with proper cleanup and state management
- Added confirmation modals for destructive actions like discarding drafts
- Added success notifications with icons for workflow execution status
Key points to review:
- Error handling is missing in several workflow action hooks
- Potential race conditions in state management between workflow and version loading
- Some hooks have inconsistent logic in onClick handlers (e.g., deactivate workflow)
- No loading states implemented for async operations
- Position parameters could benefit from centralized constants instead of hardcoded values
36 file(s) reviewed, 36 comment(s)
Edit PR Review Bot Settings | Greptile
| {contextStoreTargetedRecordsRule.mode === 'exclusion' || | ||
| (contextStoreTargetedRecordsRule.mode === 'selection' && | ||
| contextStoreTargetedRecordsRule.selectedRecordIds.length > 1 && ( | ||
| <MultipleRecordsActionMenuEntrySetterEffect | ||
| objectMetadataItem={objectMetadataItem} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
logic: The exclusion mode check is incorrectly structured - the OR condition will always evaluate the second expression. This should be grouped with parentheses to properly handle both conditions
| registerDeleteMultipleRecordsAction({ position: 0 }); | ||
| registerExportMultipleRecordsAction({ position: 1 }); |
There was a problem hiding this comment.
style: position parameter is now passed during registration rather than hook initialization, which could cause issues if these actions are registered multiple times with different positions
| unregisterDeleteMultipleRecordsAction(); | ||
| unregisterExportViewNoSelectionRecordsAction(); | ||
| unregisterExportMultipleRecordsAction(); |
There was a problem hiding this comment.
style: unregister functions no longer receive position parameters while register functions do - consider making the APIs more consistent
| objectMetadataItem={objectMetadataItem} | ||
| /> | ||
| {objectMetadataItem.nameSingular === CoreObjectNameSingular.Workflow && ( | ||
| <WorkflowSingleRecordActionMenuEntrySetterEffect startPosition={3} /> |
There was a problem hiding this comment.
style: startPosition={3} is hardcoded in multiple places. Consider making this configurable or using a constant to avoid magic numbers
| {objectMetadataItem.nameSingular === CoreObjectNameSingular.Workflow && ( | ||
| <WorkflowSingleRecordActionMenuEntrySetterEffect startPosition={3} /> | ||
| )} | ||
| {objectMetadataItem.nameSingular === | ||
| CoreObjectNameSingular.WorkflowVersion && ( | ||
| <WorkflowVersionsSingleRecordActionMenuEntrySetterEffect | ||
| startPosition={3} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
style: workflow and workflow version checks could be combined into a switch statement for better maintainability as more object types are added
| import { useFindManyRecords } from '@/object-record/hooks/useFindManyRecords'; | ||
| import { Workflow, WorkflowVersion } from '@/workflow/types/Workflow'; | ||
|
|
||
| export const useActiveWorkflowVersion = (workflowId?: string) => { |
There was a problem hiding this comment.
style: missing JSDoc documentation for the hook's purpose, parameters, and return value
| }, | ||
| }); | ||
|
|
||
| return workflowVersions?.[0]; |
There was a problem hiding this comment.
logic: no handling for case where multiple active versions exist - could lead to inconsistent state
| workflowId: { | ||
| eq: workflowId, | ||
| }, |
There was a problem hiding this comment.
logic: no validation if workflowId is undefined - could cause unnecessary query execution
| await mutate({ | ||
| variables: { input: { workflowVersionId, payload } }, | ||
| }); |
There was a problem hiding this comment.
style: no error handling for failed mutations - consider wrapping in try/catch to show error snackbar
| payload?: Record<string, any>; | ||
| }) => { | ||
| await mutate({ | ||
| variables: { input: { workflowVersionId, payload } }, | ||
| }); | ||
|
|
||
| enqueueSnackBar('', { |
There was a problem hiding this comment.
style: empty message string passed to snackbar - consider removing the empty string parameter if not needed
Implemented the following actions for the workflows:
And the following actions for the workflow versions:
Video example:
Enregistrement.de.l.ecran.2024-11-29.a.16.38.20.mov
A few of these actions are links to the relations of the workflow object (link to a filtered table). To generalize this behavior, I will create an hook named
useSeeRelationsActionSingleRecordActionto automatically generate links to a showPage if the relation is a Many To One or to a filtered table if the relation is a One to Many for all the record types.I will also create actions which will allow to create a relation.