fix: naive ui form reset does not meet expectations #4569
Conversation
|
WalkthroughThe pull request introduces modifications primarily to the form handling capabilities within the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
packages/@core/ui-kit/form-ui/src/config.ts (1)
Line range hint
1-85: Summary and Next StepsThe changes in this file improve configuration handling and type safety. However, they don't directly address the form reset issue (#4564). To fully resolve the issue:
- Verify the results of the shell scripts to understand how the configuration options and shadcn-ui components are used in relation to form reset functionality.
- If necessary, implement additional changes to address the form reset behavior, possibly in the shadcn-ui components or in the form reset logic.
- Add unit tests to ensure the correct behavior of the form reset functionality.
- Update the documentation to reflect any changes in the form reset behavior or configuration options.
Consider implementing a dedicated method for form reset in the
setupVbenFormfunction or in a separate utility, which could provide more control over the reset behavior across different UI component libraries.apps/web-naive/src/adapter/form.ts (1)
Line range hint
1-124: Overall assessment: The changes effectively address the form reset issue.The modifications to
setupVbenFormare focused and align well with the PR objectives. They should resolve the reported bug where the reset text wasn't disappearing after form reset. The rest of the file remains unchanged, which minimizes the risk of introducing new issues.To ensure a comprehensive review:
- Verify that these changes don't negatively impact other parts of the application that use
useVbenForm.- Consider adding or updating unit tests to cover the form reset functionality.
- Update the documentation to reflect these changes, especially noting the new behavior of form resets.
packages/@core/ui-kit/form-ui/src/form-render/form.vue (2)
90-90: Approved: Addition ofemptyStateValuepropertyThe introduction of the
emptyStateValueproperty is a good addition that aligns with the PR's objective of improving form reset functionality. It provides a mechanism to control the empty state of form fields, which can be useful in addressing the reset behavior.Consider adding a comment explaining the purpose and usage of
emptyStateValueto improve code readability and maintainability.
Line range hint
90-111: Summary: Foundation laid for addressing form reset issueThe changes introduced in this PR provide a solid foundation for addressing the form reset functionality issue. The addition of
emptyStateValueand its incorporation into the form schema allow for more precise control over the reset behavior of individual form fields.However, these changes alone may not fully resolve the issue of reset text not disappearing after form reset. To complete the fix:
- Ensure that the
FormFieldcomponent correctly utilizes theemptyStateValueproperty.- Implement logic in the reset handling to clear the reset text based on the
emptyStateValue.- Add test cases to verify the correct behavior of the form reset functionality, specifically checking if the reset text disappears as expected.
Consider documenting the usage of
emptyStateValuein the component's documentation or comments to guide other developers on how to leverage this new feature for proper form reset behavior.packages/@core/ui-kit/form-ui/src/types.ts (2)
156-159: Approved: Addition ofemptyStateValuetoFormCommonConfigThe addition of the
emptyStateValueproperty is a good solution to address the form reset functionality issue mentioned in the PR objectives. This will allow for consistent empty state handling across different UI frameworks.Consider adding an English translation of the Chinese comment for better international developer understanding:
/** * Empty state value for all form items. Default is undefined, while naive-ui's empty state value is null. * 所有表单项的空状态值,默认都是undefined，naive-ui的空状态值是null */ emptyStateValue?: null | undefined;
348-348: Approved: Addition ofemptyStateValuetoVbenFormAdapterOptionsThe addition of the
emptyStateValueproperty to theVbenFormAdapterOptionsinterface is consistent with the changes made toFormCommonConfigand provides necessary flexibility for adapter-level configuration.For consistency and clarity, consider adding a comment similar to the one in
FormCommonConfig:config?: { baseModelPropName?: string; disabledOnChangeListener?: boolean; /** * Empty state value for all form items. Default is undefined, while naive-ui's empty state value is null. */ emptyStateValue?: null | undefined; modelPropNameMap?: Partial<Record<T, string>>; };packages/@core/ui-kit/form-ui/src/form-render/form-field.vue (2)
36-36: Approve addition ofemptyStateValuepropertyThe addition of the
emptyStateValueproperty aligns with the PR objective to fix the form reset functionality. This property likely provides a default value when the model value is undefined, addressing the issue where reset text doesn't disappear.Consider adding a comment to explain the purpose and usage of
emptyStateValuefor better code maintainability.
217-217: Approve modification infieldBindEventfunctionThe change in the
fieldBindEventfunction directly addresses the PR objective by usingemptyStateValuewhen the value is undefined. This should fix the issue where reset text doesn't disappear after form reset.Consider using the nullish coalescing operator (
??) for a more concise expression:[bindEventField]: value ?? emptyStateValue,This would handle both
undefinedandnullcases.packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (1)
Line range hint
115-118: Ensure consistency of 'autoLoad' configuration between 'options' and 'init'In the
optionscomputed property (lines 115-118), you setmergedOptions.proxyConfig.autoLoad = false, which prevents automatic data loading. However, in theinitfunction (lines 168-172),autoLoadis retrieved fromdefaultGridOptions.proxyConfig.autoLoad, which might betrueif not explicitly set tofalseindefaultGridOptions. This could lead to inconsistent behavior whereautoLoadistrueininitbutfalseinoptions.To maintain consistency, consider setting
defaultGridOptions.proxyConfig.autoLoad = falsein theinitfunction as well:defaultGridOptions.proxyConfig.autoLoad = false;Alternatively, you could adjust
initto referenceoptions.value.proxyConfig.autoLoadto ensure both places use the same configuration:const autoLoad = options.value.proxyConfig?.autoLoad;This ensures that the
autoLoadsetting is consistent across both the computed property and the initialization function.Also applies to: 168-172
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- apps/web-naive/src/adapter/form.ts (1 hunks)
- packages/@core/ui-kit/form-ui/src/config.ts (1 hunks)
- packages/@core/ui-kit/form-ui/src/form-render/form-field.vue (4 hunks)
- packages/@core/ui-kit/form-ui/src/form-render/form.vue (2 hunks)
- packages/@core/ui-kit/form-ui/src/types.ts (2 hunks)
- packages/effects/common-ui/src/ui/authentication/types.ts (1 hunks)
- packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (1 hunks)
- packages/stores/src/modules/user.ts (1 hunks)
- packages/utils/src/index.ts (1 hunks)
🔇 Additional comments (12)
packages/utils/src/index.ts (2)
2-2: LGTM: New export statement added.The addition of
export * from '@vben-core/shared/cache';looks good. This change makes cache-related functionalities from the shared package available through this utils module.
1-4: Verify the relevance of this change to the PR objectives.The added export statement for cache functionality doesn't seem directly related to the form reset issue mentioned in the PR objectives. Could you please clarify how this change addresses the problem with the reset text not disappearing in the
useVbenFormcomponent?To help understand the connection, let's check for any usage of the newly exported cache functionality in relation to form reset:
✅ Verification successful
The added export for cache functionality is not utilized in form-related files and does not impact the form reset issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of cache functionality in form-related files # Look for imports from '@vben-core/shared/cache' in form-related files echo "Searching for cache imports in form-related files:" rg --type typescript "from '@vben-core/shared/cache'" -g "*form*.ts" -g "*form*.vue" # Look for potential usage of cache in form reset logic echo "Searching for potential cache usage in form reset logic:" rg --type typescript "resetFields.*cache" -g "*form*.ts" -g "*form*.vue"Length of output: 471
Script:
#!/bin/bash # Description: Search for usage of cache functionality in form-related files # Look for imports from '@vben-core/shared/cache' in form-related files echo "Searching for cache imports in form-related files:" rg --type ts "from '@vben-core/shared/cache'" -g "*form*.ts" -g "*form*.vue" # Look for potential usage of cache in form reset logic echo "Searching for potential cache usage in form reset logic:" rg --type ts "resetFields.*cache" -g "*form*.ts" -g "*form*.vue"Length of output: 385
packages/stores/src/modules/user.ts (1)
4-4: Approved, but clarification needed on PR objectives alignmentThe addition of
[key: string]: any;to theBasicUserInfointerface increases its flexibility, allowing for dynamic properties beyond the explicitly defined ones. This can be beneficial for handling varying data structures.However, I have some concerns:
- This change may reduce type safety and could potentially mask typos or incorrect property usage.
- Debugging might become more challenging if unexpected properties are added to
BasicUserInfoobjects.Could you please clarify how this change relates to the form reset functionality mentioned in the PR objectives? It's not immediately clear how this interface modification addresses the described issue.
If possible, consider using a more specific type instead of
any. For example, if you know the potential additional properties, you could use a union type or extend the interface. This would maintain flexibility while preserving some type safety.To verify the impact of this change, let's check for any usage of
BasicUserInfothat might be affected:✅ Verification successful
Change Verified: Index Signature Addition is Safe
The addition of
[key: string]: any;to theBasicUserInfointerface has been reviewed.
- No dynamic property accesses on
BasicUserInfowere found in the current codebase, ensuring that type safety remains intact.- The interface modifications do not negatively impact existing functionalities.
However, please clarify how this change aligns with the PR's objective of addressing the form reset functionality bug, as the connection is not immediately evident.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of BasicUserInfo and check for any property access that might be affected by this change # Search for BasicUserInfo usage echo "Searching for BasicUserInfo usage:" rg --type typescript "BasicUserInfo" -C 3 # Search for potential dynamic property access on BasicUserInfo objects echo "\nSearching for potential dynamic property access:" rg --type typescript "userInfo\[.+\]" -C 3Length of output: 346
Script:
#!/bin/bash # Description: Find usages of BasicUserInfo and check for any property access that might be affected by this change # Search for BasicUserInfo usage in TypeScript files echo "Searching for BasicUserInfo usage in .ts and .tsx files:" rg "BasicUserInfo" --glob "*.ts" --glob "*.tsx" -C 3 # Search for potential dynamic property access on BasicUserInfo objects in TypeScript files echo "\nSearching for potential dynamic property access in .ts and .tsx files:" rg "userInfo\[" --glob "*.ts" --glob "*.tsx" -C 3Length of output: 2465
packages/@core/ui-kit/form-ui/src/config.ts (2)
46-52: Improved configuration handling, but consider form reset functionalityThe changes to the
setupVbenFormfunction improve the configuration handling:
- The use of destructuring with default values for
disabledOnChangeListenerandemptyStateValueis more robust.- Using
Object.assignto updateDEFAULT_FORM_COMMON_CONFIGcentralizes the configuration updates, improving maintainability.These changes enhance the overall code quality. However, they don't directly address the form reset issue mentioned in the PR objectives (#4564).
To ensure these changes don't indirectly affect the form reset functionality, let's verify the usage of these configuration options:
Line range hint
1-45: Verify shadcn-ui components behavior during form resetWhile the changes in this file are minimal, it's important to note that the form components are imported from '@vben-core/shadcn-ui'. To ensure that the form reset issue (#4564) is fully addressed, we should verify the behavior of these shadcn-ui components during the form reset operation.
Let's check the implementation of these components:
Also applies to: 53-85
apps/web-naive/src/adapter/form.ts (1)
87-90: Approve: These changes effectively address the form reset issue.The two new configuration options added to
setupVbenFormdirectly tackle the reported bug:
disabledOnChangeListener: true: This prevents potential conflicts with the naive-ui components' internal event handling, which could have been interfering with the form reset functionality.
emptyStateValue: null: This ensures that when the form is reset, the components will properly clear their values tonullinstead ofundefined. This is crucial for naive-ui components to recognize the reset state and update their display accordingly.These changes align well with the PR objectives and should resolve the issue where the reset text wasn't disappearing after form reset.
To ensure these changes don't introduce any unintended side effects, let's verify the usage of
setupVbenFormacross the project:✅ Verification successful
**: The changes effectively address the form reset issue without impacting other parts of the codebase. No additional issues were found in other usages of
setupVbenFormor related configurations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other usages of setupVbenForm that might be affected by these changes # Search for other occurrences of setupVbenForm echo "Searching for other usages of setupVbenForm:" rg --type typescript "setupVbenForm" -C 5 # Check for any custom implementations that might rely on the previous behavior echo "\nChecking for potential conflicts with custom form implementations:" rg --type typescript "useVbenForm|FormComponentType" -C 5Length of output: 417
Script:
#!/bin/bash # Description: Check for any other usages of setupVbenForm that might be affected by these changes # Search for other occurrences of setupVbenForm echo "Searching for other usages of setupVbenForm:" rg --type ts "setupVbenForm" -C 5 # Check for any custom implementations that might rely on the previous behavior echo "\nChecking for potential conflicts with custom form implementations:" rg --type ts "useVbenForm|FormComponentType" -C 5Length of output: 42070
packages/@core/ui-kit/form-ui/src/form-render/form.vue (1)
111-111: Approved: Inclusion ofemptyStateValuein schema objectThe inclusion of
emptyStateValuein the returned schema object is a positive change. It allows for more granular control over the reset behavior of individual form fields, which is crucial for addressing the issue described in the PR.To ensure this change has the intended effect, please verify that the
FormFieldcomponent correctly utilizes theemptyStateValueproperty. You may want to add a test case that checks if the form reset functionality now behaves as expected, particularly focusing on the disappearance of reset text.packages/@core/ui-kit/form-ui/src/types.ts (1)
Line range hint
1-359: Summary: Targeted changes to address form reset functionalityThe changes made to this file are focused and consistent, adding the
emptyStateValueproperty to bothFormCommonConfigandVbenFormAdapterOptionsinterfaces. These additions directly address the form reset functionality issue mentioned in the PR objectives.The changes allow for flexible configuration of empty state values, which should resolve the discrepancy between the expected behavior and the actual behavior of the form reset functionality. This solution is minimal and targeted, reducing the risk of unintended side effects while providing the necessary flexibility to handle empty states consistently across different UI frameworks.
To further improve the changes:
- Consider adding English translations to the Chinese comments for better international developer understanding.
- Ensure that the implementation of these new properties in the actual form logic correctly handles the reset functionality as intended.
- Update any relevant documentation or usage examples to reflect these new configuration options.
To ensure that these changes are properly implemented and don't introduce any regressions, please run the following verification script:
packages/@core/ui-kit/form-ui/src/form-render/form-field.vue (3)
59-59: Approve renaming offieldComponenttoFieldComponentThe renaming of the computed property from
fieldComponenttoFieldComponentfollows the Vue 3 convention of using PascalCase for component names. This change improves code consistency and readability.
304-304: Approve template update for renamed componentThe change in the template section correctly reflects the renaming of the computed property from
fieldComponenttoFieldComponent. This ensures consistency between the script and template sections.
Line range hint
1-341: Summary of changes and overall approvalThe changes implemented in this file effectively address the reported issue with the form reset functionality. The addition of the
emptyStateValueproperty and its usage in thefieldBindEventfunction should resolve the problem where reset text doesn't disappear after form reset. The code changes are well-structured and follow Vue 3 conventions.A few minor suggestions have been made for further improvement:
- Adding documentation for the new
emptyStateValueproperty.- Considering the use of the nullish coalescing operator in the
fieldBindEventfunction.Overall, these changes are approved and should be ready for merging after addressing the minor suggestions.
packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (1)
Line range hint
227-232: Confirm that the form reset functionality resolves issue #4564The
handleResetfunction invbenFormOptionsis updated to reset the form values and then reload the grid data using the reset values. Please verify that this change effectively fixes the bug described in issue #4564, ensuring that clicking the reset button not only resets the form values but also clears the reset text as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
playground/src/api/core/auth.ts (1)
Line range hint
27-29: UpdateloginApifunction to handle optional parametersWith the changes to the
LoginParamsinterface, theloginApifunction should be updated to handle cases whereusernameorpasswordmight be undefined. Consider adding validation or default values to ensure the function behaves correctly with the new optional parameters.Here's a suggested modification:
export async function loginApi(data: AuthApi.LoginParams) { + if (!data.username || !data.password) { + throw new Error('Username and password are required for login'); + } return requestClient.post<AuthApi.LoginResult>('/auth/login', data); }This change ensures that the function explicitly checks for the presence of both username and password before making the API call, maintaining the previous behavior and preventing potential issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apps/web-antd/src/api/core/auth.ts (1 hunks)
- apps/web-ele/src/api/core/auth.ts (1 hunks)
- apps/web-naive/src/api/core/auth.ts (1 hunks)
- playground/src/api/core/auth.ts (1 hunks)
🔇 Additional comments (5)
apps/web-antd/src/api/core/auth.ts (1)
6-7:⚠️ Potential issueConsider the implications of making login parameters optional
The changes to the
LoginParamsinterface make bothpasswordandusernameoptional. This modification has several implications:
- It allows for more flexible use of the interface, potentially supporting different authentication methods.
- However, it may lead to unexpected behavior if not handled correctly in the implementation.
To ensure this change doesn't introduce unintended consequences:
- Verify that the server-side implementation can handle requests where either
passwordorusernamemight be missing.- Update the
loginApifunction and any other functions usingLoginParamsto handle cases where these fields might be undefined.- Update relevant documentation to reflect this change in the API contract.
Let's verify the usage of
LoginParamsin the codebase:Additionally, this change doesn't seem to directly address the form reset issue mentioned in the PR objectives. Could you clarify how this change relates to the described bug, or if this is an unrelated modification?
apps/web-ele/src/api/core/auth.ts (2)
6-7:⚠️ Potential issueConsider the implications of making
passwordandusernameoptional.The changes to the
LoginParamsinterface make bothpasswordandusernameoptional. While this allows for more flexibility in the login process, it raises some concerns:
- Security: Making the password optional could potentially lead to security vulnerabilities if not handled correctly in the backend.
- Functionality: It's unusual for both username and password to be optional in a standard login process. This change might indicate a shift in the authentication mechanism (e.g., supporting alternative login methods).
- Consistency: Ensure that this change is reflected in the backend API and doesn't break existing functionality.
To verify the impact and usage of this change, please run the following script:
#!/bin/bash # Description: Check for usage of LoginParams and loginApi function # Search for LoginParams usage echo "LoginParams usage:" rg --type typescript "LoginParams" -C 3 # Search for loginApi function calls echo "\nloginApi function calls:" rg --type typescript "loginApi\(" -C 3 # Check if there are any comments or TODOs related to this change echo "\nRelated comments or TODOs:" rg --type typescript "TODO.*login|FIXME.*login" -C 2This will help us understand how these changes affect the rest of the codebase and if there are any related comments or TODOs that need to be addressed.
Line range hint
27-29: Review and updateloginApifunction if necessary.Given the changes to the
LoginParamsinterface, theloginApifunction may need to be updated to handle cases whereusernameorpasswordare not provided. Please review the implementation of this function and any related backend logic to ensure it correctly handles optional parameters.To help with this review, please run the following script to examine the
loginApifunction implementation:This will help us understand how the
loginApifunction is implemented and if there's any existing error handling for missing credentials that might need to be updated.apps/web-naive/src/api/core/auth.ts (1)
6-7:⚠️ Potential issueClarify the purpose of making
passwordandusernameoptionalThe changes to the
LoginParamsinterface make bothpasswordandusernameoptional. This raises several concerns:
These changes don't seem to align with the PR objectives, which mention fixing a bug related to form reset in the naive UI framework. Could you clarify how these changes address the described issue?
Making these fields optional could lead to potential issues:
- It may allow for empty login attempts, which might not be the intended behavior.
- If the backend API still requires these fields, this change could lead to unexpected errors.
This change might require updates to form validation logic elsewhere in the codebase.
Could you please provide more context on why these fields are being made optional? Additionally, let's verify the impact of these changes:
If this change is intentional, consider the following:
- Ensure that the
loginApifunction and any associated form validation logic handle these optional fields appropriately.- Update the documentation to reflect the new optional nature of these fields and any new login flows this change enables.
- If this is part of a larger refactoring, consider creating a more comprehensive PR that includes all related changes for easier review and testing.
playground/src/api/core/auth.ts (1)
6-7:⚠️ Potential issueConsider the implications of making
passwordandusernameoptional.The changes to the
LoginParamsinterface make bothpasswordandusernameoptional. While this change allows for more flexibility in the login process, it raises some concerns:
- Security: Making the password optional could potentially allow for authentication attempts without a password, which is a security risk.
- Functionality: It's unclear how the login process would work without a username or password. This change might lead to unexpected behavior in the authentication flow.
- Consistency: The PR objectives mention a form reset issue, but these changes seem unrelated to that problem.
To better understand the impact of these changes, let's verify how the
loginApifunction is used throughout the codebase:Additionally, let's check if there are any comments or documentation explaining this change:
Could you provide more context on why these fields are being made optional? If this change is intentional, consider adding comments explaining the rationale and updating the documentation to reflect the new behavior.
Description
fixed #4564
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
disabledOnChangeListenerandemptyStateValue.LoginParamsinterface to allow optionalusernameandpassword.Bug Fixes
Documentation
Chores