fix(Select): support item-aligned position mode#6358
Conversation
📝 WalkthroughWalkthroughAdds a Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/content/docs/2.components/select.md (1)
226-255:⚠️ Potential issue | 🟡 MinorThe example conflates
item-alignedwith popper-mode options.Reka UI's Select component treats
position="item-aligned"andposition="popper"as distinct modes. Theitem-alignedmode has "limited side control," whileside,align, andsideOffsetare controls specific toposition="popper". The current example listssideas an available option despite settingposition: item-aligned, which is misleading. Switch this snippet toposition="popper"or removesidefrom the options when demonstratingitem-aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/2.components/select.md` around lines 226 - 255, The example incorrectly mixes item-aligned behavior with popper-specific controls: update the example's content prop so it uses position: popper (or alternatively remove popper-only props like content.side/content.sideOffset from the item-aligned example); specifically modify the YAML block that defines items/content.position and the content props (look for content.position, content.align, content.side and the props: content section) so that either position is changed to "popper" when listing side/sideOffset options or remove side/sideOffset from the item-aligned variant to avoid the misleading combination.src/runtime/components/Select.vue (1)
348-405:⚠️ Potential issue | 🟠 Major
viewportRefis exposed inconsistently depending on content position.When
position === 'item-aligned', the template switches to<SelectViewport>(a Vue component), causing the exposedviewportRefto be a component instance instead of an HTMLElement. This breaks the public API contract with consumers expectingRef<HTMLDivElement | null>. Normalize by extracting$elfrom component instances, matching the pattern already used fortriggerRef:Suggested change
defineExpose({ triggerRef: toRef(() => triggerRef.value?.$el as HTMLButtonElement), - viewportRef: toRef(() => viewportRef.value) + viewportRef: toRef(() => { + const el = viewportRef.value as { $el?: HTMLDivElement } | HTMLDivElement | null + return (el && typeof el === 'object' && '$el' in el ? el.$el : el) as HTMLDivElement | null + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Select.vue` around lines 348 - 405, The exposed viewportRef becomes a component instance when position === 'item-aligned' because the template uses <SelectViewport>; update the logic that exposes viewportRef (same place triggerRef is normalized) to unwrap component instances by assigning viewportRef.value = (instance && '$el' in instance) ? instance.$el as HTMLElement : instance as HTMLElement | null so consumers always get HTML element (reference SelectViewport, viewportRef and the normalization approach used for triggerRef).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/SelectMenu.vue`:
- Line 622: The DOM attribute data-position on the ComboboxContent is hardcoded
to "popper" while contentProps may carry a different position; update the
ComboboxContent usage (in SelectMenu.vue) to bind data-position dynamically from
contentProps.position (similar to Select.vue) so the attribute reflects the
actual component state when spreading contentProps; ensure you keep the existing
:class="ui.content({ class: uiProp?.content })" and v-bind="contentProps" but
replace the static data-position with a dynamic binding to
contentProps.position.
---
Outside diff comments:
In `@docs/content/docs/2.components/select.md`:
- Around line 226-255: The example incorrectly mixes item-aligned behavior with
popper-specific controls: update the example's content prop so it uses position:
popper (or alternatively remove popper-only props like
content.side/content.sideOffset from the item-aligned example); specifically
modify the YAML block that defines items/content.position and the content props
(look for content.position, content.align, content.side and the props: content
section) so that either position is changed to "popper" when listing
side/sideOffset options or remove side/sideOffset from the item-aligned variant
to avoid the misleading combination.
In `@src/runtime/components/Select.vue`:
- Around line 348-405: The exposed viewportRef becomes a component instance when
position === 'item-aligned' because the template uses <SelectViewport>; update
the logic that exposes viewportRef (same place triggerRef is normalized) to
unwrap component instances by assigning viewportRef.value = (instance && '$el'
in instance) ? instance.$el as HTMLElement : instance as HTMLElement | null so
consumers always get HTML element (reference SelectViewport, viewportRef and the
normalization approach used for triggerRef).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 711959df-443a-4132-acdc-746ab2a21670
⛔ Files ignored due to path filters (4)
test/components/__snapshots__/Select-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Select.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/SelectMenu-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/SelectMenu.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
docs/content/docs/2.components/select.mdsrc/runtime/components/Select.vuesrc/runtime/components/SelectMenu.vuesrc/theme/select.ts
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/runtime/components/Select.vue (2)
333-333: Optional: replace the single-elementv-forwith a computed.Using
v-forover[displayValue(modelValue)]just to create a template-scopeddisplayedModelValueworks, but it's an unusual idiom and:keywill beundefinedwhen the display value is nullish (Vue will issue a dev warning). A plaincomputed(e.g.const displayedModelValue = computed(() => displayValue(modelValue ...))) or a<script setup>helper would be clearer and avoid the key-on-undefined case. SincemodelValuehere comes fromSelectRoot's default slot, if you prefer to keep it inline, dropping the:keywould be enough to silence the warning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Select.vue` at line 333, The template uses a single-element v-for to create displayedModelValue which causes an unnecessary pattern and a potential undefined :key warning; instead, add a computed displayedModelValue (e.g. in the component's script or <script setup> compute displayedModelValue = computed(() => displayValue(modelValue as any))) and replace the v-for template with a normal binding to that computed, or if you want the minimal change simply remove the :key from the template line; update references to use displayedModelValue and keep displayValue and modelValue from SelectRoot as before.
184-188: Optional: drop theas anycast ondefaultVariantslookup.The fallback chain is correct, but
(appConfig.ui?.select as any)?.defaultVariants?.positionforfeits type safety. SinceSelect(theComponentConfigalias) already models the theme's variants, consider narrowing via theSelect['variants']['position']union rather thanany, or declare a small local type so misuse surfaces at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Select.vue` around lines 184 - 188, The lookup for the default position uses an `as any` cast which loses type safety; update the expression that computes `position` (the computed `position` and the `contentProps` toRef usage) to narrow the type of `appConfig.ui?.select?.defaultVariants` instead of using `any` — e.g. declare a local type alias matching the Select component variants (or use the existing `Select['variants']['position']` / `ComponentConfig` alias) and use that type when accessing `.defaultVariants?.position` so the fallback chain remains the same but the compiler catches invalid variant names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/Select.vue`:
- Line 339: The template currently uses the literal string ' ' which Vue
will HTML-escape and render as the text " "; update the expression so it
returns an actual non-breaking space character instead (use the Unicode
codepoint, e.g. '\u00A0') so the placeholder preserves layout. Replace the
interpolated fallback in the Select.vue template (the expression using
displayedModelValue and placeholder) to use the actual NBSP (placeholder ??
'\u00A0') so displayedModelValue ?? (placeholder ?? '\u00A0') renders a real
non-breaking space when both are nullish.
---
Nitpick comments:
In `@src/runtime/components/Select.vue`:
- Line 333: The template uses a single-element v-for to create
displayedModelValue which causes an unnecessary pattern and a potential
undefined :key warning; instead, add a computed displayedModelValue (e.g. in the
component's script or <script setup> compute displayedModelValue = computed(()
=> displayValue(modelValue as any))) and replace the v-for template with a
normal binding to that computed, or if you want the minimal change simply remove
the :key from the template line; update references to use displayedModelValue
and keep displayValue and modelValue from SelectRoot as before.
- Around line 184-188: The lookup for the default position uses an `as any` cast
which loses type safety; update the expression that computes `position` (the
computed `position` and the `contentProps` toRef usage) to narrow the type of
`appConfig.ui?.select?.defaultVariants` instead of using `any` — e.g. declare a
local type alias matching the Select component variants (or use the existing
`Select['variants']['position']` / `ComponentConfig` alias) and use that type
when accessing `.defaultVariants?.position` so the fallback chain remains the
same but the compiler catches invalid variant names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa15c55a-8136-4d7d-8223-186684ee4347
⛔ Files ignored due to path filters (4)
test/components/__snapshots__/Select-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Select.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/SelectMenu-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/SelectMenu.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
.github/contributing/documentation.mdAGENTS.mddocs/content/docs/2.components/select.mdplaygrounds/nuxt/app/pages/components/select.vuesrc/runtime/components/Select.vuesrc/theme/select.ts
✅ Files skipped from review due to trivial changes (2)
- playgrounds/nuxt/app/pages/components/select.vue
- docs/content/docs/2.components/select.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/theme/select.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/runtime/components/Select.vue (1)
339-339:⚠️ Potential issue | 🟠 Major
' 'still renders as literal text.Vue's
{{ }}interpolation HTML-escapes output, so this will render the literal 6-character string rather than a non-breaking space when bothdisplayedModelValueandplaceholderare nullish. Use'\u00A0'instead.🐛 Proposed fix
- {{ displayedModelValue ?? (placeholder ?? ' ') }} + {{ displayedModelValue ?? (placeholder ?? '\u00A0') }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Select.vue` at line 339, The template currently uses the literal string ' ' inside interpolation which Vue escapes and renders as the six-character sequence; update the interpolation expression in Select.vue so when both displayedModelValue and placeholder are nullish it returns a real non-breaking space (use '\u00A0') instead of ' '; locate the template expression that references displayedModelValue and placeholder and replace the fallback value accordingly so the rendered output contains a true NBSP.
🧹 Nitpick comments (1)
src/runtime/components/Select.vue (1)
295-303:viewportRefunwrapping looks correct, but consider typing the template ref.The
'$el' in instancecheck correctly handles both branches of<component :is>: whenSelectViewportis rendered the ref holds a component instance (unwrap$el), and when the plaindivis rendered the ref already holds theHTMLElement. One small improvement — typinguseTemplateRef<HTMLElement | ComponentPublicInstance>('viewportRef')would remove the need for the runtime shape check and make the intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Select.vue` around lines 295 - 303, The template ref viewportRef should be explicitly typed to HTMLElement | ComponentPublicInstance to avoid the runtime "$el" shape check; update the useTemplateRef call to useTemplateRef<HTMLElement | ComponentPublicInstance>('viewportRef') and then simplify the defineExpose viewportRef to just return viewportRef.value (or a toRef wrapper) so the unwrap logic and "$el" in instance check can be removed; keep triggerRef exposure unchanged (triggerRef in defineExpose) and ensure imports/types for ComponentPublicInstance are present if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/Select.vue`:
- Line 356: The Select component currently conditionally renders SelectViewport
based on isItemAligned which breaks the required scrolling container when
position='popper'; replace the conditional <component :is="isItemAligned ?
SelectViewport : 'div'"> usage with an unconditional SelectViewport element
using the existing ref="viewportRef" and props (e.g., :class="ui.viewport({
class: uiProp?.viewport })") so the scrolling container is always present; leave
the positioning control to SelectContent's position prop and remove any logic
that swaps to a plain div around SelectViewport or relies on isItemAligned for
structural rendering.
---
Duplicate comments:
In `@src/runtime/components/Select.vue`:
- Line 339: The template currently uses the literal string ' ' inside
interpolation which Vue escapes and renders as the six-character sequence;
update the interpolation expression in Select.vue so when both
displayedModelValue and placeholder are nullish it returns a real non-breaking
space (use '\u00A0') instead of ' '; locate the template expression that
references displayedModelValue and placeholder and replace the fallback value
accordingly so the rendered output contains a true NBSP.
---
Nitpick comments:
In `@src/runtime/components/Select.vue`:
- Around line 295-303: The template ref viewportRef should be explicitly typed
to HTMLElement | ComponentPublicInstance to avoid the runtime "$el" shape check;
update the useTemplateRef call to useTemplateRef<HTMLElement |
ComponentPublicInstance>('viewportRef') and then simplify the defineExpose
viewportRef to just return viewportRef.value (or a toRef wrapper) so the unwrap
logic and "$el" in instance check can be removed; keep triggerRef exposure
unchanged (triggerRef in defineExpose) and ensure imports/types for
ComponentPublicInstance are present if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10d705a6-547c-4c48-a195-e62f2419ac0c
📒 Files selected for processing (1)
src/runtime/components/Select.vue
1 participant
🔗 Linked issue
❓ Type of change
📚 Description
Fix
position: 'item-aligned'onUSelectby wrapping the trigger value inSelectValueand the viewport inSelectViewport(which Reka UI requires for its layout measurement), plus scope the popper-only open/close animations todata-[position=popper]:in the theme.📝 Checklist