-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix issues with Apollo cache in workflow module #7569
Conversation
…ache has not been filled yet with other versions than the one deleted
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.
LGTM!
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 pull request addresses Apollo cache synchronization issues in the workflow module, focusing on improving cache updates after mutations and enhancing UI consistency.
- Simplified cache update logic in
triggerDeleteRecordsOptimisticEffect.ts
by removing unnecessary code and variables - Updated
useActivateWorkflowVersion
hook to directly modify cache, updating activated version status and archiving other active versions - Modified
useDeactivateWorkflowVersion
to usemodifyRecordFromCache
for direct cache updates instead of refetching data - Improved
useWorkflowWithCurrentVersion
hook to better handle current workflow version selection, addressing active version handling issues - Updated
RecordShowPageWorkflowHeader
andRecordShowPageWorkflowVersionHeader
components to ensure proper cache updates after workflow operations
7 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
const activateWorkflowVersion = async ({ | ||
workflowVersionId, | ||
workflowId, | ||
}: { | ||
workflowVersionId: string; | ||
workflowId: string; | ||
}) => { |
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 a type definition for the function parameters
for (const workflowVersion of allWorkflowVersions) { | ||
apolloClient.cache.modify({ | ||
id: apolloClient.cache.identify(workflowVersion), | ||
fields: { | ||
status: () => { | ||
return workflowVersion.id !== workflowVersionId && | ||
workflowVersion.status === 'ACTIVE' | ||
? 'ARCHIVED' | ||
: workflowVersion.status; | ||
}, | ||
}, | ||
}); | ||
} |
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: This loop modifies cache entries individually. Consider using a batch update if possible for better performance
const apolloMetadataClient = useApolloMetadataClient(); | ||
const apolloClient = useApolloClient(); |
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 moving these client initializations outside the hook for better performance
const [mutate] = useMutation< | ||
ActivateWorkflowVersionMutation, | ||
ActivateWorkflowVersionMutationVariables | ||
>(DEACTIVATE_WORKFLOW_VERSION, { | ||
client: apolloMetadataClient ?? ({} as ApolloClient<any>), | ||
}); |
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: The mutation type seems to be incorrect. It's using ActivateWorkflowVersionMutation for a deactivate operation
recordId: workflowVersionId, | ||
objectMetadataItem: objectMetadataItemWorkflowVersion, | ||
fieldModifiers: { | ||
status: () => 'DEACTIVATED', |
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 using an enum or constant for the status value
Fixes #7523