feat(select): multiselect tag handling improvements, story updates, visual fixes #587#613
Open
airikej wants to merge 8 commits into
Open
feat(select): multiselect tag handling improvements, story updates, visual fixes #587#613airikej wants to merge 8 commits into
airikej wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds bulk-selection utilities and UI (global and per-group), single-row multiselect tag measurement/overflow with contextual hiding, keyboard-mode focus behavior, single-value/tag helpers and contexts, displayName annotations, expanded stories/tests, and related styling updates. ChangesSelect component bulk & tag enhancements
Sequence Diagram(s)sequenceDiagram
participant User
participant MenuList
participant BulkHelpers
participant SelectRoot
User->>MenuList: click "Select all" checkbox
MenuList->>BulkHelpers: toggleBulkSelection(currentSelected, enabledOptions)
BulkHelpers-->>MenuList: newSelectedSet
MenuList->>SelectRoot: bulkApi.setValue(newSelectedSet, action)
SelectRoot-->>User: selection state updated (UI)
sequenceDiagram
participant Browser
participant ValueContainer
participant DOM
participant TagsContext
participant MultiValue
Browser->>ValueContainer: mount or selection change
ValueContainer->>DOM: render tags + refs
ValueContainer->>DOM: measure child widths (layout / ResizeObserver)
DOM-->>ValueContainer: widths
ValueContainer->>ValueContainer: compute visibleCount & hiddenCount
ValueContainer->>TagsContext: provide {isSingleRow, visibleCount}
MultiValue->>TagsContext: useSelectTagsContext()
MultiValue-->>Browser: render or hide tag based on visibleCount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
This was referenced Apr 28, 2026
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
airikej
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tedi/components/form/select/components/select-multi-value.tsx (1)
40-63:⚠️ Potential issue | 🟡 MinorMissing
displayNameon exported component.Per coding guidelines, all exported components must have a
displayNameproperty set.Proposed fix
export const SelectMultiValue = ({ isTagRemovable, children, removeProps, ...props }: MultiValueType): JSX.Element | null => { // ... component body }; + +SelectMultiValue.displayName = 'SelectMultiValue';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/select/components/select-multi-value.tsx` around lines 40 - 63, The exported React component SelectMultiValue is missing a displayName; add a static displayName property to the component after its declaration (e.g., set SelectMultiValue.displayName = 'SelectMultiValue') so the exported symbol SelectMultiValue has a readable displayName for debugging and tooling.
🧹 Nitpick comments (2)
src/tedi/components/form/select/components/select-group-heading.tsx (1)
15-62: Set adisplayNamefor this exported component.This component is exported from
src/tedi/components, so it should declaredisplayNameconsistently with the TEDI component conventions.As per coding guidelines, "All exported components must have a `displayName` property set".Suggested change
export const SelectGroupHeading = ({ optionGroupHeadingText, ...props }: GroupHeadingType): ReactElement => { const textSettings = props.data.text || optionGroupHeadingText; ... }; + +SelectGroupHeading.displayName = 'SelectGroupHeading';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/select/components/select-group-heading.tsx` around lines 15 - 62, The exported React component SelectGroupHeading is missing a displayName; add a displayName assignment after the component declaration (e.g., set SelectGroupHeading.displayName = 'SelectGroupHeading' or the TEDI naming convention used elsewhere) so the exported symbol has a declared displayName property consistent with other TEDI components; ensure this is added in the same file immediately after the SelectGroupHeading declaration.src/tedi/components/form/select/select.spec.tsx (1)
568-616: Consider extractingSelectMultiValueRemovetests to a dedicated spec file.These unit tests for
SelectMultiValueRemoveare nested inside theSelectdescribe block, but they test the component in isolation. Moving them toselect-multi-value-remove.spec.tsxwould improve test organization and align with the project's convention of placing tests incomponent-name.spec.tsxfiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/select/select.spec.tsx` around lines 568 - 616, Extract the SelectMultiValueRemove block into its own spec file: create select-multi-value-remove.spec.tsx and move the entire describe('SelectMultiValueRemove', ...) suite (including the helper renderRemove and all its tests) out of the larger Select spec; ensure the new file imports React, render, fireEvent, jest utilities, and the SelectMultiValueRemove component, and remove the duplicated tests from the original select.spec.tsx so only the isolated suite remains in the new file (keep the helper name renderRemove and test assertions unchanged).
🤖 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/tedi/components/form/select/components/select-bulk-helpers.ts`:
- Around line 29-35: getGroupEnabledOptions currently looks up the group by
label which breaks when multiple GroupBase<ISelectOption> share the same label;
change the helper (getGroupEnabledOptions) to accept the group object or its
options array directly (e.g., signature accepting GroupBase<ISelectOption> or
ISelectOption[] instead of groupLabel) and filter that array for non-disabled
items, then update callers to pass the current group (or group.options) rather
than the rendered label; keep the isGroupedOptions check only where callers
still need to guard for grouped vs flat options.
In `@src/tedi/components/form/select/components/select-group-heading.tsx`:
- Around line 42-55: The wrapper div and the Checkbox both call handleToggle,
causing duplicate toggles because clicks bubble; to fix, stop the click/change
event from bubbling when the Checkbox handles the toggle: change the Checkbox
onChange prop to a handler that calls event.stopPropagation() then invokes
handleToggle (e.g., onChange={(e) => { e.stopPropagation(); handleToggle(); }}),
ensuring the wrapper's onClick can remain for clicks outside the actual input
while checkbox activations only trigger one toggle; reference Checkbox and
handleToggle in your change.
- Around line 19-37: Replace direct usage of selectProps.value and
selectProps.onChange with react-select helpers: read current selection with
props.getValue() and update with props.setValue(...) inside handleToggle (use
same bulk-selection logic: getGroupEnabledOptions, toggleBulkSelection,
areAllSelected, isIndeterminate). Remove the wrapper element's onClick so only
the Checkbox's onChange calls handleToggle to avoid double firing. Ensure
handleToggle uses props.setValue to apply the new value and uses
props.getValue() to compute selected; keep references to selectableGroups,
isMulti and options as needed. Finally, add a displayName property to the
exported component to match the codebase convention.
In `@src/tedi/components/form/select/components/select-menu-list.tsx`:
- Around line 14-74: Add a displayName to the exported React component
SelectMenuList to satisfy the coding guideline; locate the export const
SelectMenuList declaration and, immediately after the component definition, set
its displayName property (e.g., SelectMenuList.displayName = 'SelectMenuList')
so the component has a readable name in devtools and warnings.
- Around line 48-65: The wrapper div and the Checkbox both call handleSelectAll
causing a double-toggle; fix by preventing duplicate invocation: either remove
the Checkbox prop onChange={handleSelectAll} so only the div's onClick invokes
handleSelectAll, or keep the Checkbox onChange and modify its handler to stop
propagation (e.g., call event.stopPropagation() before invoking handleSelectAll)
so the div's onClick won't also run; update references around the JSX containing
the div with class tedi-select__select-all, the Checkbox component and the
handleSelectAll function accordingly.
In `@src/tedi/components/form/select/components/select-value-container.tsx`:
- Around line 17-110: The exported React component SelectValueContainer is
missing a displayName; add a displayName property after its declaration like
SelectValueContainer.displayName = 'SelectValueContainer' so the component
follows the guideline and shows a readable name in React DevTools and error
messages; place this statement directly below the SelectValueContainer function
export in the same file.
- Around line 92-97: Exported component SelectValueContainer is missing a
displayName; add SelectValueContainer.displayName = 'SelectValueContainer' after
the component definition/export to satisfy the coding guideline. Locate the
SelectValueContainer component (references: SelectValueContainer, containerRef,
innerProps) and append the displayName assignment before exporting or
immediately after its declaration.
---
Outside diff comments:
In `@src/tedi/components/form/select/components/select-multi-value.tsx`:
- Around line 40-63: The exported React component SelectMultiValue is missing a
displayName; add a static displayName property to the component after its
declaration (e.g., set SelectMultiValue.displayName = 'SelectMultiValue') so the
exported symbol SelectMultiValue has a readable displayName for debugging and
tooling.
---
Nitpick comments:
In `@src/tedi/components/form/select/components/select-group-heading.tsx`:
- Around line 15-62: The exported React component SelectGroupHeading is missing
a displayName; add a displayName assignment after the component declaration
(e.g., set SelectGroupHeading.displayName = 'SelectGroupHeading' or the TEDI
naming convention used elsewhere) so the exported symbol has a declared
displayName property consistent with other TEDI components; ensure this is added
in the same file immediately after the SelectGroupHeading declaration.
In `@src/tedi/components/form/select/select.spec.tsx`:
- Around line 568-616: Extract the SelectMultiValueRemove block into its own
spec file: create select-multi-value-remove.spec.tsx and move the entire
describe('SelectMultiValueRemove', ...) suite (including the helper renderRemove
and all its tests) out of the larger Select spec; ensure the new file imports
React, render, fireEvent, jest utilities, and the SelectMultiValueRemove
component, and remove the duplicated tests from the original select.spec.tsx so
only the isolated suite remains in the new file (keep the helper name
renderRemove and test assertions unchanged).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e0beb85-d491-413a-b7ea-3ab609e97161
📒 Files selected for processing (14)
src/tedi/components/form/select/components/select-bulk-helpers.spec.tssrc/tedi/components/form/select/components/select-bulk-helpers.tssrc/tedi/components/form/select/components/select-group-heading.tsxsrc/tedi/components/form/select/components/select-menu-list.tsxsrc/tedi/components/form/select/components/select-multi-value.spec.tssrc/tedi/components/form/select/components/select-multi-value.tsxsrc/tedi/components/form/select/components/select-single-value.tsxsrc/tedi/components/form/select/components/select-tags-context.tssrc/tedi/components/form/select/components/select-value-container.tsxsrc/tedi/components/form/select/select.module.scsssrc/tedi/components/form/select/select.spec.tsxsrc/tedi/components/form/select/select.stories.tsxsrc/tedi/components/form/select/select.tsxsrc/tedi/providers/label-provider/labels-map.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tedi/components/form/select/components/select-value-container.tsx (1)
55-57: Consider deriving the class selector fromprops.selectProps.classNamePrefixfor robustness.Currently, the selector
.select__input-containerat lines 55-57 assumesclassNamePrefixis"select". While this is hardcoded in the Select component today, deriving the selector fromprops.selectProps.classNamePrefixwould make the code resilient to future changes. SinceselectPropsis already available via the component's props, this is a straightforward defensive improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/select/components/select-value-container.tsx` around lines 55 - 57, The code currently queries '.select__input-container' assuming classNamePrefix "select"; update the selector construction to derive the prefix from props.selectProps.classNamePrefix (falling back to "select" if undefined) before calling container.querySelector, and use that computed prefix to build the input container selector (the code around inputEl / inputMin / available in select-value-container.tsx). Ensure you still parse computed minWidth and compute available the same way after changing the selector.
🤖 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/tedi/components/form/select/components/select-group-heading.tsx`:
- Around line 55-57: The generated checkbox id currently concatenates
props.selectProps.instanceId with props.data.label which can collide when labels
repeat; change the id generation in the SelectGroupHeading component to use a
stable unique group identifier (e.g., props.data.id or props.data.value)
combined with props.selectProps.instanceId (and as a fallback use the group
index or a short generated uid) instead of props.data.label, and ensure the
matching label's htmlFor/for attribute uses that same id so the input/label
association remains correct.
---
Nitpick comments:
In `@src/tedi/components/form/select/components/select-value-container.tsx`:
- Around line 55-57: The code currently queries '.select__input-container'
assuming classNamePrefix "select"; update the selector construction to derive
the prefix from props.selectProps.classNamePrefix (falling back to "select" if
undefined) before calling container.querySelector, and use that computed prefix
to build the input container selector (the code around inputEl / inputMin /
available in select-value-container.tsx). Ensure you still parse computed
minWidth and compute available the same way after changing the selector.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3bcc1204-21f4-4460-aa63-a60c854d0a1e
📒 Files selected for processing (9)
src/tedi/components/form/select/components/select-bulk-helpers.spec.tssrc/tedi/components/form/select/components/select-bulk-helpers.tssrc/tedi/components/form/select/components/select-group-bulk-context.tssrc/tedi/components/form/select/components/select-group-heading.tsxsrc/tedi/components/form/select/components/select-group.tsxsrc/tedi/components/form/select/components/select-menu-list.tsxsrc/tedi/components/form/select/components/select-single-option.tsxsrc/tedi/components/form/select/components/select-value-container.tsxsrc/tedi/components/form/select/select.module.scss
✅ Files skipped from review due to trivial changes (1)
- src/tedi/components/form/select/components/select-bulk-helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tedi/components/form/select/select.module.scss
ly-tempel-bitweb
requested changes
May 5, 2026
ly-tempel-bitweb
left a comment
ly-tempel-bitweb
left a comment
There was a problem hiding this comment.
Please check the dark mode as well.
| isClearable={false} | ||
| isSearchable={false} | ||
| /> | ||
| <Select |
There was a problem hiding this comment.
In Angular, for Searchable select "Enter" has to be used to choose an option, with "Space" it writes to the input.
In React, it's possible to use "Space" for selecting when input is empty or predefined value is selected, otherwise it writes to the input as well.
I don't know, which behavior is correct, though. 😄
Contributor
Author
There was a problem hiding this comment.
could be react-selects own behaviour, cant really say if any of the behaviours is wrong
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tedi/components/form/select/components/select-multi-value.tsx (1)
40-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
displayNametoSelectMultiValue.This exported component still misses the repo-required
displayName, so it will show up anonymously in DevTools and stack traces.Proposed fix
export const SelectMultiValue = ({ isTagRemovable, children, removeProps, ...props }: MultiValueType): JSX.Element | null => { const { isSingleRow, visibleCount } = useSelectTagsContext(); @@ ); }; + +SelectMultiValue.displayName = 'SelectMultiValue';As per coding guidelines, "All exported components must have a
displayNameproperty set".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/form/select/components/select-multi-value.tsx` around lines 40 - 95, The exported React component SelectMultiValue is missing a displayName; add a displayName property after the component definition by assigning SelectMultiValue.displayName = 'SelectMultiValue' so the component is named in DevTools and stack traces; ensure the assignment appears in the same module after the SelectMultiValue function declaration and before the export is finalized.
🧹 Nitpick comments (6)
src/tedi/components/form/select/select.spec.tsx (2)
647-685: 💤 Low valueOverflow test relies on prototype-level width mocks that may bleed across tests.
Patching
HTMLElement.prototype.clientWidth/offsetWidthinbeforeAll/afterAllis global for every element rendered in thisdescribeblock (and any nested helpers). If any of the unrelated assertions in this PR (or future ones) start depending on real layout dimensions, they will silently see100/50. Consider scoping the mock to the test (beforeEach/afterEach) or stubbing on the specific node viajest.spyOn(node, 'clientWidth', 'get')so the override is local.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/form/select/select.spec.tsx` around lines 647 - 685, The test currently overrides HTMLElement.prototype.clientWidth/offsetWidth in beforeAll/afterAll which is global and can leak; change the setup to scope the mocks per test by moving the defineProperty/restore logic into beforeEach/afterEach or, better, replace the prototype stubs with per-node spies using jest.spyOn(element, 'clientWidth', 'get') and jest.spyOn(element, 'offsetWidth', 'get') for the rendered Select node inside the 'renders a +N counter...' test; update the restore logic to call mockRestore() on those spies (or restore the prototype in afterEach) and keep references to the spies so they are cleaned up after each test.
405-420: 💤 Low valueBrittle dependency on the absence of a
LabelProvider.These tests assert against the literal i18n key
'select.select-all'. If the test environment ever wraps the tree in aLabelProvider(e.g. via a global test setup), every Select-All assertion in this file will silently break. A safer pattern is to render with a knownLabelProvider(or mockuseLabels) and assert on the resolved label. Optional, but worth considering for resilience.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/form/select/select.spec.tsx` around lines 405 - 420, The test currently asserts the raw i18n key via SELECT_ALL_KEY which breaks if a LabelProvider is present; update the test to provide a deterministic label provider or mock for useLabels so the rendered "Select All" text is predictable. Specifically, in the test named 'renders a Select All toggle when showSelectAll is true in multi mode' (where SELECT_ALL_KEY and the render(<Select {...defaultProps} multiple showSelectAll isSearchable={false} />) call are used) wrap the rendered tree with a simple LabelProvider (or mock useLabels to return { 'select.select-all': 'Select All' }) and change the assertion to look for the resolved string ("Select All") instead of the raw key so the test no longer depends on the absence of a global LabelProvider.src/tedi/components/form/select/components/select-multi-option.tsx (1)
14-24: 💤 Low valueReuse
isSelectAllSentinelfor the sentinel check.
select-bulk-helpers.tsalready exportsisSelectAllSentineland the rest of the codebase uses it (e.g. insideselect.tsxonly the constant is used, but the helper exists for exactly this case). Direct=== SELECT_ALL_VALUEcomparisons here and on lines 19 and 21 duplicate that logic. Minor consistency nit.♻️ Suggested change
-import { areAllSelected, getEnabledOptions, isIndeterminate, SELECT_ALL_VALUE } from './select-bulk-helpers'; +import { areAllSelected, getEnabledOptions, isIndeterminate, isSelectAllSentinel } from './select-bulk-helpers'; @@ - const isSelectAll = props.data.value === SELECT_ALL_VALUE; + const isSelectAll = isSelectAllSentinel(props.data); @@ - const enabled = getEnabledOptions(props.options).filter((o) => o.value !== SELECT_ALL_VALUE); + const enabled = getEnabledOptions(props.options).filter((o) => !isSelectAllSentinel(o)); const selected = (props.getValue() as ReadonlyArray<ISelectOption>) ?? []; - const realSelected = selected.filter((o) => o.value !== SELECT_ALL_VALUE); + const realSelected = selected.filter((o) => !isSelectAllSentinel(o));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/form/select/components/select-multi-option.tsx` around lines 14 - 24, Replace direct sentinel comparisons to SELECT_ALL_VALUE inside the select-multi-option component with the shared helper isSelectAllSentinel: change the isSelectAll check from "props.data.value === SELECT_ALL_VALUE" to use isSelectAllSentinel(props.data.value) and similarly use that helper when filtering out the select-all sentinel from enabled and selected sets; keep the existing use of getEnabledOptions, props.getValue(), areAllSelected and isIndeterminate unchanged.src/tedi/components/form/select/select.tsx (3)
490-527: 💤 Low value
actionMeta.optioncast bypasses the existingActionMetadiscriminated union.
react-select'sActionMeta<ISelectOption>already narrowsoptionbased onaction('select-option' | 'deselect-option'carry the toggled option). Re-asserting via(actionMeta as { option?: ISelectOption })discards that typing and gives up the safety net for unrelated actions (e.g.'remove-value','clear'). Safer to switch onactionMeta.actionfirst and readactionMeta.optionfrom the narrowed branch.♻️ Suggested change
if (showSelectAllMode) { const enabled = getEnabledOptions(options ?? []); - const toggledOption = (actionMeta as { option?: ISelectOption }).option; - const toggledSentinel = toggledOption?.value === SELECT_ALL_VALUE; - - if (toggledSentinel && actionMeta.action === 'select-option') { + const toggledSentinel = + (actionMeta.action === 'select-option' || actionMeta.action === 'deselect-option') && + actionMeta.option?.value === SELECT_ALL_VALUE; + + if (toggledSentinel && actionMeta.action === 'select-option') { const previouslyDisabled = currentValueArray.filter( (s) => s.value !== SELECT_ALL_VALUE && !enabled.some((e) => e.value === s.value) ); resolved = [...previouslyDisabled, ...enabled]; } else if (toggledSentinel && actionMeta.action === 'deselect-option') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/form/select/select.tsx` around lines 490 - 527, In onChangeHandler, avoid the unsafe cast of actionMeta to { option?: ISelectOption }; instead switch on actionMeta.action (inside onChangeHandler) so TypeScript narrows the union and you can safely read actionMeta.option in the 'select-option' and 'deselect-option' branches; keep the existing sentinel logic (handle toggledSentinel when action is 'select-option' or 'deselect-option'), handle the Array.isArray(option) branch for other actions by filtering out SELECT_ALL_VALUE, and ensure other actions like 'remove-value' and 'clear' fall through to the normal resolved path without casting.
462-488: 💤 Low value
defaultValueis now effectively dead once the wrapper always suppliesvalue.
valueForReactSelectis computed fromcurrentValue, which isvaluewhen controlled andinternalValue(initialised todefaultValue ?? null) when uncontrolled. The result is always defined, which means react-select sees a non-undefinedvalueprop on every render and runs in controlled mode — so thedefaultValue={defaultValue}pass-through on line 618 has no effect (and risks a controlled/uncontrolled warning if anyone reasons about this prop later).Either drop
defaultValuefrom the spread or skip the wrapper's ownvaluewhen uncontrolled and let react-select keep usingdefaultValuedirectly. Behaviour is currently fine, but the prop wiring is misleading.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/form/select/select.tsx` around lines 462 - 488, The wrapper forces a defined value to react-select (making defaultValue unused); update value wiring so react-select can use its own defaultValue when uncontrolled: detect isControlled (existing const isControlled) and when !isControlled return undefined from valueForReactSelect (instead of returning currentValue), thereby preserving defaultValue={defaultValue} behavior; keep the existing select-all logic by only injecting selectAllSentinel when isControlled and when areAllSelected(currentValueArray, enabled) is true.
596-608: ⚡ Quick winExtend the
react-selectmodule augmentation for the custom selectProps.The single
@ts-expect-errorsuppresses TypeScript errors only on the immediate next attribute. With seven custom props following (showSelectAll,selectableGroups,dropdownType,renderValue,keyboardMode,exitKeyboardMode), and strict mode enabled, this pattern is fragile. While the build currently passes (likely because TypeScript reports JSX-attribute mismatches as an aggregate error), adding proper module augmentation makes the code more robust and explicit:♻️ Extend the existing augmentation
declare module 'react-select/dist/declarations/src/Select' { export interface Props<Option, IsMulti extends boolean, Group extends GroupBase<Option>> { inputIsHidden?: boolean; + tagsDirection?: 'stack' | 'row'; + showSelectAll?: boolean; + selectableGroups?: boolean; + dropdownType?: 'menu' | 'grid'; + renderValue?: (option: ISelectOption) => React.ReactNode; + keyboardMode?: boolean; + exitKeyboardMode?: () => void; } }After updating, remove the
@ts-expect-errorcomment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/form/select/select.tsx` around lines 596 - 608, The JSX uses ReactSelectElement with several custom props (tagsDirection, showSelectAll, selectableGroups, dropdownType, renderValue, keyboardMode, exitKeyboardMode) but only one `@ts-expect-error` was used so TypeScript still flags the other attributes; fix by adding a proper module augmentation for "react-select" that extends the SelectProps/Props/PropsWithChildren (or the appropriate exported SelectComponent props interface) to include these custom props (types for ISelectOption/IGroupedOptions where applicable), update the augmentation file, then remove the `@ts-expect-error` line and ensure ReactSelectElement accepts those custom props without errors (look for ReactSelectElement generic usage and the listed prop names to verify).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tedi/components/form/select/components/select-menu-list.tsx`:
- Around line 16-20: Replace the unsafe cast through unknown when reading
selectProps by declaring the proper generic/intersection type for MenuListProps
so TypeScript knows the custom fields; e.g., change usage of selectProps to a
typed signature like MenuListProps & { keyboardMode?: boolean;
exitKeyboardMode?: () => void; dropdownType?: 'menu' | 'grid' } (mirroring the
pattern used by SelectValueContainer) and then read keyboardMode,
exitKeyboardMode and dropdownType from that properly typed selectProps instead
of using "as unknown as".
In `@src/tedi/components/form/select/components/select-value-container.tsx`:
- Around line 116-129: The loop that measures tag widths and sets visible (in
select-value-container.tsx) currently forces visible to 1 when no tags fit (if
(visible === 0) visible = 1;), which causes overflow/clipping; remove that line
so visible can remain 0 when nothing fits, leaving the hiddenCount branch to
render only the counter and input; keep the rest of the logic (including
updating lastMeasuredWidthRef.current = containerWidth) unchanged.
---
Outside diff comments:
In `@src/tedi/components/form/select/components/select-multi-value.tsx`:
- Around line 40-95: The exported React component SelectMultiValue is missing a
displayName; add a displayName property after the component definition by
assigning SelectMultiValue.displayName = 'SelectMultiValue' so the component is
named in DevTools and stack traces; ensure the assignment appears in the same
module after the SelectMultiValue function declaration and before the export is
finalized.
---
Nitpick comments:
In `@src/tedi/components/form/select/components/select-multi-option.tsx`:
- Around line 14-24: Replace direct sentinel comparisons to SELECT_ALL_VALUE
inside the select-multi-option component with the shared helper
isSelectAllSentinel: change the isSelectAll check from "props.data.value ===
SELECT_ALL_VALUE" to use isSelectAllSentinel(props.data.value) and similarly use
that helper when filtering out the select-all sentinel from enabled and selected
sets; keep the existing use of getEnabledOptions, props.getValue(),
areAllSelected and isIndeterminate unchanged.
In `@src/tedi/components/form/select/select.spec.tsx`:
- Around line 647-685: The test currently overrides
HTMLElement.prototype.clientWidth/offsetWidth in beforeAll/afterAll which is
global and can leak; change the setup to scope the mocks per test by moving the
defineProperty/restore logic into beforeEach/afterEach or, better, replace the
prototype stubs with per-node spies using jest.spyOn(element, 'clientWidth',
'get') and jest.spyOn(element, 'offsetWidth', 'get') for the rendered Select
node inside the 'renders a +N counter...' test; update the restore logic to call
mockRestore() on those spies (or restore the prototype in afterEach) and keep
references to the spies so they are cleaned up after each test.
- Around line 405-420: The test currently asserts the raw i18n key via
SELECT_ALL_KEY which breaks if a LabelProvider is present; update the test to
provide a deterministic label provider or mock for useLabels so the rendered
"Select All" text is predictable. Specifically, in the test named 'renders a
Select All toggle when showSelectAll is true in multi mode' (where
SELECT_ALL_KEY and the render(<Select {...defaultProps} multiple showSelectAll
isSearchable={false} />) call are used) wrap the rendered tree with a simple
LabelProvider (or mock useLabels to return { 'select.select-all': 'Select All'
}) and change the assertion to look for the resolved string ("Select All")
instead of the raw key so the test no longer depends on the absence of a global
LabelProvider.
In `@src/tedi/components/form/select/select.tsx`:
- Around line 490-527: In onChangeHandler, avoid the unsafe cast of actionMeta
to { option?: ISelectOption }; instead switch on actionMeta.action (inside
onChangeHandler) so TypeScript narrows the union and you can safely read
actionMeta.option in the 'select-option' and 'deselect-option' branches; keep
the existing sentinel logic (handle toggledSentinel when action is
'select-option' or 'deselect-option'), handle the Array.isArray(option) branch
for other actions by filtering out SELECT_ALL_VALUE, and ensure other actions
like 'remove-value' and 'clear' fall through to the normal resolved path without
casting.
- Around line 462-488: The wrapper forces a defined value to react-select
(making defaultValue unused); update value wiring so react-select can use its
own defaultValue when uncontrolled: detect isControlled (existing const
isControlled) and when !isControlled return undefined from valueForReactSelect
(instead of returning currentValue), thereby preserving
defaultValue={defaultValue} behavior; keep the existing select-all logic by only
injecting selectAllSentinel when isControlled and when
areAllSelected(currentValueArray, enabled) is true.
- Around line 596-608: The JSX uses ReactSelectElement with several custom props
(tagsDirection, showSelectAll, selectableGroups, dropdownType, renderValue,
keyboardMode, exitKeyboardMode) but only one `@ts-expect-error` was used so
TypeScript still flags the other attributes; fix by adding a proper module
augmentation for "react-select" that extends the
SelectProps/Props/PropsWithChildren (or the appropriate exported SelectComponent
props interface) to include these custom props (types for
ISelectOption/IGroupedOptions where applicable), update the augmentation file,
then remove the `@ts-expect-error` line and ensure ReactSelectElement accepts
those custom props without errors (look for ReactSelectElement generic usage and
the listed prop names to verify).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baea9273-66f7-481d-b93e-54142667e1ca
📒 Files selected for processing (14)
src/tedi/components/form/select/components/select-bulk-helpers.tssrc/tedi/components/form/select/components/select-group-heading.tsxsrc/tedi/components/form/select/components/select-menu-list.tsxsrc/tedi/components/form/select/components/select-multi-option.tsxsrc/tedi/components/form/select/components/select-multi-value.tsxsrc/tedi/components/form/select/components/select-single-option.tsxsrc/tedi/components/form/select/components/select-value-container.tsxsrc/tedi/components/form/select/examples/multiple-handled.tsxsrc/tedi/components/form/select/select.module.scsssrc/tedi/components/form/select/select.spec.tsxsrc/tedi/components/form/select/select.stories.tsxsrc/tedi/components/form/select/select.tsxsrc/tedi/components/tags/tag/tag.module.scsssrc/tedi/components/tags/tag/tag.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tedi/components/form/select/components/select-multi-value.tsx (1)
40-88:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
displayNameon the exported component.
SelectMultiValueis exported but does not assign adisplayName. This violates the project rule that all exported components must declare one (also reflected in the recently-addedSelectValueContainer.displayNamein this PR).Proposed fix
export const SelectMultiValue = ({ isTagRemovable, children, removeProps, ...props }: MultiValueType): JSX.Element | null => { // ... }; + +SelectMultiValue.displayName = 'SelectMultiValue';As per coding guidelines: "All exported components must have a
displayNameproperty set".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/form/select/components/select-multi-value.tsx` around lines 40 - 88, The exported React component SelectMultiValue is missing a displayName; add a displayName property on the function after its declaration, e.g. SelectMultiValue.displayName = 'SelectMultiValue', so the component has an explicit name for debugging and conforms to the project rule that all exported components declare displayName.
🧹 Nitpick comments (3)
src/tedi/components/form/select/components/select-value-container.tsx (2)
31-38: 💤 Low valueRe-measuring on every
inputValuekeystroke is unnecessary work.
inputValueis in this effect's dependency list, so each keystroke in the search input nullsvisibleCountand forces the second layout effect to re-query the DOM and re-measure all tags. Tag widths don't change as the user types — only the input's own width might, and that is already covered by theResizeObserveron the container. DroppinginputValuefrom the deps avoids the extra synchronous measurement on every keypress.Proposed change
- }, [isSingleRow, totalCount, isFocused, inputValue]); + }, [isSingleRow, totalCount, isFocused]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/form/select/components/select-value-container.tsx` around lines 31 - 38, The effect using useLayoutEffect (the block that sets visibleCount based on isSingleRow) is re-running on every keystroke because inputValue is in its dependency array; remove inputValue from the deps so typing doesn't unnecessarily null visibleCount and trigger synchronous re-measurements. Update the dependency array for the useLayoutEffect that references isSingleRow, totalCount and isFocused (keep the existing eslint disable if you still need to suppress exhaustive-deps), ensuring only [isSingleRow, totalCount, isFocused] remain and do not include inputValue or other transient values.
71-77: ⚡ Quick winConsider decoupling from react-select's
classNamePrefixclass name for the input measurement.The query
.select__input-containerdepends onclassNamePrefix="select"remaining fixed. While currently hardcoded inselect.tsx(line 617), if the prefix is ever changed or removed,inputElbecomesnullandinputReservefalls back to0, causing overflow calculations to misalign.Since the codebase already uses
data-tedi-*attributes for stable element hooks (e.g.,data-tedi-tag-index), consider addingdata-tedi-select-input-containerto the input element inSelectInputand querying that instead. This provides a single source of truth and future-proofs against prefix changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/form/select/components/select-value-container.tsx` around lines 71 - 77, The measurement currently queries for '.select__input-container' in select-value-container.tsx which relies on react-select's classNamePrefix; update the code to query a stable data attribute instead (e.g., data-tedi-select-input-container) so the element lookup doesn't break if classNamePrefix changes: add data-tedi-select-input-container to the input element in the SelectInput component and replace the querySelector('.select__input-container') usage in the logic that computes inputReserve to querySelector('[data-tedi-select-input-container]') (keeping the same offsetWidth/minWidth fallback behavior).src/tedi/components/form/select/components/select-multi-value.tsx (1)
9-65: 💤 Low valueRedundant logic between the helper and the in-component handlers.
createMultiValueCloseHandleralready discriminates onevent.typeand re-checksEnter/Space, but the in-componenthandleCloseClick/handleCloseKeyDownduplicate the propagation/preventDefault and key-filter logic, then delegate back to the helper. The helper effectively becomes a thin re-wrapper ofremoveProps.onClick. Either:
- Drop
handleCloseClick/handleCloseKeyDownand passhandleClosedirectly (keeping the helper as the single source of truth), or- Inline the handler logic in the component and let the exported helper exist purely for the spec.
Also worth reconsidering the intersection type on line 23 (
MouseEventHandler<…> & KeyboardEventHandler<…>). Functionally what you want is a single function accepting either event, which reads more clearly as a union on the parameter:Suggested simplification
-export const createMultiValueCloseHandler = - ( - removeProps: RemoveProps - ): React.MouseEventHandler<HTMLButtonElement> & React.KeyboardEventHandler<HTMLButtonElement> => - (event) => { +export const createMultiValueCloseHandler = + (removeProps: RemoveProps) => + (event: React.MouseEvent<HTMLButtonElement> | React.KeyboardEvent<HTMLButtonElement>) => { if (event.type === 'click' && removeProps.onClick) { removeProps.onClick(event as unknown as React.MouseEvent<HTMLDivElement>); return; } - - if (event.type === 'keydown') { - const keyboardEvent = event as React.KeyboardEvent<HTMLButtonElement>; - if ((keyboardEvent.key === 'Enter' || keyboardEvent.key === ' ') && removeProps.onClick) { - keyboardEvent.preventDefault(); - keyboardEvent.stopPropagation(); - removeProps.onClick(keyboardEvent as unknown as React.MouseEvent<HTMLDivElement>); - } + if (event.type === 'keydown' && removeProps.onClick) { + const ke = event as React.KeyboardEvent<HTMLButtonElement>; + if (ke.key === 'Enter' || ke.key === ' ') { + ke.preventDefault(); + ke.stopPropagation(); + removeProps.onClick(ke as unknown as React.MouseEvent<HTMLDivElement>); + } } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/form/select/components/select-multi-value.tsx` around lines 9 - 65, The component duplicates event discrimination and propagation handling: createMultiValueCloseHandler already handles click vs key events and Enter/Space, but SelectMultiValue defines handleCloseClick and handleCloseKeyDown that repeat the same logic and then call handleClose; remove the redundant wrappers by passing handleClose (the result of createMultiValueCloseHandler) directly to the Tag/button handlers, or alternatively move the full logic into SelectMultiValue and delete createMultiValueCloseHandler; also change the createMultiValueCloseHandler signature (and its return type) to accept a single event parameter union rather than an intersection so it clearly models "MouseEvent | KeyboardEvent" when calling removeProps.onClick (referencing createMultiValueCloseHandler, SelectMultiValue, handleClose, handleCloseClick, handleCloseKeyDown, and removeProps.onClick).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/tedi/components/form/select/components/select-multi-value.tsx`:
- Around line 40-88: The exported React component SelectMultiValue is missing a
displayName; add a displayName property on the function after its declaration,
e.g. SelectMultiValue.displayName = 'SelectMultiValue', so the component has an
explicit name for debugging and conforms to the project rule that all exported
components declare displayName.
---
Nitpick comments:
In `@src/tedi/components/form/select/components/select-multi-value.tsx`:
- Around line 9-65: The component duplicates event discrimination and
propagation handling: createMultiValueCloseHandler already handles click vs key
events and Enter/Space, but SelectMultiValue defines handleCloseClick and
handleCloseKeyDown that repeat the same logic and then call handleClose; remove
the redundant wrappers by passing handleClose (the result of
createMultiValueCloseHandler) directly to the Tag/button handlers, or
alternatively move the full logic into SelectMultiValue and delete
createMultiValueCloseHandler; also change the createMultiValueCloseHandler
signature (and its return type) to accept a single event parameter union rather
than an intersection so it clearly models "MouseEvent | KeyboardEvent" when
calling removeProps.onClick (referencing createMultiValueCloseHandler,
SelectMultiValue, handleClose, handleCloseClick, handleCloseKeyDown, and
removeProps.onClick).
In `@src/tedi/components/form/select/components/select-value-container.tsx`:
- Around line 31-38: The effect using useLayoutEffect (the block that sets
visibleCount based on isSingleRow) is re-running on every keystroke because
inputValue is in its dependency array; remove inputValue from the deps so typing
doesn't unnecessarily null visibleCount and trigger synchronous re-measurements.
Update the dependency array for the useLayoutEffect that references isSingleRow,
totalCount and isFocused (keep the existing eslint disable if you still need to
suppress exhaustive-deps), ensuring only [isSingleRow, totalCount, isFocused]
remain and do not include inputValue or other transient values.
- Around line 71-77: The measurement currently queries for
'.select__input-container' in select-value-container.tsx which relies on
react-select's classNamePrefix; update the code to query a stable data attribute
instead (e.g., data-tedi-select-input-container) so the element lookup doesn't
break if classNamePrefix changes: add data-tedi-select-input-container to the
input element in the SelectInput component and replace the
querySelector('.select__input-container') usage in the logic that computes
inputReserve to querySelector('[data-tedi-select-input-container]') (keeping the
same offsetWidth/minWidth fallback behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6728fcf7-7a44-49ff-ada9-ec05fe395f12
📒 Files selected for processing (5)
src/tedi/components/form/select/components/select-menu-list.tsxsrc/tedi/components/form/select/components/select-multi-value.tsxsrc/tedi/components/form/select/components/select-value-container.tsxsrc/tedi/components/form/select/select.module.scsssrc/tedi/components/form/select/select.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tedi/components/form/select/components/select-menu-list.tsx
- src/tedi/components/form/select/select.module.scss
- src/tedi/components/form/select/select.tsx
| isTagRemovable | ||
| isClearable | ||
| /> | ||
| <Select |
There was a problem hiding this comment.
When selecting only "Longer text on one row" it turns to "+1", but together with "Longer text" it shows both options in full.
Screen.Recording.2026-05-08.at.13.00.21.mov
Contributor
Author
There was a problem hiding this comment.
the examples should work better now and the bugs fixed. improved async description as well
There was a problem hiding this comment.
Tested it and the issue seems to be present still:
Screen.Recording.2026-05-19.at.15.15.53.mov
| .tedi-select__single-value, | ||
| .tedi-select__multi-value, | ||
| .tedi-select__placeholder, | ||
| .tedi-select__input { |
There was a problem hiding this comment.
Seems that this color is not applied:
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Behavior
Style
Documentation
Tests
Labels