-
Notifications
You must be signed in to change notification settings - Fork 0
classfunc #1
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?
classfunc #1
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.
Summary of Changes
Hello @zerolethanh, 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 introduces a comprehensive, schema-driven dynamic form system designed to simplify the creation and management of user input forms. It allows developers to define form structures and behaviors directly within Zod schemas, including dynamic field options fetched from APIs and complex inter-field dependencies. This system is integrated into the project's code generation pipeline, enabling API hooks to automatically expose ready-to-use form components, significantly reducing boilerplate and improving development efficiency for data input interfaces.
Highlights
- Dynamic Form System: Introduced a new
DynamicForm
React component (InputForm.tsx
) that leveragesreact-hook-form
andzod
for schema-driven form generation, validation, and submission. This component supports complex UI configurations and dynamic field options. - Advanced UI Configuration and Effects: Implemented
InputFormHelpers.tsx
to parse UI metadata from Zod schema descriptions, enabling flexible control over form field rendering (e.g., component type, placeholders, labels, custom props). It also supports dynamic field options fetched from API endpoints, with configurable methods, headers, and body, and the ability to trigger these fetches based on changes in other form fields. - Integration with Generated Hooks: The
useFn.template.tsx
is updated to expose aForm
component, allowing generated API hooks to automatically provide a dynamic form interface based on their input Zod schemas, streamlining form creation for API interactions. - Dependency Updates: Added new dependencies,
@hookform/resolvers
andreact-hook-form
, to support the newly introduced dynamic form functionality.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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
This pull request introduces a powerful DynamicForm
component and integrates it into the existing hook generation structure. This is a significant feature addition that will improve UI development velocity.
Overall, the implementation is solid, but there are several critical and high-severity issues that need to be addressed before merging. These primarily relate to incorrect hook usage patterns, potential race conditions, and type safety reductions that could lead to runtime errors and maintenance difficulties.
I've left detailed comments on the specific lines of code with suggestions for fixes. Please pay close attention to the comments marked as critical
and high
.
<DynamicForm | ||
formSchema={GreetingINData_schema} | ||
defaultValues={GreetingIN_defaultValues.data} | ||
useSubmitHook={useGreetingPost as 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.
This line introduces a critical architectural flaw. The useGreetingPost
hook is being passed as the useSubmitHook
prop, and DynamicForm
will invoke it as a hook. However, useGreetingPost
requires a set of props (inData
, stream
, etc.) that are not available in this context, which will lead to runtime errors.
This creates an incorrect recursive-like pattern where the hook is being used to render a component that then tries to use the same hook for submission.
A significant refactor is required. The useSubmitHook
prop should receive a properly configured hook factory, or the fire
function from the parent hook's instance should be passed down and used for submission. The current implementation will not work as intended.
if (!ui?.effects) return; | ||
|
||
ui.effects.forEach(async effect => { | ||
if (effect.listensTo === changedFieldName) { | ||
console.log(`[Effect Triggered] Field '${targetFieldName}' is listening to '${changedFieldName}'.`); | ||
|
||
const listenedValue = get(allFormValues, changedFieldName); | ||
|
||
// Reset field target trước khi fetch | ||
setValue(targetFieldName as Path<TData>, '' as any, {shouldValidate: true}); | ||
setDynamicOptions(prev => ({...prev, [targetFieldName]: []})); | ||
|
||
if (!listenedValue) { | ||
setFieldLoading(prev => ({...prev, [targetFieldName]: false})); | ||
return; | ||
} | ||
const currentFormValues = getValues(); | ||
|
||
if (effect.action === 'fetchOptions') { | ||
setFieldLoading(prev => ({...prev, [targetFieldName]: true})); | ||
|
||
// +++ REFACTORED: Sử dụng helper và chỉ thêm header khi có token +++ | ||
const token = await getAuthToken(); | ||
const headers: HeadersInit = { | ||
'Content-Type': 'application/json', | ||
...resolvePlaceholders(effect.headers, currentFormValues), | ||
} as Record<string, any>; | ||
if (token) { | ||
headers['Authorization'] = `Bearer ${token}`; | ||
} | ||
|
||
const requestOptions: RequestInit = { | ||
method: effect.method || 'POST', | ||
headers: headers, | ||
}; | ||
// +++ END REFACTOR +++ | ||
|
||
if (requestOptions.method !== 'GET' && effect.body) { | ||
requestOptions.body = JSON.stringify(resolvePlaceholders(effect.body, currentFormValues)); | ||
} | ||
|
||
fetch(effect.endpoint, requestOptions) | ||
.then(res => res.json()) | ||
.then(data => { | ||
let optionsData = effect.optionsPath ? get(data, effect.optionsPath) : data; | ||
if (!isArray(optionsData)) { | ||
console.warn(`[DynamicForm] Expected an array for field '${targetFieldName}'...`); | ||
optionsData = []; | ||
} | ||
setDynamicOptions(prev => ({...prev, [targetFieldName]: optionsData})); | ||
}) | ||
.catch(err => { | ||
console.error(`Failed to fetch options for ${targetFieldName}:`, err); | ||
setDynamicOptions(prev => ({...prev, [targetFieldName]: []})); | ||
}) | ||
.finally(() => { | ||
setFieldLoading(prev => ({...prev, [targetFieldName]: 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.
Array.prototype.forEach
does not wait for async
callbacks to complete. This means that if a field has multiple effects defined, they will all be triggered in parallel. This can lead to race conditions if the effects depend on one another or modify the same state.
To ensure effects are executed sequentially, you should use a for...of
loop, which correctly handles await
.
Example:
for (const effect of ui.effects) {
if (effect.listensTo === changedFieldName) {
// ... your async logic here, which can now be awaited
try {
// ... await fetch(...)
} catch (e) {
// ...
}
}
}
padding: 0.75rem; | ||
border-radius: 0.375rem; | ||
grid-column: 1 / -1; | ||
@apply break-all; |
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 @apply
directive is specific to Tailwind CSS and is not standard CSS. This will cause a parsing error in browsers and will not work unless this CSS module is processed by a tool like PostCSS with the Tailwind CSS plugin.
If that build setup is not in place, you should replace this with standard CSS properties to ensure the styles are applied correctly.
@apply break-all; | |
word-break: break-all; |
method: z.enum(['GET', 'POST', 'PUT', 'DELETE']).optional().default('POST'), | ||
// +++ NEW: Headers cho request, hỗ trợ placeholder | ||
headers: z.record(z.string()).optional(), | ||
// +++ NEW: Body của request, hỗ trợ placeholder | ||
// Ví dụ: { "countryCode": "{{country}}", "searchTerm": "{{search_term}}" } | ||
body: z.record(z.any()).optional(), | ||
// Tên của field trong object trả về chứa mảng options (hữu ích khi API trả về dạng { data: [...] }) | ||
optionsPath: z.string().optional(), | ||
}); |
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.
}); | ||
}, [formSchema, getValues, setDynamicOptions, setFieldLoading, setValue]); | ||
|
||
|
||
// +++ NEW: Effect để chạy các side-effect cho giá trị mặc định KHI MOUNT +++ | ||
useEffect(() => { | ||
console.log("[DynamicForm] Component mounted. Checking default values to run initial effects..."); | ||
if (defaultValues) { | ||
const initialFormValues = getValues(); | ||
Object.keys(defaultValues).forEach(fieldName => { | ||
if (initialFormValues[fieldName]) { | ||
runEffectsFor(fieldName, initialFormValues); | ||
} | ||
}); | ||
} | ||
}, [runEffectsFor, defaultValues]); // Chỉ chạy khi component mount hoặc defaultValues thay đổi | ||
|
||
// +++ MODIFIED: Effect `watch` giờ đây đơn giản hơn +++ | ||
useEffect(() => { | ||
const subscription = watch((value, {name, type}) => { | ||
if (!name || type !== 'change') return; | ||
// Chỉ cần gọi hàm tái sử dụng |
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.
This useEffect
is designed to run only once on component mount by using the initialEffectsRan
ref. However, if the defaultValues
prop changes during the component's lifecycle, this effect will not re-trigger to run the effects for the new default values. This might be a bug if defaultValues
is expected to be dynamic.
If the initial effects should re-run when defaultValues
changes, you should consider resetting initialEffectsRan.current
to false
in another effect that tracks changes to defaultValues
.
import React, {JSX, ReactNode, useCallback, useEffect, useTransition} from 'react'; | ||
|
||
import {getAuthToken, getHtmlInputType, getUiMetadata, getZodInnerType, resolvePlaceholders} from './InputFormHelpers'; | ||
// @ts-ignore |
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.
Using @ts-ignore
should be avoided as it suppresses type-checking errors. This is likely present because TypeScript doesn't have type definitions for .module.css
files.
To fix this properly, you can add a declaration file (e.g., declarations.d.ts
) in your project with the following content:
declare module '*.module.css' {
const classes: { readonly [key: string]: string };
export default classes;
}
This will provide type safety for all CSS module imports and allow you to remove the @ts-ignore
.
renderSuccessContent?: (result: { apiResponse: any, submittedValues: TData }) => React.ReactNode; successDataPath?: string; customStyles?: CustomStyles; componentRegistry?: Record<string, React.ComponentType<any>>; showResponseDetails?: boolean; successDataPathFields?: DataPathFieldConfig[];
# Conflicts: # esbuild_out/scripts/templates/InputForm/DataDisplayTable.tsx # esbuild_out/scripts/templates/InputForm/InputForm.tsx
# Conflicts: # esbuild_out/scripts/templates/InputForm/DataDisplayTable.tsx # esbuild_out/scripts/templates/InputForm/InputForm.tsx
No description provided.