fix: single-select validation and improve test coverage#10055
fix: single-select validation and improve test coverage#10055deleonio wants to merge 10 commits intocodex/fix-issue-with-component-dasfrom
Conversation
Changes: 1. Single-select: Add data-testid attribute to clear button for e2e test visibility 2. Single-select: Normalize undefined _value to null immediately 3. Single-select: Check and clear _value when it becomes invalid due to options change 4. Toolbar: Call component focus() method directly instead of native focus() Fixes test failures: - single-select: Clear button visibility and clearing functionality - single-select: Value clearing when options change or value set to undefined - toolbar: Proper focus delegation to currently active toolbar item https://claude.ai/code/session_01BRnViGPGv2XXH7kaE3EDCZ
The setFocus function is no longer used after updating the focus() method to call the component's focus() method directly. https://claude.ai/code/session_01BRnViGPGv2XXH7kaE3EDCZ
deleonio
changed the title
|
🚀 MCP preview deployed to Vercel: https://kolibri-67osbxbmf-public-ui-kolibri-mcp.vercel.app |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Playwright configuration to use a stable server for all environments, adds a test ID to the single-select component, and implements logic to reset the single-select value if it becomes invalid after options change. Additionally, it refactors the toolbar's focus handling. Feedback was provided to optimize the value normalization in the single-select component to avoid redundant watcher execution and to improve type safety in the toolbar component by replacing a type cast to any with FocusableElement.
| value = null; | ||
| this._value = null; |
There was a problem hiding this comment.
The normalization of undefined to null causes this._value to be modified, which will re-trigger the @Watch('_value') decorator. The current implementation continues to execute the rest of the function with the new null value, which is redundant because the function will be called again by the watcher.
To make the flow clearer and avoid redundant execution, you can simply set this._value = null and then return from the if block.
| value = null; | |
| this._value = null; | |
| this._value = null; | |
| return; |
|
Netlify Draft Deployment |
The custom implementation caused ESLint errors due to unsafe any type usage. Reverting to the original setFocus approach which is properly typed. https://claude.ai/code/session_01BRnViGPGv2XXH7kaE3EDCZ
The unconditional command change was causing test failures because the directory move operation was being executed in CI environment. Restoring the original behavior where the move only happens in CI mode. https://claude.ai/code/session_01BRnViGPGv2XXH7kaE3EDCZ
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a data-testid for the single-select delete button, performs minor whitespace cleanup, and adds logic to validate the current selection when options change or when undefined values are encountered. Feedback was provided to improve the validation logic by using the parsed state options instead of the raw input to correctly handle JSON strings and to ensure that disabled options are not considered valid selections.
- Use `this.state._options` instead of raw `value` parameter to handle options passed as JSON strings via HTML attributes - Add check to exclude disabled options from being considered valid selections - Ensures consistency with rest of component validation logic Addresses code review feedback from Gemini. https://claude.ai/code/session_01BRnViGPGv2XXH7kaE3EDCZ
Ensure `this.state._options` is an array before calling `.some()` to prevent potential runtime errors when the state hasn't been fully initialized. https://claude.ai/code/session_01BRnViGPGv2XXH7kaE3EDCZ
Use the `value` parameter directly if it's an array (for direct property assignment in tests), otherwise fall back to `this.state._options` (for JSON string attributes). This ensures the validation works in both scenarios while maintaining the disabled option check from the code review. https://claude.ai/code/session_01BRnViGPGv2XXH7kaE3EDCZ
Remove unnecessary fallback logic and use the `value` parameter passed to the watcher, which represents the new options being set. Add proper type assertion to fix TypeScript issues. https://claude.ai/code/session_01BRnViGPGv2XXH7kaE3EDCZ
Call updateInputValue unconditionally in validateOptions to leverage its existing logic for clearing invalid values when options change. This ensures the value is cleared if it no longer exists in the new options (including disabled status check and undefined normalization). https://claude.ai/code/session_01BRnViGPGv2XXH7kaE3EDCZ
single-select: Use state._options (parsed by controller) for validation and restore original else-branch for updateInputValue to avoid erasing user input when _value is null. toolbar: Call firstEnabledItem.focus() (the Stencil @method) instead of setFocus() which calls the native HTMLElement focus and doesn't delegate to the inner button. Both HTMLKolButtonWcElement and HTMLKolLinkWcElement expose focus(): Promise<void> so no any-cast is needed. https://claude.ai/code/session_01BRnViGPGv2XXH7kaE3EDCZ
2 participants
Summary
This PR addresses value validation in the single-select component and improves test infrastructure consistency.
Key Changes
Single-select validation improvements:
data-testid="single-select-delete") to the delete button for better test coverageundefinedvalues tonullfor consistent state managementToolbar focus handling:
setFocusutility import with direct focus method invocation on the elementTest configuration:
Implementation Details
The single-select validation now properly handles edge cases where options are dynamically updated. When
validateOptions()is called with a new options array, it checks if the currently selected value still exists in the new options. If not, it clears the selection and resets the input value, preventing orphaned selections.The undefined-to-null normalization ensures that the component's internal state remains consistent regardless of how values are passed in from parent components.
https://claude.ai/code/session_01BRnViGPGv2XXH7kaE3EDCZ