refactor(editor): Add Select component #20963
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on November 5. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
BundleMonUnchanged files (2)
No change in files bundle size Groups updated (2)
Final result: ✅ View report in BundleMon website ➡️ |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 8 files
Prompt for AI agents (all 6 issues)
Understand the root cause of the following 6 issues and fix them.
<file name="packages/frontend/@n8n/design-system/src/v2/components/Select/Select.test.ts">
<violation number="1" location="packages/frontend/@n8n/design-system/src/v2/components/Select/Select.test.ts:405">
The test labelled for the `item-label` slot still registers the `item-trailing` slot, so it just duplicates the trailing-slot check and leaves the label slot untested. Please target the `item-label` slot here so coverage matches the description.</violation>
</file>
<file name="packages/frontend/@n8n/design-system/src/v2/components/Select/Select.vue">
<violation number="1" location="packages/frontend/@n8n/design-system/src/v2/components/Select/Select.vue:141">
Rule violated: **Prefer Typeguards over Type casting**
Please remove the `as string` cast on `props.labelKey`. The prop is typed as `GetItemKeys<T> | undefined`, so this assertion hides the undefined case—callers can still omit the prop, which would make `get` receive `undefined` and throw at runtime. Adjust the typing (e.g., derive a default key or refine the prop type) so the compiler knows the value is a string without a cast, in line with our “Prefer type guards over type casting” rule.</violation>
<violation number="2" location="packages/frontend/@n8n/design-system/src/v2/components/Select/Select.vue:172">
The selection indicator ignores the new `selectedIcon` prop, so passing a custom icon has no effect. Please bind the indicator to the prop instead of hard-coding "check".</violation>
</file>
<file name="packages/frontend/@n8n/design-system/src/__tests__/setup.ts">
<violation number="1" location="packages/frontend/@n8n/design-system/src/__tests__/setup.ts:30">
Rule violated: **Prefer Typeguards over Type casting**
Avoid narrowing with `as` here; annotate the variable instead so the optional pointer APIs are reflected without a cast. The "Prefer Typeguards over Type casting" rule forbids this assertion scenario.</violation>
<violation number="2" location="packages/frontend/@n8n/design-system/src/__tests__/setup.ts:63">
Overriding MouseEvent.defaultPrevented to always return false hides preventDefault() effects, so tests can’t detect default-preventing logic.</violation>
</file>
<file name="packages/frontend/@n8n/design-system/src/v2/components/Select/Select.stories.ts">
<violation number="1" location="packages/frontend/@n8n/design-system/src/v2/components/Select/Select.stories.ts:134">
Rule violated: **Prefer Typeguards over Type casting**
Remove the `(args.items as SelectItem[])` assertion and either type the story args or guard the runtime value so you don't bypass the SelectItem contract; this cast currently suppresses TS2322 and violates the "Prefer Typeguards over Type casting" rule.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| defaultOpen: true, | ||
| }, | ||
| slots: { | ||
| 'item-trailing': '<span data-test-id="custom-trailing">any</span>', |
There was a problem hiding this comment.
The test labelled for the item-label slot still registers the item-trailing slot, so it just duplicates the trailing-slot check and leaves the label slot untested. Please target the item-label slot here so coverage matches the description.
Prompt for AI agents
Address the following comment on packages/frontend/@n8n/design-system/src/v2/components/Select/Select.test.ts at line 405:
<comment>The test labelled for the `item-label` slot still registers the `item-trailing` slot, so it just duplicates the trailing-slot check and leaves the label slot untested. Please target the `item-label` slot here so coverage matches the description.</comment>
<file context>
@@ -0,0 +1,471 @@
+ defaultOpen: true,
+ },
+ slots: {
+ 'item-trailing': '<span data-test-id="custom-trailing">any</span>',
+ },
+ });
</file context>
✅ Addressed in 4d1eca3
| <slot name="item-trailing" :item="item as NestedItem<T>" :index="index" /> | ||
| <SelectItemIndicator as-child> | ||
| <Icon icon="check" /> |
There was a problem hiding this comment.
The selection indicator ignores the new selectedIcon prop, so passing a custom icon has no effect. Please bind the indicator to the prop instead of hard-coding "check".
Prompt for AI agents
Address the following comment on packages/frontend/@n8n/design-system/src/v2/components/Select/Select.vue at line 172:
<comment>The selection indicator ignores the new `selectedIcon` prop, so passing a custom icon has no effect. Please bind the indicator to the prop instead of hard-coding "check".</comment>
<file context>
@@ -0,0 +1,361 @@
+ <slot name="item-trailing" :item="item as NestedItem<T>" :index="index" />
+
+ <SelectItemIndicator as-child>
+ <Icon icon="check" />
+ </SelectItemIndicator>
+ </span>
</file context>
| <Icon icon="check" /> | |
| <Icon :icon="selectedIcon" /> |
| constructor(type: string, eventInit?: MouseEventInit) { | ||
| super(type, eventInit); | ||
| Object.defineProperty(this, 'defaultPrevented', { | ||
| get: () => false, |
There was a problem hiding this comment.
Overriding MouseEvent.defaultPrevented to always return false hides preventDefault() effects, so tests can’t detect default-preventing logic.
Prompt for AI agents
Address the following comment on packages/frontend/@n8n/design-system/src/__tests__/setup.ts at line 63:
<comment>Overriding MouseEvent.defaultPrevented to always return false hides preventDefault() effects, so tests can’t detect default-preventing logic.</comment>
<file context>
@@ -20,3 +21,60 @@ window.ResizeObserver =
+ constructor(type: string, eventInit?: MouseEventInit) {
+ super(type, eventInit);
+ Object.defineProperty(this, 'defaultPrevented', {
+ get: () => false,
+ });
+ }
</file context>
| const value = ref(args.modelValue); | ||
| const icon = computed( | ||
| // @ts-expect-error TS2322 | ||
| () => (args.items as SelectItem[])?.find((item) => item.value === value.value)?.icon, |
There was a problem hiding this comment.
Rule violated: Prefer Typeguards over Type casting
Remove the (args.items as SelectItem[]) assertion and either type the story args or guard the runtime value so you don't bypass the SelectItem contract; this cast currently suppresses TS2322 and violates the "Prefer Typeguards over Type casting" rule.
Prompt for AI agents
Address the following comment on packages/frontend/@n8n/design-system/src/v2/components/Select/Select.stories.ts at line 134:
<comment>Remove the `(args.items as SelectItem[])` assertion and either type the story args or guard the runtime value so you don't bypass the SelectItem contract; this cast currently suppresses TS2322 and violates the "Prefer Typeguards over Type casting" rule.</comment>
<file context>
@@ -0,0 +1,263 @@
+ const value = ref(args.modelValue);
+ const icon = computed(
+ // @ts-expect-error TS2322
+ () => (args.items as SelectItem[])?.find((item) => item.value === value.value)?.icon,
+ );
+ return { args, value, icon };
</file context>
| v-if="isSelectItem(item) && item.type === 'label'" | ||
| :class="$style.SelectLabel" | ||
| > | ||
| {{ get(item, props.labelKey as string) }} |
There was a problem hiding this comment.
Rule violated: Prefer Typeguards over Type casting
Please remove the as string cast on props.labelKey. The prop is typed as GetItemKeys<T> | undefined, so this assertion hides the undefined case—callers can still omit the prop, which would make get receive undefined and throw at runtime. Adjust the typing (e.g., derive a default key or refine the prop type) so the compiler knows the value is a string without a cast, in line with our “Prefer type guards over type casting” rule.
Prompt for AI agents
Address the following comment on packages/frontend/@n8n/design-system/src/v2/components/Select/Select.vue at line 141:
<comment>Please remove the `as string` cast on `props.labelKey`. The prop is typed as `GetItemKeys<T> | undefined`, so this assertion hides the undefined case—callers can still omit the prop, which would make `get` receive `undefined` and throw at runtime. Adjust the typing (e.g., derive a default key or refine the prop type) so the compiler knows the value is a string without a cast, in line with our “Prefer type guards over type casting” rule.</comment>
<file context>
@@ -0,0 +1,361 @@
+ v-if="isSelectItem(item) && item.type === 'label'"
+ :class="$style.SelectLabel"
+ >
+ {{ get(item, props.labelKey as string) }}
+ </SelectLabel>
+
</file context>
| */ | ||
| beforeAll(() => { | ||
| // Patch missing pointer APIs | ||
| const elementProto = HTMLElement.prototype as HTMLElement & { |
There was a problem hiding this comment.
Rule violated: Prefer Typeguards over Type casting
Avoid narrowing with as here; annotate the variable instead so the optional pointer APIs are reflected without a cast. The "Prefer Typeguards over Type casting" rule forbids this assertion scenario.
Prompt for AI agents
Address the following comment on packages/frontend/@n8n/design-system/src/__tests__/setup.ts at line 30:
<comment>Avoid narrowing with `as` here; annotate the variable instead so the optional pointer APIs are reflected without a cast. The "Prefer Typeguards over Type casting" rule forbids this assertion scenario.</comment>
<file context>
@@ -20,3 +21,60 @@ window.ResizeObserver =
+ */
+beforeAll(() => {
+ // Patch missing pointer APIs
+ const elementProto = HTMLElement.prototype as HTMLElement & {
+ hasPointerCapture?: (pointerId: number) => boolean;
+ setPointerCapture?: (pointerId: number) => void;
</file context>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| const variant = computed(() => variants[props.variant]); | ||
| const sizes = { |
There was a problem hiding this comment.
I'd suggest adding a constraint here to maintain consistency between props and defined values.
| const sizes = { | |
| const sizes: Record<SelectProps['size'], string> = { |
| triggerRef, | ||
| }); | ||
| const variants = { |
There was a problem hiding this comment.
I'd suggest adding a constraint here to maintain consistency between props and defined values.
| const variants = { | |
| const sizes: Record<SelectProps['variant'], string> = { |
| <RSelectItem | ||
| v-else | ||
| :disabled="isSelectItem(item) && item.disabled" | ||
| :value="isSelectItem(item) ? get(item, props.valueKey as string) : item" |
There was a problem hiding this comment.
This is likely to cause a performance issue. The get will get called on every single re-render for every single item. I'd add a wrapper component for SelectItem and have this live within a computed property.
There was a problem hiding this comment.
+1 on having a dedicated SelectItem component that is to be consumed by this new Select.
There was a problem hiding this comment.
in terms of performance for the amount of data that this component is expect to handle, there is no significant difference(if any). For the combobox that is expected to handle lots of data then yes.
i think the SelectedItem is not exported on purpose, why?
we have the item slot - that allows to override the content of the item - which also makes sure that no random component can be fed to the list
item-leading, item-label, item-trailing allow for chaging only specific parts
| <SelectItemText> | ||
| <slot name="item-label" :item="item as NestedItem<T>" :index="index"> | ||
| {{ isSelectItem(item) ? get(item, props.labelKey as string) : item }} |
There was a problem hiding this comment.
Same as above. This should probably get extracted alongside SelectItem (maybe in the same component).
| line-height: var(--line-height--md); | ||
| border: 1px solid transparent; | ||
| background-color: var(--color--background--light-2); | ||
| height: 24px; |
There was a problem hiding this comment.
Please discuss with design team whether we want to have consistent component heights across sizes and extract into CSS variables. I'd say yes, since we want the select to have the same height as inputs/buttons at this size.
There was a problem hiding this comment.
@Tuukkaa this one is for you :)
| background-color: var(--color--background--light-2); | ||
| height: 24px; | ||
| position: relative; | ||
| gap: 6px; |
| margin-left: auto; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| gap: 6px; |
| } | ||
| .SelectItem { | ||
| font-size: 13px; |
| position: relative; | ||
| user-select: none; | ||
| color: var(--text--color); | ||
| gap: 6px; |
| display: flex; | ||
| align-items: center; | ||
| height: 24px; | ||
| padding: 0 8px; |
| } | ||
| .SelectLabel { | ||
| padding: 6px 8px 4px; |
| :disabled="disabled" | ||
| :default-value="castToSelectItemValue(defaultValue)" | ||
| :model-value="castToSelectItemValue(modelValue)" | ||
| style="z-index: 1000" |
There was a problem hiding this comment.
Create and use z-index variable and say no to inline styles 😅
There was a problem hiding this comment.
this must have slipped from my test, nice catch!
|
Visual comments: I don't see the drop shadow for the up and down carets on scrollable options in the design: Also: the options list should also show a scrollbar |
| } | ||
| &[data-placeholder] { | ||
| color: var(--color--text--tint-1); |
There was a problem hiding this comment.
As raised by the accessibility plugin in figma (and also noticed by my own eyes when testing manual): this color results in a low contrast of 3.03:1, when it should be at least 4.5:1 (for reference: white on black would be 21:1 contrast ratio).
There was a problem hiding this comment.
Good point! Color contrasts have been a problem for a long while. We'll have to discuss with the design team.
| height: 24px; | ||
| position: relative; | ||
| gap: 6px; | ||
| color: var(--text--color); |
There was a problem hiding this comment.
I inspected the element in devtools to see what value this variable resolves to, and it turns out it's not defined. Is this just a sideeffect of the storybook dev server?
I also don't see any reference to --text-color anywhere else in our code though.
There was a problem hiding this comment.
the figma file doesn't have the right color token so i've done best estimates but we need the definitive ones, specially when it comes to accesibility
about the shadow and the scrollbar. this was already agreed with @Tuukkaa. even if it's not 1:1 with the designs |
3 participants
Summary
Add select component based on reka-ui for component migration
Link to figma screens: https://www.figma.com/design/8zib7Trf2D2CHYXrEGPHkg/n8n-Design-System-V3?node-id=2121-511
Related Linear tickets, Github issues, and Community forum posts
PAY-3996
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)