feat(select,tree-select,grid-select,base-select):select component reconstruction #3847
Conversation
WalkthroughThis PR updates many Playwright test selectors to new base-select class names and page-level locators, changes a default dropdown icon fallback, enhances grid-select to auto-trigger remote search on first panel show, adds a select-wrapper renderless module, refactors the Select PC component into a dynamic wrapper, improves tree-select guards, and adds small style tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Wrapper as SelectWrapper
participant Renderless
participant Child as ChildComponent
User->>Wrapper: Mount/select props
Wrapper->>Renderless: renderless.setup(props, context, extendOptions)
Renderless->>Renderless: determine resolvedComponent (Base/Tree/Grid)
Renderless-->>Wrapper: resolvedComponent, mergedProps, listeners
Wrapper->>Child: render dynamic component with mergedProps & listeners
User->>Child: interact (open/select)
Child->>Child: update internal state / emit
Child-->>Wrapper: emit via listeners
Wrapper-->>User: propagate events
sequenceDiagram
participant User
participant GridPanel as GridSelectPanel
participant HandleVis as handleVisibleChange
participant API as RemoteAPI
participant State
User->>GridPanel: open panel (visible=true)
GridPanel->>HandleVis: visible change
HandleVis->>State: check firstAutoSearch && props.remoteConfig?.autoSearch
alt first auto-search needed
HandleVis->>API: api.filter('') %% trigger remote search
API-->>State: results
State->>State: set firstAutoSearch = false
end
HandleVis-->>GridPanel: finished
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (4)
packages/theme/src/option/index.less (1)
95-95: Consider extracting hardcoded padding value to a CSS variable.The padding-top: 3px is hardcoded directly in the stylesheet. For consistency with the component's theming approach (which uses CSS variables extensively throughout), consider defining this value as a reusable variable.
Current approach uses variables for other properties (e.g.,
var(--tv-Option-padding),var(--tv-Option-font-size)), so this pixel value stands out.Example: Define
--tv-Option-label-padding-top: 3px;in the component's CSS variable definitions (likely invars.less) and use it here:.@{option-prefix-cls}-label { display: inline-block; width: 100%; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; vertical-align: top; - padding-top: 3px; + padding-top: var(--tv-Option-label-padding-top); }examples/sites/demos/pc/app/select/copy-multi.spec.ts (1)
39-44: Consider the fragility of page-level text-based locators.The locator strategy has shifted from a local, specific selector (
wrap.locator('.tiny-select').nth(1)) to a page-level text filter (page.locator('div').filter({ hasText: /^北京上海$/ }).first()). While this may work with the current DOM structure, text-based filters are generally more fragile and could match unintended elements if the page structure changes.Consider whether a more specific selector (e.g., combining a class with the text filter, or using a data-testid attribute) would be more robust.
Alternative approach:
- const select = page - .locator('div') - .filter({ hasText: /^北京上海$/ }) - .first() + const select = wrap.locator('.tiny-select').nth(1)Or add a test-specific attribute to the component for more reliable targeting.
examples/sites/demos/pc/app/select/automatic-dropdown.spec.ts (1)
26-26: Note the locator strategy inconsistency.Line 26 now uses
page.getByRole('list').getByText('上海')for clicking the option, while Line 30 still usespage.getByRole('listitem').filter({ hasText: '上海' })for verification. This inconsistency suggests the two locators may target elements differently.Consider aligning both to use the same strategy for consistency, or verify that the different approaches are intentional and both correctly target the desired elements.
- await page.getByRole('list').getByText('上海').click() + await page.getByRole('listitem').filter({ hasText: '上海' }).click()Or update Line 30 to match the new approach if the list-level locator is preferred.
packages/renderless/src/grid-select/vue.ts (1)
32-32: Consider adding explicit typing for the api object.The
anytype removes type safety benefits. If the api structure is stable, consider defining an interface for it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
examples/sites/demos/pc/app/select/automatic-dropdown.spec.ts(1 hunks)examples/sites/demos/pc/app/select/copy-multi.spec.ts(1 hunks)examples/sites/demos/pc/app/select/events.spec.ts(2 hunks)examples/sites/demos/pc/app/select/input-box-type.spec.ts(1 hunks)examples/sites/demos/pc/app/select/nest-grid-remote.spec.ts(5 hunks)examples/sites/demos/pc/app/select/nest-grid.spec.ts(3 hunks)examples/sites/demos/pc/app/select/nest-radio-grid-much-data.spec.ts(1 hunks)examples/sites/demos/pc/app/select/nest-tree.spec.ts(2 hunks)examples/sites/demos/pc/app/select/popup-style-position.spec.ts(1 hunks)examples/sites/demos/pc/app/select/remote-method.spec.ts(1 hunks)packages/renderless/src/base-select/index.ts(1 hunks)packages/renderless/src/grid-select/index.ts(1 hunks)packages/renderless/src/grid-select/vue.ts(3 hunks)packages/renderless/src/select-wrapper/vue.ts(1 hunks)packages/renderless/src/tree-select/vue.ts(2 hunks)packages/theme/src/option/index.less(2 hunks)packages/vue/src/base-select/src/pc.vue(2 hunks)packages/vue/src/select/src/mobile-first.vue(1 hunks)packages/vue/src/select/src/pc.vue(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/renderless/src/base-select/index.ts (1)
examples/sites/src/tools/useTheme.js (1)
designConfig(53-62)
packages/renderless/src/grid-select/vue.ts (1)
packages/renderless/src/grid-select/index.ts (1)
handleVisibleChange(70-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (20)
packages/theme/src/option/index.less (1)
108-110: Verify flex display rule is comprehensive.A new dedicated
&__checkbox-wrap { display: flex; }rule is added here. This ensures the checkbox wrapper uses flex layout.However, confirm whether additional flex properties should be included for alignment, such as
align-items: center;orjustify-content: center;, depending on the intended layout of checkbox content within the wrapper.Please verify:
- Does the checkbox-wrap require only
display: flex;, or should it also include alignment properties?- Are there other select components (tree-select, grid-select) with similar checkbox-wrap styling that should remain consistent with this change?
examples/sites/demos/pc/app/select/nest-grid.spec.ts (1)
11-11: LGTM! Selector updates align with base-select refactoring.The caret selector updates from the legacy naming to
.tiny-base-select__caretare consistent across all three test cases and align with the broader component reconstruction mentioned in the PR objectives.Also applies to: 34-34, 74-74
examples/sites/demos/pc/app/select/popup-style-position.spec.ts (1)
9-9: LGTM! Tags group selector updated correctly.The selector update to
.tiny-base-select__tags-groupaligns with the new base class naming convention.examples/sites/demos/pc/app/select/nest-radio-grid-much-data.spec.ts (1)
11-11: LGTM! Caret selector simplified and updated.The selector update to
.tiny-base-select__caretboth simplifies the locator path and aligns with the new base class naming.examples/sites/demos/pc/app/select/remote-method.spec.ts (1)
31-31: LGTM! Input selector updated for multi-select case.The selector update to
.tiny-base-select__inputaligns with the new base class naming for the multi-select remote search scenario.examples/sites/demos/pc/app/select/nest-tree.spec.ts (1)
11-11: LGTM! Tree select caret selectors updated.The caret selector updates to
.tiny-base-select__caretare consistent with the base-select refactoring and simplify the locator paths.Also applies to: 35-35
packages/vue/src/select/src/mobile-first.vue (1)
454-458: LGTM! Improved class binding structure.The refactor from string concatenation to array form is cleaner and more maintainable. The functionality remains the same—static classes in the first element, conditional classes in the second.
examples/sites/demos/pc/app/select/input-box-type.spec.ts (1)
63-63: LGTM! Selector updates align with component refactoring.The test selectors have been correctly updated to use the new
.tiny-base-select__*class naming convention, consistent with the component architecture changes across this PR.Also applies to: 71-71
packages/vue/src/base-select/src/pc.vue (2)
253-254: LGTM! Defensive checks prevent invalid width calculations.The added guards (
state.inputWidth > 0) prevent potential division-by-zero or negative width issues when the input width hasn't been measured yet, falling back to sensible defaults ('auto' and 'none').
541-541: Minor template cleanup.The
v-elsedirective clarifies the conditional rendering logic for the loading icon.packages/renderless/src/tree-select/vue.ts (1)
18-56: LGTM! Robust normalization for flexible tree data input.The new
resolveTreeDatahelper elegantly handles multiple input formats (array, object with data property, or empty), and the updated watcher correctly tracks reactive changes for both formats. The defensiveArray.isArraycheck ensures type safety.examples/sites/demos/pc/app/select/events.spec.ts (1)
29-30: LGTM! Test selectors updated for refactored component structure.The selector changes from
.tiny-select__caret.tiny-select__closeto.tiny-input__suffix-innercorrectly target the clear button in the new component architecture. The increased wait time (500ms → 1000ms) on line 30 likely accommodates longer rendering cycles in the wrapper-based architecture.Also applies to: 68-68
packages/renderless/src/base-select/index.ts (1)
2010-2010: Icon name updated to standardized convention.The fallback dropdown icon has been updated from
'icon-delta-down'to'icon-down-ward', aligning with the icon naming standardization visible across the component refactor. The icon is properly imported and registered in the component.examples/sites/demos/pc/app/select/nest-grid-remote.spec.ts (2)
12-12: LGTM! Test selectors correctly updated for base-select architecture.All selector changes properly target the new
.tiny-base-select__*class hierarchy, consistent with the component refactoring.Also applies to: 48-48, 75-75, 97-97, 112-114
83-85: I'm unable to access the repository in the sandbox environment due to cloning restrictions. To verify the review comment's concerns, I need your help. Could you please provide:
The full test file content (examples/sites/demos/pc/app/select/nest-grid-remote.spec.ts), or at minimum:
- The complete test case containing lines 83-85
- Any setup/beforeEach blocks
- Related test cases
Or search results from running this in your repository:
# Check for similar select click patterns in remote tests rg -n -C8 "select.*click\(\)|\.click\(\).*await.*page\.waitFor" examples/sites/demos/pc/app/select/ --type=ts # List all select test files to identify related tests fd ".*select.*\.spec\.ts$" examples/sites/demos/pc/app/select/
- Or clarification on:
- Was this click behavior added in this PR or was it pre-existing?
- Are there other multi-select remote tests that follow the same pattern?
- Has the multi-select component API changed recently?
With this information, I can verify whether this is intended behavior or a regression and provide a proper rewritten review comment.
packages/vue/src/select/src/pc.vue (1)
1-316: I'm unable to clone the repository to verify the architectural concerns directly. However, I can provide an assessment based on the code snippet provided and mark what requires manual verification.Major architectural refactor: Requires thorough testing before merge.
This PR substantially rewrites the select component to use a compositional wrapper pattern. The code provided shows legitimate architectural changes, but key concerns require verification that I cannot perform without repository access.
Verified changes from code snippet:
- Template reduced to dynamic component delegation with slot forwarding (lines 2-10)
- Renderless wrapper pattern with runtime component selection
- Props surface expanded from spread to explicit declarations
inheritAttrs: falseenables proper attribute forwarding- Three child components dynamically composed
Concerns requiring verification:
- Backward compatibility: Cannot verify without checking existing component usage patterns and whether
$refsaccess points have changed- Performance impact: Cannot measure without access to performance profiling tools or benchmarks
- Error handling: No visible error boundaries in the template shown; requires validation that child component failures are gracefully handled
- Migration path: Cannot verify impact on existing code accessing select internals without searching the codebase
- Test coverage: Cannot confirm existing tests pass or new tests are added
Critical actions needed:
- Run full test suite, especially integration tests for select component
- Search codebase for direct
$refsaccess to select internals and evaluate migration impact- Check if child component implementations (TinyBaseSelect, TinyTreeSelect, TinyGridSelect) properly handle the renderless wrapper pattern
- Verify the renderless implementation correctly delegates to appropriate child based on props
packages/renderless/src/grid-select/vue.ts (2)
56-57: LGTM!The additions of
isMountedandfirstAutoSearchto the state properly support the new automatic remote search functionality.
72-72: I encountered an issue cloning the repository. Let me request additional context to verify the concern about theinitQueryremoval.To properly verify this concern, I need to see:
- The public API array in
packages/renderless/src/grid-select/index.ts(lines 16-29) - to confirminitQuerywas indeed removed- The implementation to see if
initQueryis still used internally- Any documentation or exports that indicate whether
initQuerywas part of the public API contractCould you provide:
- The content of the public API array section from
index.ts- Whether
initQueryappears anywhere else in the grid-select module- Any git history or commit message explaining why
initQuerywas removed from the public APIAlternatively, if you can access the repository, please run:
# Search for initQuery usage in the entire codebase grep -r "initQuery" packages/ --include="*.ts" --include="*.vue" --include="*.js" # Check the public API exports cat packages/renderless/src/grid-select/index.ts | head -50This will help me determine if this removal is truly a breaking change or if
initQuerywas never part of the intended public API.packages/renderless/src/grid-select/index.ts (2)
86-98: LGTM!The enhanced
checkRowKeysderivation logic provides robust handling for both regular arrays and Vue computed refs, preventing potential runtime errors from unexpected data structures.
70-84: I'm unable to clone the repository to perform automated verification. Let me provide a rewritten review comment based on code analysis and request manual verification where needed.Verify auto-search implementation handles edge cases properly.
The new automatic remote search feature on panel open has several considerations:
Unconventional condition check: The condition
state.firstAutoSearch !== falseis non-standard. Consider usingstate.firstAutoSearch === trueor renaming to a boolean likehasPerformedFirstAutoSearchfor clarity.Empty string query handling: Verify that all
remoteMethodimplementations correctly handle empty string queries (api.filter('')). This should be documented if intentional—empty strings may trigger different filtering logic than expected.Network request timing: This introduces a new network request when the panel first opens. Confirm this behavior is desired and doesn't conflict with existing data-loading patterns or cause duplicate requests in concurrent scenarios.
State mutation: Ensure
state.firstAutoSearch = falseis the intended way to track this state. If this is shared across multiple instances, verify isolation and reset logic on component cleanup/remount.
| export const api = [ | ||
| 'state', | ||
| 'resolvedComponent', | ||
| 'mergedProps', | ||
| 'listeners', | ||
| 'slotNames', | ||
| 'hasScopedDefault', | ||
| 'focus', | ||
| 'blur' | ||
| ] | ||
|
|
||
| export const renderless = (props, { reactive, computed, useAttrs }, { constants, vm, components }) => { | ||
| const api = {} | ||
|
|
||
| const resolvedComponent = computed(() => computedResolvedComponent({ props, constants, vm, components })) | ||
|
|
||
| const mergedProps = computed(() => { | ||
| const runtimeAttrs = typeof useAttrs === 'function' ? useAttrs() : null | ||
| const attrs = runtimeAttrs || vm.$attrs || {} | ||
| const className = attrs.class | ||
| const classArray = Array.isArray(className) | ||
| ? ['tiny-select', ...className] | ||
| : className | ||
| ? ['tiny-select', className] | ||
| : ['tiny-select'] | ||
|
|
||
| const { class: _omitClass, ...restAttrs } = attrs | ||
|
|
||
| return { | ||
| ...props, | ||
| ...restAttrs, | ||
| class: classArray | ||
| } | ||
| }) | ||
|
|
||
| const slotNames = computed(() => Object.keys(vm.$slots || {}).filter((name) => name && name !== 'default')) | ||
|
|
||
| const hasScopedDefault = computed(() => { | ||
| const scoped = vm.$scopedSlots?.default | ||
| if (scoped && scoped.length) { | ||
| return true | ||
| } | ||
| const slot = vm.$slots?.default | ||
| return typeof slot === 'function' && slot.length > 0 | ||
| }) | ||
|
|
||
| const listeners = computed(() => { | ||
| if (vm.$listeners) { | ||
| return vm.$listeners | ||
| } | ||
| return {} | ||
| }) | ||
|
|
||
| const getChildComponent = () => vm.$refs?.childComponent | ||
|
|
||
| // 暴露子组件的 state，让用户可以通过 ref.state 访问子组件的状态（如 cachedOptions） | ||
| // 使用 Proxy 代理子组件的 state，实现动态访问 | ||
| const state = new Proxy( | ||
| {}, | ||
| { | ||
| get(target, prop) { | ||
| const child = getChildComponent() | ||
| return child?.state?.[prop] | ||
| }, | ||
| set(target, prop, value) { | ||
| const child = getChildComponent() | ||
| if (child?.state) { | ||
| child.state[prop] = value | ||
| return true | ||
| } | ||
| return false | ||
| }, | ||
| has(target, prop) { | ||
| const child = getChildComponent() | ||
| return prop in (child?.state || {}) | ||
| }, | ||
| ownKeys(target) { | ||
| const child = getChildComponent() | ||
| return Object.keys(child?.state || {}) | ||
| }, | ||
| getOwnPropertyDescriptor(target, prop) { | ||
| const child = getChildComponent() | ||
| const childState = child?.state || {} | ||
| if (prop in childState) { | ||
| return { | ||
| enumerable: true, | ||
| configurable: true, | ||
| value: childState[prop] | ||
| } | ||
| } | ||
| return undefined | ||
| } | ||
| } | ||
| ) | ||
|
|
||
| const focus = () => { | ||
| const child = getChildComponent() | ||
| child && typeof child.focus === 'function' && child.focus() | ||
| } | ||
|
|
||
| const blur = () => { | ||
| const child = getChildComponent() | ||
| child && typeof child.blur === 'function' && child.blur() | ||
| } | ||
|
|
||
| const hasData = (value: any) => { | ||
| if (!value) { | ||
| return false | ||
| } | ||
| if (Array.isArray(value)) { | ||
| return value.length > 0 | ||
| } | ||
| if (typeof value === 'object') { | ||
| return Object.keys(value).length > 0 | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| const computedResolvedComponent = ({ props, constants, vm, components }) => { | ||
| // 优先使用传入的 components，否则从 vm.$options.components 获取 | ||
| const comps = components || vm.$options?.components || {} | ||
|
|
||
| if (props.renderType === constants.TYPE.Tree || hasData(props.treeOp)) { | ||
| return comps.TinyTreeSelect || 'TinyTreeSelect' | ||
| } | ||
| if (props.renderType === constants.TYPE.Grid || hasData(props.gridOp)) { | ||
| return comps.TinyGridSelect || 'TinyGridSelect' | ||
| } | ||
| return comps.TinyBaseSelect || 'TinyBaseSelect' | ||
| } | ||
| Object.assign(api, { | ||
| state, | ||
| resolvedComponent, | ||
| mergedProps, | ||
| listeners, | ||
| slotNames, | ||
| hasScopedDefault, | ||
| focus, | ||
| blur | ||
| }) | ||
|
|
||
| return api | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Check gridOp/treeOp usage to see if 0 or false are valid values
rg -n "gridOp\s*=|treeOp\s*=" --type=vue --type=ts -A 2 -B 2 packages/vue/src/select/ packages/renderless/src/select-wrapper/ 2>/dev/null | head -50Repository: opentiny/tiny-vue
<shell_result>
Repository: opentiny/tiny-vue
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository opentiny/tiny-vue: Clone operation failed
</shell_result>
Script executed:
#!/bin/bash
# Also search more broadly for gridOp and treeOp assignments
rg -n "gridOp|treeOp" --type=vue --type=ts -C 2 packages/ | grep -E "(gridOp|treeOp)\s*[:=]" | head -30Repository: opentiny/tiny-vue
<shell_result>
Repository: opentiny/tiny-vue
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository opentiny/tiny-vue: Clone operation failed
</shell_result>
Script executed:
#!/bin/bash
# Check if there are tests for the select-wrapper
fd -e "spec\|test" packages/renderless/src/select-wrapper/ -o packages/vue/src/select/ 2>/dev/null | head -20Repository: opentiny/tiny-vue
<shell_result>
Repository: opentiny/tiny-vue
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository opentiny/tiny-vue: Clone operation failed
</shell_result>
Script executed:
#!/bin/bash
# Look at the actual child components to understand if state property is guaranteed
rg -n "state\s*=" packages/renderless/src/ --type=ts -A 1 -B 1 | grep -E "(TinyTreeSelect|TinyGridSelect|TinyBaseSelect)" | head -20Repository: opentiny/tiny-vue
<shell_result>
Repository: opentiny/tiny-vue
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository opentiny/tiny-vue: Clone operation failed
</shell_result>
Script executed:
#!/bin/bash
# Check Vue class handling - whether duplicate classes are an issue
rg -n "class.*tiny-select" packages/ --type=vue --type=ts -C 1 | head -30Repository: opentiny/tiny-vue
<shell_result>
Repository: opentiny/tiny-vue
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository opentiny/tiny-vue: Clone operation failed
</shell_result>
I'm unable to access the repository to run verification scripts due to a clone failure. However, I can provide a rewritten review based on static code analysis of the provided snippet.
Address three edge cases in renderless logic to improve robustness.
This wrapper elegantly delegates to dynamically selected child components, but has three correctness gaps:
Critical:
- Lines 106-117 (
hasData): Treats0andfalseas falsy returns, incorrectly triggering fallback behavior if gridOp/treeOp are legitimately0orfalse. Refactor to explicitly check forundefined/null:const hasData = (value: any) => { return value != null && (Array.isArray(value) ? value.length > 0 : typeof value === 'object' ? Object.keys(value).length > 0 : true) }
Important:
-
Lines 58-94 (State Proxy): Silently fails when child component lacks
stateproperty; no warning in development. All proxy traps returnundefinedorfalseinstead of indicating a misconfiguration. Add a guard and warning on first access. -
Lines 22-25 (Class merging): Always prepends
'tiny-select'even if present in input className, creating duplicates. UseSetor check before adding:const classArray = Array.from(new Set(['tiny-select', ...(Array.isArray(className) ? className : className ? [className] : [])]))
🤖 Prompt for AI Agents
In packages/renderless/src/select-wrapper/vue.ts around lines 22-25, 58-94 and
106-117: fix three robustness issues — (1) Class merging: avoid duplicate
"tiny-select" by normalizing incoming className to an array and only prepending
"tiny-select" when it's not already present (or use a Set) so duplicates are not
introduced. (2) State Proxy: add a guard that detects when the child component
has no state and, on first proxy access, emit a development-mode console.warn
(or use vm.$warn) describing the misconfiguration; ensure proxy traps return
safe defaults after the warning rather than silently failing. (3) hasData:
change the falsy check to explicitly treat only null/undefined as absent (i.e.,
return false if value is null/undefined), keep existing checks for
arrays/objects for emptiness, and allow legitimate values like 0 or false to be
treated as present.
| InputBoxType: { | ||
| type: String, | ||
| default: 'input', | ||
| validator: (value: string) => ['input', 'underline'].includes(value) |
There was a problem hiding this comment.
Typo: Prop name inconsistency.
Line 266 defines InputBoxType (capital I) but the rest of the codebase likely uses inputBoxType (lowercase). This could cause props not to bind correctly.
Apply this fix:
- InputBoxType: {
+ inputBoxType: {
type: String,
default: 'input',
validator: (value: string) => ['input', 'underline'].includes(value)
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| InputBoxType: { | |
| type: String, | |
| default: 'input', | |
| validator: (value: string) => ['input', 'underline'].includes(value) | |
| inputBoxType: { | |
| type: String, | |
| default: 'input', | |
| validator: (value: string) => ['input', 'underline'].includes(value) | |
| }, |
🤖 Prompt for AI Agents
In packages/vue/src/select/src/pc.vue around lines 266 to 269, the prop is
declared as InputBoxType (capital I) but the codebase expects inputBoxType
(camelCase); rename the prop declaration to inputBoxType, keep the type, default
and validator unchanged, and update any local references/usages in the
template/script to use inputBoxType so the prop binds correctly.
discreted66
force-pushed
the
select1124
branch
from
8b63954 to
29bfe1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/renderless/src/select-wrapper/vue.ts (1)
17-33: Tighten class merging, state proxy guard, andhasDatasemantics (non-blocking).These are minor robustness tweaks on otherwise solid wrapper logic:
Class merging:
classArrayalways prepends'tiny-select'and then appends any runtimeclass. If callers already include'tiny-select', you’ll get duplicates. Not harmful, but you can easily de‑dup:const classArray = Array.from( new Set([ 'tiny-select', ...(Array.isArray(className) ? className : className ? [className] : []) ]) )State Proxy guard: When the child lacks a
statefield, all traps silently returnundefined/false. Emitting a one-time dev warning in that case (e.g., the first timegetruns with nochild.state) would make misconfiguration easier to spot without changing behavior in production.
hasDataand falsy values:hasDatacurrently treats all falsy primitives as “no data”, so a future use of0/falseas a meaningful config forgridOp/treeOpwould be ignored. If you want to be future-proof while still treatingnull/undefinedas empty, you could adjust it to:const hasData = (value: any) => { if (value == null) return false if (Array.isArray(value)) return value.length > 0 if (typeof value === 'object') return Object.keys(value).length > 0 return true }All three are nice-to-have improvements and not blockers.
To double-check whether
gridOp/treeOpmight legitimately be0/falseanywhere, you can run a quick search in your repo:#!/bin/bash rg -n "gridOp|treeOp" packages --type=ts --type=vue -C2 || trueAlso applies to: 58-94, 106-130
packages/vue/src/select/src/pc.vue (1)
262-270: Prop name casing forInputBoxTypeis likely wrong and may break binding.Here the prop is declared as
InputBoxType, but templates typically useinput-box-type, which Vue normalizes toinputBoxType. That may not match this capitalized prop name, and it’s also inconsistent with the rest of the props.To avoid subtle binding issues, I’d rename the prop to
inputBoxType:- InputBoxType: { + inputBoxType: { type: String, default: 'input', validator: (value: string) => ['input', 'underline'].includes(value) },Please also adjust any internal references (including in base-select/tree-select/grid-select, demos, and docs) to use
inputBoxTypeconsistently.You can quickly confirm which casing is used elsewhere with:
#!/bin/bash rg -n "inputBoxType|input-box-type|InputBoxType" packages -C2 || true
🧹 Nitpick comments (6)
packages/vue/src/select/src/mobile-first.vue (1)
454-457: LGTM! Array syntax is correct.The change from spread arguments to array syntax for the
m()helper is functionally equivalent and valid. The conditional class logic correctly applies brand colors whenstate.selectCls !== 'check'.Minor observation: The file uses mixed syntax for
m()calls—some with arrays (here and line 490), others with spread arguments (line 308). Consider standardizing the pattern in future refactors for consistency.examples/sites/demos/pc/app/select/nest-grid-remote.spec.ts (4)
44-51: Consider narrowing the caret locator with.first()for stability
select.locator('.tiny-base-select__caret')may match multiple elements if more carets or similar icons are ever introduced under this select. Using.first()(as in other tests) would keep the intent clear and maintain previous behavior of asserting a single primary caret.
74-96: Multi-select: selectors look good; consider updating caret class and avoiding fixed timeouts
- The switch to
select.locator('.tiny-base-select__input').first()plus an initialselect.click()makes sense with the new base-select structure and should reduce flakiness around input visibility.- This block still uses the old
.tiny-select__caretclass at Line 77. If the caret class has been fully migrated to.tiny-base-select__caretelsewhere, this is likely an oversight and could break when the old class is removed.- The added
waitForTimeout(200)(and later 1000ms waits) works but is brittle. When convenient, consider replacing with expectations such asawait expect(dropdown).toBeVisible()orawait expect(input).toBeVisible()to make the test less timing-sensitive.
96-103: Scope tag locator to the current select to avoid cross-select interference
const tags = page.locator('.tiny-base-select .tiny-tag')is global to the page and could accidentally pick up tags from other selects if more are added to the demo. Scoping to the currentselectwould make the assertion more robust, e.g.:const tags = select.locator('.tiny-base-select .tiny-tag') // or if .tiny-base-select wraps this select: // const tags = select.locator('.tiny-tag')
110-138: Fourth test selectors align with new base-select API; optional.first()on inputThe use of
.tiny-base-select__inputand.tiny-base-select__caret.first() is consistent with the refactor and the test flow looks sound. If this variant can ever render multiple base-select inputs, you might optionally mirror the.first()usage oninputfor clarity, but it’s not strictly necessary.examples/sites/demos/pc/app/select/automatic-dropdown.spec.ts (1)
24-30: Scope the option click to the dropdown for more robust tests.Using
page.getByRole('list').getByText('上海')is page-wide and could target the wrong list if the page gains other lists containing the same text. Consider scoping through the dropdown locator instead:await dropdown.getByRole('list').getByText('上海').click()This keeps the test coupled only to the select’s dropdown DOM.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
examples/sites/demos/pc/app/select/automatic-dropdown.spec.ts(1 hunks)examples/sites/demos/pc/app/select/copy-multi.spec.ts(1 hunks)examples/sites/demos/pc/app/select/events.spec.ts(2 hunks)examples/sites/demos/pc/app/select/input-box-type.spec.ts(1 hunks)examples/sites/demos/pc/app/select/nest-grid-remote.spec.ts(5 hunks)examples/sites/demos/pc/app/select/nest-grid.spec.ts(3 hunks)examples/sites/demos/pc/app/select/nest-radio-grid-much-data.spec.ts(1 hunks)examples/sites/demos/pc/app/select/nest-tree.spec.ts(2 hunks)examples/sites/demos/pc/app/select/popup-style-position.spec.ts(1 hunks)examples/sites/demos/pc/app/select/remote-method.spec.ts(1 hunks)packages/renderless/src/base-select/index.ts(1 hunks)packages/renderless/src/grid-select/index.ts(1 hunks)packages/renderless/src/grid-select/vue.ts(3 hunks)packages/renderless/src/select-wrapper/vue.ts(1 hunks)packages/renderless/src/tree-select/index.ts(3 hunks)packages/renderless/src/tree-select/vue.ts(2 hunks)packages/theme/src/option/index.less(2 hunks)packages/vue/src/base-select/src/pc.vue(2 hunks)packages/vue/src/select/src/mobile-first.vue(1 hunks)packages/vue/src/select/src/pc.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- examples/sites/demos/pc/app/select/nest-tree.spec.ts
- examples/sites/demos/pc/app/select/events.spec.ts
- examples/sites/demos/pc/app/select/remote-method.spec.ts
- packages/renderless/src/tree-select/vue.ts
- examples/sites/demos/pc/app/select/popup-style-position.spec.ts
- examples/sites/demos/pc/app/select/copy-multi.spec.ts
- packages/theme/src/option/index.less
- packages/vue/src/base-select/src/pc.vue
- packages/renderless/src/grid-select/vue.ts
- examples/sites/demos/pc/app/select/nest-radio-grid-much-data.spec.ts
- packages/renderless/src/base-select/index.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/renderless/src/select-wrapper/vue.ts (4)
packages/renderless/src/grid-select/vue.ts (2)
api(16-29)renderless(31-139)packages/renderless/src/tree-select/vue.ts (2)
api(13-13)renderless(15-77)packages/vue-common/src/index.ts (1)
props(55-73)packages/renderless/src/base-select/index.ts (2)
focus(442-448)blur(450-455)
packages/renderless/src/grid-select/index.ts (1)
packages/renderless/src/grid-select/vue.ts (1)
api(16-29)
packages/renderless/src/tree-select/index.ts (2)
packages/renderless/src/grid-select/vue.ts (1)
api(16-29)packages/renderless/src/tree-select/vue.ts (1)
api(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (7)
examples/sites/demos/pc/app/select/nest-grid-remote.spec.ts (1)
8-15: Confirm caret DOM location for the basic single-select caseHere
suffixSvgis now resolved fromdropdown.locator('.tiny-base-select__caret'), whereas other tests resolve the caret from the.tiny-selectroot. If.tiny-base-select__caretactually lives in the base-select wrapper rather than inside the dropdown panel, this assertion may become a no-op (element never found / always “hidden”). Please double-check the DOM for this variant and align the root (selectvsdropdown) with where the caret truly sits.packages/renderless/src/tree-select/index.ts (1)
159-204: Lazy-load guards in mounted/watchValue look good.The added checks around
state.modelValueandoption && option.length > 0, plus the early returns wheninitialNodes/dataare empty, make the multiple and single branches safer in lazy-load/remote scenarios and prevent updatingbaseSelectwith empty selections. If you ever allow0/falseas valid model values, you may eventually want to relax the!state.modelValuecheck tostate.modelValue == null, but as-is this is consistent with prior behavior.Also applies to: 233-239
packages/renderless/src/grid-select/index.ts (2)
70-84: Auto remote-search on first open is wired safely.Extending
handleVisibleChangeto takepropsand gatingapi.filter('')behindprops.remote && props.remoteConfig?.autoSearch && state.firstAutoSearch !== false(plusstate.isMounted) keeps the first-auto-search behavior predictable and ensures it only fires once per instance.
86-98: checkRowKeys normalization in buildSelectConfig improves robustness.Normalizing
state.gridCheckedDataintocheckRowKeys(array, orraw.valuearray, else[]) makes the select–grid wiring tolerant of both raw arrays and ref-like shapes, and avoids leakingundefined/object wrappers into the table config.examples/sites/demos/pc/app/select/nest-grid.spec.ts (1)
11-12: Caret selectors updated consistently to the new base-select class.Switching the caret locator to
.tiny-base-select__caretacross single/multiple/searchable cases keeps these specs aligned with the new DOM structure while still scoping via the.tiny-selectwrapper.Also applies to: 34-35, 74-75
examples/sites/demos/pc/app/select/input-box-type.spec.ts (1)
58-64: Assertions now match base-select caret and multiple-mode classes.Using
.tiny-base-select__caretfor the icon color assertion and.tiny-base-select__multiplefor the multi-select class keeps this spec in sync with the refactored select DOM.Also applies to: 70-72
packages/vue/src/select/src/pc.vue (1)
2-10: Dynamic wrapper composition for select variants looks clean and compatible.Delegating to
<component ref="childComponent" :is="resolvedComponent" v-bind="mergedProps" v-on="listeners">with slot/slotProps forwarding andextendOptions: { components }keepsTinyBaseSelect,TinyTreeSelect, andTinyGridSelectbehind a single public<tiny-select>surface while preserving listeners and slots. Combined with the renderless wrapper, this should make variant switching and future extension much easier to maintain.Also applies to: 28-36, 306-314
1 participant
PR
feat:select组件重构
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.