fix: Add validation feedback for test suite name in evals #944
Conversation
dosubot
bot
added
size:L
WalkthroughThis change refactors the suite name editing mechanism from a stateful in-place editing mode to a simplified validation-flag approach. The editing state variables are removed and replaced with a Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/src/components/evals/eval-runner/ReviewStep.tsx (1)
54-56: Consider extracting the duplicate error condition.The check
showNameError && !suiteName.trim()appears in both styling and message rendering.Optional refactor to reduce duplication:
export function ReviewStep({ suiteName, suiteDescription, minimumPassRate, selectedServers, selectedModels, validTestTemplates, onSuiteNameChange, onSuiteDescriptionChange, onMinimumPassRateChange, onEditStep, showNameError = false, }: ReviewStepProps) { + const hasNameError = showNameError && !suiteName.trim(); + return ( <div className="space-y-6"> <div className="space-y-4"> <div className="space-y-2"> <Label htmlFor="suite-name" className="text-sm font-medium"> Test Suite Name <span className="text-destructive">*</span> </Label> <Input id="suite-name" type="text" value={suiteName} onChange={(event) => onSuiteNameChange(event.target.value)} placeholder="e.g., Weather API Integration Tests" className={cn( "text-lg font-semibold", - showNameError && - !suiteName.trim() && - "border-destructive focus-visible:ring-destructive", + hasNameError && "border-destructive focus-visible:ring-destructive", )} autoFocus /> - {showNameError && !suiteName.trim() && ( + {hasNameError && ( <p className="text-sm text-red-600 font-medium"> Please provide a name for this test suite </p> )}Also applies to: 60-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/components/evals/eval-runner.tsx(4 hunks)client/src/components/evals/eval-runner/ReviewStep.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/components/evals/eval-runner/ReviewStep.tsx (2)
shared/types.ts (1)
ModelDefinition(148-155)client/src/components/evals/eval-runner/types.ts (1)
TestTemplate(6-11)
🔍 Remote MCP
I'll search for relevant information about form validation best practices and patterns that would help in reviewing this PR effectively.## Form Validation & Accessibility Best Practices for PR Review
Based on comprehensive research, here are key considerations for reviewing the PR implementation:
Validation Error Messaging Best Practices
Error messages should be displayed near corresponding input fields and form submission should be prevented when validation errors exist. The PR follows this approach with inline validation feedback.
Inline validation displaying errors next to the problematic field provides relevant feedback in context and improves user experience. The PR's approach of showing a red border with inline error message aligns with this guidance.
WCAG Accessibility Compliance Considerations
Critical: WCAG Success Criterion 3.3.1 requires that when an error is detected, the problematic field must be identified and the error described in text—not solely relying on visual cues like red borders.
Key accessibility requirements:
aria-describedbyassociates error messages with fields for assistive technologies, whilearia-invalidindicates if a field has an error- Do not use visual cues like color or icons alone to identify form fields with errors—if color is used, provide the same information in an alternative method
- Move focus to the first form control with an error or common error message, and associate inline errors with form fields using
aria-describedby
Required Field Indicator (Asterisk) Best Practices
Using an asterisk to mark required fields is an easy way to improve form usability, and the answer is yes even if most fields are required. The PR includes a required field indicator, which is appropriate.
Implementation guidance: Major design systems (Google Material Design, Salesforce Lightning, Apple HIG) use the asterisk for its discreet yet effective visibility, ensuring users can identify requirements without overwhelming the interface.
Accessibility consideration: Asterisks provide visual solutions but can be less meaningful to screen readers; also, screen readers don't necessarily announce form field labels unless users Tab to them.
Auto-focus Considerations
The PR implements auto-focus on the name input field. Automatic focus management that moves focus to the first error field helps keyboard users quickly identify and resolve issues.
Key Review Checklist Items
- Verify error messages pair visual indicators with text: Pair visual indicators with descriptive text to ensure all users, including those with visual impairments, understand the error
- Confirm ARIA attributes are implemented: The validation approach should include
aria-invalidandaria-describedbyfor accessibility - Test focus management: Verify auto-focus behavior works as intended without causing unexpected screen reader announcements
- Validate error message clarity: Clear validation feedback helps users pinpoint issues quickly; avoid generic messages like "Invalid input" in favor of specific guidance explaining what's wrong
[::web_search::] - Form Validation Best Practices
[::web_search::] - Accessibility and WCAG Error Identification
[::web_search::] - Required Field Indicators and UX Standards
🔇 Additional comments (7)
client/src/components/evals/eval-runner.tsx (4)
113-113: Clean state addition for validation feedback.The
showNameErrorflag provides clear UI state tracking without side effects.
457-464: Validation logic correctly prevents submission with missing name.The approach of setting the error flag and staying on the current step provides better UX than the previous navigation-based feedback.
544-544: Appropriate cleanup of validation state.Clearing
showNameErroralongside form fields ensures clean state for subsequent runs.
603-608: Props correctly wired to ReviewStep.The direct state setter pattern for
onSuiteNameChangeand the validation flag propagation align with the simplified API design.client/src/components/evals/eval-runner/ReviewStep.tsx (3)
2-3: Necessary imports for the refactored UI.Input, Label, and cn utility support the new always-visible validation approach.
Also applies to: 8-8
12-24: Interface cleanly reflects the simplified validation model.Removing editing-mode props in favor of direct
onSuiteNameChangeandshowNameErrorreduces API surface and complexity.
66-78: Clean implementation of the optional description field.The separate Label and Textarea with clear "(optional)" indicator provides good UX and maintains consistency with the name input structure.
| <div className="space-y-2"> | ||
| <Label htmlFor="suite-name" className="text-sm font-medium"> | ||
| Test Suite Name <span className="text-destructive">*</span> | ||
| </Label> | ||
| <Input | ||
| id="suite-name" | ||
| type="text" | ||
| value={editedSuiteName} | ||
| onChange={(event) => onEditedSuiteNameChange(event.target.value)} | ||
| onBlur={onSuiteNameBlur} | ||
| onKeyDown={onSuiteNameKeyDown} | ||
| value={suiteName} | ||
| onChange={(event) => onSuiteNameChange(event.target.value)} | ||
| placeholder="e.g., Weather API Integration Tests" | ||
| className={cn( | ||
| "text-lg font-semibold", | ||
| showNameError && | ||
| !suiteName.trim() && | ||
| "border-destructive focus-visible:ring-destructive", | ||
| )} | ||
| autoFocus | ||
| className="w-full px-3 py-2 text-lg font-semibold border border-input rounded-md focus:outline-none focus:ring-2 focus:ring-ring bg-background" | ||
| placeholder="New Test Suite" | ||
| /> | ||
| ) : ( | ||
| <Button | ||
| variant="ghost" | ||
| onClick={onSuiteNameClick} | ||
| className="w-full justify-start px-3 py-2 h-auto text-lg font-semibold hover:bg-accent border border-transparent hover:border-input rounded-md" | ||
| > | ||
| {suiteName || ( | ||
| <span className="text-muted-foreground">New Test Suite</span> | ||
| )} | ||
| </Button> | ||
| )} | ||
| <Textarea | ||
| value={suiteDescription} | ||
| onChange={(event) => onSuiteDescriptionChange(event.target.value)} | ||
| rows={3} | ||
| placeholder="What does this suite cover?" | ||
| className="resize-none" | ||
| /> | ||
| {showNameError && !suiteName.trim() && ( | ||
| <p className="text-sm text-red-600 font-medium"> | ||
| Please provide a name for this test suite | ||
| </p> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Critical: Add ARIA attributes for accessibility compliance.
The validation feedback lacks semantic markup required by WCAG Success Criterion 3.3.1. Screen readers cannot associate the error message with the input or identify the error state.
Apply this diff to add proper ARIA attributes:
+ {showNameError && !suiteName.trim() && (
+ <p id="suite-name-error" className="text-sm text-destructive font-medium">
+ Please provide a name for this test suite
+ </p>
+ )}
</div>
<div className="space-y-2">
<Label htmlFor="suite-name" className="text-sm font-medium">
Test Suite Name <span className="text-destructive">*</span>
</Label>
<Input
id="suite-name"
type="text"
value={suiteName}
onChange={(event) => onSuiteNameChange(event.target.value)}
placeholder="e.g., Weather API Integration Tests"
+ required
+ aria-required="true"
+ aria-invalid={showNameError && !suiteName.trim()}
+ aria-describedby={showNameError && !suiteName.trim() ? "suite-name-error" : undefined}
className={cn(
"text-lg font-semibold",
showNameError &&
!suiteName.trim() &&
"border-destructive focus-visible:ring-destructive",
)}
autoFocus
/>
- {showNameError && !suiteName.trim() && (
- <p className="text-sm text-red-600 font-medium">
- Please provide a name for this test suite
- </p>
- )}Additional improvements:
- Move error message above input for better flow (error → field pattern)
- Change
text-red-600totext-destructivefor consistency - Add
requiredandaria-requiredfor semantic required field indication - Add unique
idto error message foraria-describedbyreference
Based on external tools context about WCAG requirements for error identification and assistive technology support.
Committable suggestion skipped: line range outside the PR's diff.
2 participants
Changes:
Fix: #941
Demo:
Screen.Recording.2025-11-24.at.1.52.54.PM.mov