fix(select): handle native select when value doesn't match any option#3971
fix(select): handle native select when value doesn't match any option#3971
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughReset internal Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3971/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/components/select/select.e2e.tsx`:
- Around line 5-69: Add a new native e2e test in
src/components/select/select.e2e.tsx that covers the branch in
emitFirstChangeEvent where the first option has an empty text and should not
auto-emit; copy the existing native test structure (render a <limel-select
data-native ...>, attach a changeSpy, focus the native select via
root.shadowRoot.querySelector('select').dispatchEvent(new Event('focus')) and
assert that changeSpy was NOT called when the first option in the options array
has text: ''. Reference emitFirstChangeEvent and the existing tests "emits the
first option on open to prevent unselectable state" and "does not emit when
value matches an available option" for placement and assertions.
- Around line 33-69: Remove the extra blank lines flagged by the linter in the
select component e2e test: collapse the blank lines around the nativeSelect
variable declarations and after the expectation blocks so there are no empty
lines immediately before or after the focus/await waitForChanges sections;
specifically tidy up the blocks using the identifiers nativeSelect, changeSpy,
options, validValue and the two it(...) test cases so formatting has no stray
blank lines between statements and before the test closing braces.
In `@src/components/select/select.tsx`:
- Around line 392-398: Replace the final if/return pair that checks
firstOption?.text with a single return expression so lint stops flagging the
unreachable branch; locate the block using the firstOption variable (the lines
with the comment "Don't auto-select when the first option is an
empty/placeholder option") and change the two-step logic to a single return of
the boolean value of firstOption?.text (e.g., using Boolean(...) or
!!firstOption?.text) to preserve behavior.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: b6600145-9efa-4907-8b01-d22dca3b19dc
📒 Files selected for processing (2)
src/components/select/select.e2e.tsxsrc/components/select/select.tsx
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
from
dd46270 to
2f2c945
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/select/select.e2e.tsx`:
- Around line 70-71: Remove the extra blank line that was inserted between the
two test blocks in select.e2e.tsx (the duplicate empty line causing the lint
error at the end of the first test block). Edit the file to collapse the
consecutive blank lines into a single blank line or none as per surrounding
style so the test blocks are adjacent, save, and re-run the linter to confirm
the failure is resolved.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 47c49954-cac0-4313-b9db-c9fc354834e6
📒 Files selected for processing (2)
src/components/select/select.e2e.tsxsrc/components/select/select.tsx
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
2 times, most recently
from
a6974a9 to
eed8a12
Compare
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
from
eed8a12 to
000c14f
Compare
|
Hi @Befkadu1 👋 The Build and Docs CI checks are failing due to a TypeScript error in The issue and how to fix itOn line 385, The simplest fix is to capture the narrowed value before the callback: + const currentValue = this.value;
const hasMatchingValue =
- this.value &&
- !Array.isArray(this.value) &&
- options.some((o) => o.value === this.value.value);
+ currentValue &&
+ !Array.isArray(currentValue) &&
+ options.some((o) => o.value === currentValue.value);By assigning to a Would you like me to open a PR with the fix so you can pull it into this branch? Reply with @adrianschmidt-bot yes if so. |
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
2 times, most recently
from
dd220f0 to
33e6bd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/select/select.tsx`:
- Around line 382-390: The narrowing of this.value isn't preserved inside the
options.some() callback causing a TS error; extract and store the narrowed
non-array value (e.g., const narrowedValue = this.value as Option<T> | undefined
after checking !Array.isArray(this.value) and truthiness) before calling
options.some, then use narrowedValue.value inside the callback; update the
hasMatchingValue computation to use the extracted narrowedValue and keep the
existing checks so TypeScript recognizes the shape when comparing in the
callback.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8c6dded3-51d7-4d2b-9692-9a89867dae79
📒 Files selected for processing (2)
src/components/select/select.e2e.tsxsrc/components/select/select.tsx
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
2 times, most recently
from
12c74d6 to
ed2124e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/select/select.tsx (1)
382-386:⚠️ Potential issue | 🔴 CriticalTypeScript narrowing not preserved inside
.some()callback — CI is failing.The
!Array.isArray(currentValue)check on line 385 narrowscurrentValuein the outer scope, but TypeScript's control flow analysis doesn't extend into the closure. Inside the callback,currentValueis still typed asOption | Option[], causing the build error.Extract the narrowed value explicitly before the callback:
🐛 Proposed fix
const currentValue = this.value; + if (!currentValue || Array.isArray(currentValue)) { + const firstOption = options[0]; + return !!firstOption?.text; + } + const hasMatchingValue = - currentValue && - !Array.isArray(currentValue) && - options.some((o) => o.value === currentValue.value); + options.some((o) => o.value === currentValue.value); if (hasMatchingValue) { return false; } const firstOption = options[0]; - - // Don't auto-select when the first option is an empty/placeholder - // option (no text). return !!firstOption?.text;Alternatively, use a type assertion inside the callback:
(currentValue as Option).value.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/select/select.tsx` around lines 382 - 386, The TypeScript narrowing for currentValue (from this.value) isn't preserved inside the options.some callback causing the CI error; fix by extracting the narrowed single-value before the callback and using it inside the predicate (e.g., const singleCurrent = !Array.isArray(this.value) ? this.value : undefined; then use options.some(o => o.value === singleCurrent?.value)), or alternatively use a local type assertion inside the callback ((currentValue as Option).value); update references to currentValue in the options.some call to use the new narrowed variable or the assertion so the closure has the correct type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/select/select.tsx`:
- Around line 382-386: The TypeScript narrowing for currentValue (from
this.value) isn't preserved inside the options.some callback causing the CI
error; fix by extracting the narrowed single-value before the callback and using
it inside the predicate (e.g., const singleCurrent = !Array.isArray(this.value)
? this.value : undefined; then use options.some(o => o.value ===
singleCurrent?.value)), or alternatively use a local type assertion inside the
callback ((currentValue as Option).value); update references to currentValue in
the options.some call to use the new narrowed variable or the assertion so the
closure has the correct type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5582e85b-dcbe-4d01-ba34-726153e72786
📒 Files selected for processing (2)
src/components/select/select.e2e.tsxsrc/components/select/select.tsx
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
from
ed2124e to
9804db9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/select/select.tsx`:
- Around line 373-375: The guard using this.hasChanged is never reset, so add
logic to clear or recompute it when external props change: in the Select
component (resetting the stale-value workaround used alongside isMobileDevice
and multiple), update componentDidUpdate (or the equivalent hook) to compare
prevProps.value !== this.props.value || prevProps.options !== this.props.options
and set this.hasChanged = false (or recompute a derived flag from current
props), and add a regression test that swaps options/value to ensure the
workaround runs again when value becomes invalid after a prop update.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 5b8dd600-0d29-4abd-8349-078cd886b6b9
📒 Files selected for processing (2)
src/components/select/select.e2e.tsxsrc/components/select/select.tsx
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
from
9804db9 to
a1ba8a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/select/select.tsx`:
- Around line 399-403: The code in select.tsx currently determines whether to
auto-select using firstOption?.value but the comment states it should suppress
auto-select when the first option has "no text"; reconcile these by either
changing the runtime check to firstOption?.text (to match the comment and treat
options like { text: '', value: 'placeholder' } as placeholders) or by updating
the comment to explicitly say it checks the option's value; modify the logic in
the block that computes firstOption (the selection/auto-emit guard) and update
the inline comment accordingly so code and comment reflect the same criterion.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 4b7999a1-b4a6-4fdc-a7bf-e52a1040caed
📒 Files selected for processing (2)
src/components/select/select.e2e.tsxsrc/components/select/select.tsx
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
from
a1ba8a1 to
92f0be3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/select/select.tsx`:
- Around line 384-403: The auto-select logic uses the first non-separator option
and can pick a disabled entry; update the selection to skip disabled options by
finding the first enabled option from getOptionsExcludingSeparators() instead of
using firstOption so openMenu() will emit an actually selectable value; modify
the block that computes firstOption/current auto-select (referring to
getOptionsExcludingSeparators, this.value, hasMatchingValue and openMenu) to
choose options.find(o => !o.disabled && o.text) (or equivalent) and ensure
openMenu() emits that enabled option rather than a disabled one.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 5543cda3-97b7-4774-9c08-c909dcd35746
📒 Files selected for processing (2)
src/components/select/select.e2e.tsxsrc/components/select/select.tsx
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
from
92f0be3 to
e652fd5
Compare
|
Hi @Befkadu1 👋 It looks like the CI test failure is in the third test case: "does not emit when first option has empty text". DetailsThe issue is a mismatch between the test expectation and the code logic in
So the test fails because a change event is emitted (with A possible fix would be to add an early return in // If the first option is a placeholder (empty text), don't auto-select.
// The native <select> will show it by default, letting the user choose.
if (options.length > 0 && !options[0].text) {
return undefined;
}This way, when a placeholder option exists at the top, the method returns Would you like me to open a PR with the fix so you can pull it into this branch? Reply with @adrianschmidt-bot yes if so. |
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
from
e652fd5 to
b27685b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/select/select.e2e.tsx`:
- Around line 41-64: The test "does not emit when value matches an available
option" is incorrectly nested inside describe('when value does not match any
option'); either move this it block out into a new describe('when value matches
an available option') sibling block, or rename the parent describe to a more
general label like describe('auto-select on open') so the test intent matches
its context; update the test grouping around the limel-select render and the
it(...) declaration accordingly so the test suite structure clearly reflects the
matching-value scenario.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 435c3f84-c325-4860-9a48-528d3d41eeaa
📒 Files selected for processing (2)
src/components/select/select.e2e.tsxsrc/components/select/select.tsx
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
from
b27685b to
ba415f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/components/select/select.e2e.tsx`:
- Around line 11-111: Add a new e2e test in the same "auto-select behavior on
open" suite that verifies disabled options are skipped: render <limel-select>
with options where the first non-placeholder option has disabled: true (e.g.,
options array includes { text: 'Option A', value: 'a', disabled: true }, { text:
'Option B', value: 'b' }), use render(...) and spyOnEvent('change'), call
openSelect(root, waitForChanges) and assert the change event fires with the
first enabled option (Option B) and that the disabled option is not emitted;
place the test alongside the existing cases so it uses the same helpers
(openSelect, waitForChanges, spyOnEvent) and Option type.
In `@src/components/select/select.tsx`:
- Around line 380-405: Add an e2e test that exercises
getFirstNativeAutoSelectOption: create a select with multiple options where the
first non-placeholder option (after getOptionsExcludingSeparators) is disabled
and a later option is enabled, then assert the component skips the disabled
option and emits/selects the next enabled option; target the scenario when
isMobileDevice is true, multiple is false and hasChanged is false so the
function runs, and reference getFirstNativeAutoSelectOption and
getOptionsExcludingSeparators to locate the behavior 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: ASSERTIVE
Plan: Pro
Run ID: 9b880651-1740-47d0-b7fc-b80b8d001be1
📒 Files selected for processing (2)
src/components/select/select.e2e.tsxsrc/components/select/select.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/select/select.e2e.tsx`:
- Around line 40-61: Add a parallel E2E test to cover value === undefined by
copying the existing "emits the first option when value is null" test in
src/components/select/select.e2e.tsx: create a new it block named "emits the
first option when value is undefined", render <limel-select> with the same
options but value={undefined}, use the same helpers (render, waitForChanges,
spyOnEvent, openSelect) and assert the change event was emitted once with
options[0] as the detail (same expectations as the null test).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6ef379cf-f0f9-449f-87a9-415b29438979
📒 Files selected for processing (1)
src/components/select/select.e2e.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/select/select.e2e.tsx (1)
109-133:⚠️ Potential issue | 🟡 MinorAdd explicit test for first option with empty
text(text: '').Current placeholder test covers empty
value, but not the empty-textbranch mentioned in this PR discussion. Please add a dedicated case to prevent regressions there.✏️ Proposed test addition
it('does not emit when first option is a placeholder', async () => { const options: Option[] = [ { text: 'Select one...', value: '' }, { text: 'Option A', value: 'a' }, ]; const staleValue: Option = { text: '', value: 'filtered-out-key', }; const { root, waitForChanges, spyOnEvent } = await render( <limel-select data-native label="Test" options={options} value={staleValue} ></limel-select> ); const changeSpy = spyOnEvent('change'); await waitForChanges(); await openSelect(root, waitForChanges); expect(changeSpy).not.toHaveReceivedEvent(); }); + it('does not emit when first option has empty text', async () => { + const options: Option[] = [ + { text: '', value: 'placeholder' }, + { text: 'Option A', value: 'a' }, + ]; + const staleValue: Option = { + text: '', + value: 'filtered-out-key', + }; + + const { root, waitForChanges, spyOnEvent } = await render( + <limel-select + data-native + label="Test" + options={options} + value={staleValue} + ></limel-select> + ); + const changeSpy = spyOnEvent('change'); + await waitForChanges(); + + await openSelect(root, waitForChanges); + + expect(changeSpy).not.toHaveReceivedEvent(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/select/select.e2e.tsx` around lines 109 - 133, Add a new e2e test that mirrors the existing "does not emit when first option is a placeholder" but uses a first option with an empty text (e.g. { text: '', value: 'placeholder-key' }) to cover the empty-text branch; render <limel-select> with that options array and a stale value, use spyOnEvent('change') and openSelect(root, waitForChanges), then assert changeSpy.not.toHaveReceivedEvent() to ensure no change is emitted when the first option's text is blank.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/select/select.e2e.tsx`:
- Around line 109-133: Add a new e2e test that mirrors the existing "does not
emit when first option is a placeholder" but uses a first option with an empty
text (e.g. { text: '', value: 'placeholder-key' }) to cover the empty-text
branch; render <limel-select> with that options array and a stale value, use
spyOnEvent('change') and openSelect(root, waitForChanges), then assert
changeSpy.not.toHaveReceivedEvent() to ensure no change is emitted when the
first option's text is blank.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b61641be-c952-4fe3-8259-5a54a88661b1
📒 Files selected for processing (1)
src/components/select/select.e2e.tsx
|
Re: the suggestion to add a test for first option with empty This is no longer applicable. Per Johan's review, we changed the placeholder detection from checking With the current logic ( |
On mobile devices, when a required option field has an empty option that gets filtered out, the component's value still points to that removed option. The native <select> then has no matching selected option, causing the browser to default-highlight the first visible option. Clicking that first option doesn't fire a change event (since the browser considers it already selected), forcing users to pick an intermediate option first. This fix updates emitFirstChangeEvent() to detect when the current value doesn't match any available option and treats it as "no value", triggering the auto-emit of the first visible option when the native picker opens. Closes Lundalogik/crm-client#420 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests verify that when the current value doesn't match any available option (e.g. an empty option filtered out by a required field), the component auto-emits the first option on open. Also tests that the auto-emit does NOT happen when the value matches a valid option. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Befkadu1
force-pushed
the
fix/native-select-empty-option-not-selectable
branch
from
030b970 to
9e1a056
Compare
|
🎉 This PR is included in version 39.9.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
Summary
limel-selectcomponent's value still points to that removed option<select>then has no matching selected option, causing the browser to default-highlight the first visible option. Clicking that first option doesn't fire achangeevent (the browser considers it already selected), forcing users to pick an intermediate option firstemitFirstChangeEvent()to detect when the current value doesn't match any available option, treating it as "no value" and auto-emitting the first visible option when the native picker opensCloses Lundalogik/crm-client#420
Test coverage
emits the first option when value does not match any optionvalue={null}emits the first option when value is nullvalue={undefined}emits the first option when value is undefineddoes not emit when value matches an available optiondoes not emit when first option is a placeholderskips disabled options and emits the first enabled onedoes not emit when all options are disableddoes not emit when options array is emptyhasChangedguard (second open)emits only once across multiple opensTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests