-
Notifications
You must be signed in to change notification settings - Fork 19.9k
refactor(workflow-tool): update types and improve modal handling #31525
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
base: main
Are you sure you want to change the base?
Conversation
- Removed explicit 'any' types in favor of 'unknown' for better type safety. - Refactored the WorkflowToolConfigureButton component to utilize a custom hook for managing modal state and logic. - Introduced new components for input and output tables to streamline the workflow tool configuration process. - Enhanced the form handling logic with a dedicated hook for managing form state and validation. - Cleaned up unused imports and improved overall code organization.
Summary of ChangesHello @CodingOnStar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant refactoring of the workflow tool configuration feature. The primary goal is to enhance code maintainability, improve type safety, and streamline the user interface for configuring workflow tools. By introducing custom hooks for state and form management, and dedicated components for input/output tables, the changes lead to a more modular and robust codebase, making future development and debugging more efficient. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
The pull request effectively refactors the workflow tool configuration, moving state management and logic into custom hooks (useConfigureButton, useWorkflowToolForm, useModalState). This significantly improves code organization, readability, and maintainability. The introduction of ToolInputTable and ToolOutputTable components further modularizes the UI. Type safety has been enhanced by replacing some any types with unknown and more specific types, and the eslint-suppressions.json update reflects this improvement. However, there are a few areas where magic strings are used and a React Hooks rule is violated, which should be addressed.
web/app/components/tools/workflow-tool/hooks/use-modal-state.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const ParameterRow: FC<ParameterRowProps> = ({ item, index, onParameterChange }) => { | ||
| const { t } = useTranslation() | ||
| const isImageParameter = item.name === '__image' |
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.
The string '__image' is a magic string. It would be better to define this as a constant to improve readability and prevent potential typos.
| const isImageParameter = item.name === '__image' | |
| const IMAGE_PARAMETER_NAME = '__image' | |
| const isImageParameter = item.name === IMAGE_PARAMETER_NAME |
| const isTextType = item.type === 'paragraph' || item.type === 'text-input' | ||
| return isTextType && param.type !== '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.
The strings 'paragraph' and 'text-input' are magic strings. It would be better to define these as constants (e.g., InputVarType.paragraph, InputVarType.textInput) to improve readability and maintainability.
const isTextType = item.type === InputVarType.paragraph || item.type === InputVarType.textInput| return { | ||
| name: item.variable, | ||
| required: item.required, | ||
| type: item.type === 'paragraph' ? 'string' : item.type, |
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.
| { | ||
| name: 'text', | ||
| description: t('nodes.tool.outputVars.text', { ns: 'workflow' }), | ||
| type: VarType.string, | ||
| reserved: true, | ||
| }, | ||
| { | ||
| name: 'files', | ||
| description: t('nodes.tool.outputVars.files.title', { ns: 'workflow' }), | ||
| type: VarType.arrayFile, | ||
| reserved: true, | ||
| }, | ||
| { | ||
| name: 'json', | ||
| description: t('nodes.tool.outputVars.json', { ns: 'workflow' }), | ||
| type: VarType.arrayObject, | ||
| reserved: true, | ||
| }, |
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.
The strings 'text', 'files', and 'json' used for name and the VarType values are magic strings. These should be defined as constants (e.g., RESERVED_OUTPUT_NAMES.TEXT, VarType.string) to improve readability, prevent typos, and make future changes easier.
const reservedOutputParameters = useMemo<WorkflowToolProviderOutputParameter[]>(() => [
{
name: 'text',
description: t('nodes.tool.outputVars.text', { ns: 'workflow' }),
type: VarType.string,
reserved: true,
},
{
name: 'files',
description: t('nodes.tool.outputVars.files.title', { ns: 'workflow' }),
type: VarType.arrayFile,
reserved: true,
},
{
name: 'json',
description: t('nodes.tool.outputVars.json', { ns: 'workflow' }),
type: VarType.arrayObject,
reserved: true,
},
], [t])| Toast.notify({ | ||
| type: 'error', | ||
| message: t('createTool.nameForToolCall', { ns: 'tools' }) + t('createTool.nameForToolCallTip', { ns: 'tools' }), | ||
| }) |
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.
The concatenation of two translation keys for an error message can be clunky and might lead to awkward phrasing in different languages. Consider creating a single translation key that encompasses the full error message, allowing for more natural and flexible translations (e.g., createTool.nameForToolCallInvalidTip).
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.
Pull request overview
This pull request refactors the workflow-tool component to improve code organization and type safety. It extracts complex logic into custom hooks, splits large components into smaller, reusable pieces, and replaces explicit 'any' types with 'unknown' for better type safety.
Changes:
- Extracted modal state management into a
useModalStatehook - Created
useWorkflowToolFormhook to encapsulate form state and validation logic - Introduced
useConfigureButtonhook to manage configure button state and API interactions - Split workflow tool configuration UI into smaller components (FormField, FooterActions, ToolInputTable, ToolOutputTable)
- Replaced 'any' types with 'unknown' or more specific types (TypeWithI18N)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web/eslint-suppressions.json | Removed ts/no-explicit-any suppressions (4 → 2) after fixing explicit any types |
| web/app/components/tools/types.ts | Changed team_credentials to use unknown instead of any, changed Tool.description to TypeWithI18N, added label field to WorkflowToolProviderRequest |
| web/app/components/tools/workflow-tool/index.tsx | Refactored main modal component to use custom hooks, extracted FormField and FooterActions components, changed payload type to WorkflowToolFormPayload |
| web/app/components/tools/workflow-tool/configure-button.tsx | Refactored to use useConfigureButton hook, extracted UnpublishedCard, NonManagerCard, and PublishedActions components |
| web/app/components/tools/workflow-tool/hooks/use-modal-state.ts | New hook for managing modal open/close state |
| web/app/components/tools/workflow-tool/hooks/use-workflow-tool.ts | New hooks for React Query-based API calls (fetch, create, update workflow tools) |
| web/app/components/tools/workflow-tool/hooks/use-workflow-tool-form.ts | New hook encapsulating form state, validation, and submission logic |
| web/app/components/tools/workflow-tool/hooks/use-configure-button.ts | New hook managing configure button state, data fetching, and update logic |
| web/app/components/tools/workflow-tool/components/tool-input-table.tsx | Extracted input parameters table into reusable component |
| web/app/components/tools/workflow-tool/components/tool-output-table.tsx | Extracted output parameters table into reusable component |
| web/app/components/tools/provider/detail.tsx | Updated type casting for WorkflowToolModal payload to match new type structure |
Comments suppressed due to low confidence (1)
web/app/components/tools/workflow-tool/index.tsx:36
- The test file imports
WorkflowToolModalPayloadwhich still exists but is no longer used internally (replaced byWorkflowToolFormPayload). The tests should be updated to useWorkflowToolFormPayloadinstead, and the oldWorkflowToolModalPayloadtype should be removed or deprecated to avoid confusion. Note thatWorkflowToolModalPayload.outputParametersis required whileWorkflowToolFormPayload.outputParametersis optional, which may require test updates.
export type WorkflowToolModalPayload = {
icon: Emoji
label: string
name: string
description: string
parameters: WorkflowToolProviderParameter[]
outputParameters: WorkflowToolProviderOutputParameter[]
labels: string[]
privacy_policy: string
tool?: {
output_schema?: WorkflowToolProviderOutputSchema
}
workflow_tool_id?: string
workflow_app_id?: string
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
web/app/components/tools/workflow-tool/hooks/use-modal-state.ts
Outdated
Show resolved
Hide resolved
- Introduced a custom QueryClientProvider for improved test isolation in WorkflowToolConfigureButton tests. - Updated tests to utilize the new renderWithQueryClient function for consistent query handling. - Refactored modal state management to ensure proper updates and handling of external changes. - Improved type definitions for better clarity and maintainability. - Added comprehensive tests for edge cases and user interactions in the WorkflowToolConfigureButton component.
Summary
Checklist
make lintandmake type-check(backend) andcd web && npx lint-staged(frontend) to appease the lint gods