feat: add api-select component #5024
Conversation
|
WalkthroughThe changes in this pull request introduce the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/web-antd/src/adapter/component/index.tsOops! Something went wrong! :( ESLint: 9.16.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs 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: 4
🧹 Outside diff range and nitpick comments (6)
packages/effects/common-ui/src/components/api-select/api-select.vue (3)
133-135: Enhance error handling by providing user feedbackCurrently, when an error occurs during the API fetch, it is only logged to the console. Consider notifying the user or emitting an error event to improve the user experience.
Apply this diff to emit an error event:
try { loading.value = true; // fetch logic... } catch (error) { console.warn(error); + emit('error', error); // reset status isFirstLoaded.value = false; } finally {
174-176: Use descriptive variable names in template loopsIn the
v-forloop over$slots, consider using a more descriptive variable name thanitem, such asslotName, to enhance code readability.Apply this diff to rename the variable:
-<template v-for="item in Object.keys($slots)" #[item]="data"> - <slot :name="item" v-bind="data || {}"></slot> +</template v-for="slotName in Object.keys($slots)" #[slotName]="data"> + <slot :name="slotName" v-bind="data || {}"></slot> </template>
81-85: Optimize option processing by usingmapfunctionConsider using the
Array.prototype.mapmethod to transformrefOptionsData, which can make the code more concise and readable.Apply this diff for the refactor:
const getOptions = computed(() => { const { labelField, valueField, numberToString } = props; - const data: OptionsItem[] = []; - const refOptionsData = unref(refOptions); - - for (const next of refOptionsData) { - if (next) { - const value = get(next, valueField); - data.push({ - ...objectOmit(next, [labelField, valueField]), - label: get(next, labelField), - value: numberToString ? `${value}` : value, - }); - } - } + const refOptionsData = unref(refOptions); + const data = refOptionsData + .filter(next => next) + .map(next => { + const value = get(next, valueField); + return { + ...objectOmit(next, [labelField, valueField]), + label: get(next, labelField), + value: numberToString ? `${value}` : value, + }; + }); return data.length > 0 ? data : props.options; });packages/@core/base/shared/src/utils/index.ts (1)
18-19: Verify the necessity of new lodash dependenciesAdding
getandisEqualfromlodashincreases the bundle size. Consider using native JavaScript equivalents or existing utility functions to achieve the same functionality.If lodash functions are necessary, ensure that they are used optimally. Alternatively, you can use:
- For
get: Use optional chaining and nullish coalescing operators.- For
isEqual: UseJSON.stringify(a) === JSON.stringify(b)for simple objects.apps/web-naive/src/adapter/component/index.ts (1)
45-45: UpdateComponentTypedefinition documentationAdd comments or documentation to the
ComponentTypetype to reflect the addition ofApiSelect, enhancing code readability and maintainability.Apply this diff to add a comment:
export type ComponentType = + // Added ApiSelect component for asynchronous option loading | 'ApiSelect' | 'Checkbox' | 'CheckboxGroup'playground/src/adapter/component/index.ts (1)
83-96: Consider adding error handling for the Select componentThe implementation looks correct for Ant Design Vue, but consider adding error handling for cases where the Select component fails to load or render.
ApiSelect: (props, { attrs, slots }) => { + try { return h( ApiSelect, { ...props, ...attrs, component: Select, loadingSlot: 'suffixIcon', modelField: 'value', visibleEvent: 'onVisibleChange', }, slots, ); + } catch (error) { + console.error('Failed to render ApiSelect:', error); + return null; + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
apps/web-antd/src/adapter/component/index.ts(3 hunks)apps/web-ele/src/adapter/component/index.ts(4 hunks)apps/web-naive/src/adapter/component/index.ts(3 hunks)packages/@core/base/shared/package.json(1 hunks)packages/@core/base/shared/src/utils/index.ts(1 hunks)packages/effects/common-ui/src/components/api-select/api-select.vue(1 hunks)packages/effects/common-ui/src/components/api-select/index.ts(1 hunks)packages/effects/common-ui/src/components/index.ts(1 hunks)playground/src/adapter/component/index.ts(3 hunks)playground/src/views/examples/form/basic.vue(2 hunks)pnpm-workspace.yaml(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/effects/common-ui/src/components/api-select/index.ts
🔇 Additional comments (9)
packages/effects/common-ui/src/components/api-select/api-select.vue (1)
94-97: Ensure proper synchronization of model values
When updating the model value, ensure that the change is properly synced with the parent component, especially when using dynamic model fields.
Run the following script to check for correct usage of defineModel and model updates:
✅ Verification successful
Model value synchronization is correctly implemented
The code implements proper model value synchronization:
- Uses
defineModel()to create a two-way binding - Correctly updates the model value through the dynamic
onUpdate:${props.modelField}handler - Properly binds the model value to the child component through computed
bindProps - Handles both default
modelValueand custom model field names viaprops.modelField
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that model updates are correctly implemented.
# Test: Search for incorrect usage of model updates.
ast-grep 'defineModel($$_)' --lang ts
ast-grep 'modelValue.value = $_' --lang ts
Length of output: 337
Script:
#!/bin/bash
# Let's try a different approach to verify model updates in Vue components
# Search for model update patterns in Vue files
rg -U "onUpdate:.*modelValue\.value.*=.*" --type vue -A 2 -B 2
# Search for defineModel usage in Vue files
rg "defineModel" --type vue -A 2 -B 2
# Look for v-model usage in the component
rg "v-model" --type vue -A 2 -B 2
# Check the entire api-select component implementation
fd "api-select.vue" --type f --exec cat {}
Length of output: 4558
packages/effects/common-ui/src/components/index.ts (1)
Line range hint 1-9: Confirm export order and potential conflicts
Ensure that exporting all from './api-select' at the top does not cause any conflicts with other exports in this file or lead to unintended side effects.
Run the following script to check for export conflicts:
✅ Verification successful
Based on the output from the shell scripts, I can now generate the final response:
No export conflicts found in the component exports
The exports in packages/effects/common-ui/src/components/index.ts are safe and do not cause any conflicts because:
- Each module is exported using wildcard exports (
export *) from distinct paths - The named exports from
@vben-core/shadcn-uiare explicitly listed and don't overlap with other exports - The final export
globalShareStateis a named export from a different module
The export order is also appropriate, with local component exports listed first, followed by external module exports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for duplicate exports and verify export order.
# Test: Find duplicate export names.
ast-grep 'export { $$$_ } from $_' --lang ts
ast-grep 'export * from $_' --lang ts
Length of output: 341
Script:
#!/bin/bash
# Let's check the content of each exported module to understand potential conflicts
# First, let's check if the files exist and their content
fd -e ts "api-select|captcha|ellipsis-text|icon-picker|page|resize" -x cat {}
# Also check the content of the index.ts file
cat packages/effects/common-ui/src/components/index.ts
# Check if there are any re-exports of similar names across these files
rg "export.*as" -A 1 -B 1
Length of output: 65822
apps/web-naive/src/adapter/component/index.ts (1)
68-79: Ensure consistent component integration for ApiSelect
Verify that the ApiSelect component integration aligns with how other components are handled in the adapter. Ensure that props and events are correctly mapped.
Run the following script to check for consistent component handling:
✅ Verification successful
The previous ast-grep pattern had syntax issues. Let's try with a corrected pattern and also use ripgrep as a backup to find similar component integrations. This will help us verify if the ApiSelect integration follows the same pattern as other components in the codebase.
Let's try a broader search to find component integrations and also locate the relevant component adapter files. This will help us understand the component integration patterns across the codebase.
The ApiSelect component integration is consistent across UI frameworks
The implementation in apps/web-naive/src/adapter/component/index.ts follows the same pattern used in other UI framework adapters (Antd, Element Plus). All implementations:
- Use the same component wrapper structure with
h(ApiSelect, {...}) - Pass through props and attrs consistently
- Configure the appropriate framework-specific select component
- Set framework-specific properties (e.g.,
modelField: 'value'for Naive UI)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all components follow the same integration pattern.
# Test: Search for component integrations.
ast-grep '$_: (props, { $$ }) => { $$$ }' --lang ts
Length of output: 191
Script:
#!/bin/bash
# Search for component integrations with a corrected ast-grep pattern
ast-grep --pattern '$_: ($_, { $$ }) => { return h($$$) }'
# Also search for similar component definitions using ripgrep
rg "return h\(" -A 3 "component.*\.ts$"
Length of output: 159
Script:
#!/bin/bash
# Search for component integrations using ripgrep without file pattern restriction
rg "return h\(" -A 3
# Find all potential component adapter files
fd "component.*\.ts"
# Search specifically for ApiSelect usage
rg "ApiSelect" -A 5 -B 5
Length of output: 22432
apps/web-ele/src/adapter/component/index.ts (3)
11-11: LGTM: Imports are correctly organized
The imports are properly organized with the new ApiSelect component from @vben/common-ui and ElSelectV2 from element-plus.
Also applies to: 25-25
45-45: LGTM: ComponentType definition is properly updated
The ApiSelect type is correctly added to the union type while maintaining alphabetical order.
67-79: 🛠️ Refactor suggestion
Verify the event name for dropdown visibility
The implementation looks correct, but there might be a discrepancy in the event name. Element Plus typically uses visible-change instead of onDropdownVisibleChange.
Consider updating the event name:
- visibleEvent: 'onDropdownVisibleChange',
+ visibleEvent: 'visible-change',pnpm-workspace.yaml (2)
16-16: Review version pinning strategy for dependencies
Several dependencies are using caret ranges (^). While this allows for minor version updates, it may lead to inconsistent builds across different environments. Consider:
- Using exact versions for critical dependencies
- Adding package-lock file to ensure consistent installations
Also applies to: 25-26, 39-39, 46-47, 53-54, 67-67, 85-85, 92-92, 107-107, 117-118, 119-119, 121-121, 135-135, 140-140, 156-156, 164-164, 172-173, 179-180
117-118: Verify lodash dependency usage
New lodash dependencies (lodash.get and lodash.isequal) have been added, likely to support the ApiSelect component. Ensure these utilities are used efficiently.
✅ Verification successful
Lodash utilities are used appropriately and efficiently
The verification shows that both lodash utilities are used efficiently:
lodash.getis used in the ApiSelect component for safe property access of nested objectslodash.isEqualis used for deep equality comparison in ApiSelect's value change detection- Both utilities are centrally re-exported from
@core/base/shared/src/utils/index.ts, promoting reusability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check usage of lodash utilities
# Search for lodash.get usage
echo "Checking lodash.get usage:"
rg "lodash\.get|import.*from.*lodash\.get" -A 3
# Search for lodash.isequal usage
echo "Checking lodash.isequal usage:"
rg "lodash\.isEqual|import.*from.*lodash\.isequal" -A 3
Length of output: 2761
Script:
#!/bin/bash
# Let's check for actual usage of these lodash utilities in the codebase
# Search for imports and usage of get utility
echo "Searching for get utility usage:"
rg -g '*.{ts,tsx,js,jsx,vue}' "import.*get.*from.*lodash|import.*get.*from.*'@core/base/shared'|get\(" -A 2
# Search for imports and usage of isEqual utility
echo "Searching for isEqual utility usage:"
rg -g '*.{ts,tsx,js,jsx,vue}' "import.*isEqual.*from.*lodash|import.*isEqual.*from.*'@core/base/shared'|isEqual\(" -A 2
# Check if these are re-exported from other files
echo "Checking re-exports:"
rg -g '*.{ts,tsx,js,jsx,vue}' "export.*get.*from|export.*isEqual.*from" -A 2
Length of output: 5554
playground/src/views/examples/form/basic.vue (1)
10-10: Verify API import path
Ensure that the API import path is correct and the API function is properly exported.
✅ Verification successful
API import path is correctly defined and exported
The getAllMenusApi function is properly exported from playground/src/api/core/menu.ts and the import path #/api appears to be an alias that resolves to the API directory. The function implementation is consistent across all variants and returns menu data from the /menu/all endpoint.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if getAllMenusApi is properly exported
# Search for getAllMenusApi definition
echo "Checking getAllMenusApi definition:"
rg "export.*getAllMenusApi" -A 3
# Search for potential circular dependencies
echo "Checking for circular dependencies:"
rg "import.*from.*api.*" --files-with-matches
Length of output: 3306
Description
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
Release Notes
New Features
ApiSelectcomponent for dynamic option selection from an API.ApiSelectfield for fetching and displaying menu options.Updates
ApiSelectacross various adapters.lodashlibrary to enhance data handling.Dependencies