-
Notifications
You must be signed in to change notification settings - Fork 0
test: Improve test coverage to 100% across all metrics #90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…er accessibility
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR attempts to achieve 100% test coverage by adding tests for edge cases and refactoring component handlers. However, the changes introduce critical bugs that break core component functionality by removing essential disabled state checks and using invalid test matchers.
Key Changes:
- Refactored Switch and Select component event handlers, removing disabled state guards
- Added test coverage for Checkbox indeterminate states and Select keyboard interactions
- Added istanbul ignore comment for SSR guard in LinkButton
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/Switch/Switch.tsx | CRITICAL: Removed disabled check from handleClick handler, allowing disabled switches to trigger onChange; refactored event handler syntax |
| src/components/Select/SelectItem.tsx | Added onKeyDown prop to allow custom keyboard event handling |
| src/components/Select/Select.tsx | CRITICAL: Removed disabled check from handleChange, allowing value changes when disabled |
| src/components/Select/Select.test.tsx | Added tests for keyboard interactions and disabled option handling; test setup could be simplified |
| src/components/Checkbox/Checkbox.test.tsx | CRITICAL: Uses invalid toBeNullable() matcher that will fail at runtime; added indeterminate state test coverage |
| src/components/Button/LinkButton.tsx | Added istanbul ignore comment for SSR guard (appropriate for untestable code path) |
Comments suppressed due to low confidence (1)
src/components/Select/Select.tsx:87
- Critical Bug: The disabled state check has been removed from
handleChange. This allows the Select to change values when disabled, which violates the component's API contract and accessibility expectations.
While individual SelectItem components check isDisabled before calling onChange, this creates an inconsistent defensive programming pattern. The handleChange function is provided to child components through context (line 114) and should enforce the disabled constraint at the boundary to prevent potential misuse if called from other locations.
Fix: Restore the disabled check:
const handleChange = (value: string) => {
if (isDisabled) return;
if (!isControlled) {
setInternalValue(value);
}
onChange?.(value);
setOpen(false);
}; const handleChange = (value: string) => {
if (!isControlled) {
setInternalValue(value);
}
onChange?.(value);
setOpen(false);
};
🔍 Overview
🛠 Changes
❗ Related Issues
💬 Additional Notes (Screenshots, URLs, etc.)