feat: add agreement title and nickname frontend validation#5670
feat: add agreement title and nickname frontend validation#5670weimiao67 wants to merge 7 commits into
Conversation
…frontend validation
josbell
left a comment
There was a problem hiding this comment.
Review: Request Changes
After deep investigation of the implementation, I found 2 critical issues that need to be fixed before merge. I also validated several concerns raised by automated review and found them to be false positives.
✅ Issues That Need Fixing
1. CRITICAL: Validation Bypass Due to Incomplete Dependency Array
Location: frontend/src/components/Agreements/AgreementEditor/AgreementEditForm.hooks.js:274-279
Issue: The useEffect that re-checks title uniqueness when agreementType changes is missing agreementTitle in its dependency array:
React.useEffect(() => {
if (agreementType && agreementTitle) {
checkUniqueOnBlur("name", agreementTitle);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [agreementType]); // ❌ agreementTitle used but not in depsImpact: Users can bypass uniqueness validation by editing the title without changing the agreement type:
- User types "Agreement A" → uniqueness check runs ✓
- User changes to "Duplicate Title" (without changing type) → no check runs ✗
- Form submits with duplicate title, backend rejects it
This defeats the core purpose of this PR (early duplicate detection).
Fix:
React.useEffect(() => {
if (agreementType && agreementTitle) {
checkUniqueOnBlur.cancel(); // Also fix issue #2
checkUniqueOnBlur("name", agreementTitle);
}
}, [agreementType, agreementTitle, checkUniqueOnBlur]); // ✅ Complete deps2. Race Condition: Missing Debounce Cancellation on Type Change
Location: Same useEffect at lines 274-279
Issue: When agreementType changes, pending debounced uniqueness checks aren't cancelled before triggering a new check.
Impact: If a user rapidly changes agreement types, the old debounced call (300ms delay) may execute after the new type is selected, showing stale validation errors for the wrong type.
Fix: Add checkUniqueOnBlur.cancel(); before the new check (included in Fix #1 above).
❌ False Positives (No Action Needed)
I also investigated several other concerns and found them to be invalid:
✅ Case-Sensitivity Is Consistent
- Claim: Frontend/backend case-sensitivity mismatch for title
- Reality: All three layers (DB, backend query, frontend) use case-insensitive comparison for title via
lower()/.toLowerCase(). No mismatch exists.
✅ Vest v6 Pattern Is Correct
- Claim: Conditional
if (fieldName) { only(fieldName); }violates Vest v6 patterns - Reality: This is actually the safer pattern. The CLAUDE.md warning is about passing the data object to
only(), not about conditional usage.
✅ Nickname Case-Sensitivity Is By Design
- All layers consistently treat nickname as case-sensitive (unlike title which is case-insensitive). This is working as designed.
Summary
Please fix the 2 issues in the useEffect at lines 274-279:
- Add complete dependency array:
[agreementType, agreementTitle, checkUniqueOnBlur] - Add
checkUniqueOnBlur.cancel();before calling the debounced function
These are critical to ensuring the uniqueness validation works correctly in all user workflows.
|
@josbell Thanks for the review and comments! I looked into both.
Let me know if I missed anything. |
What changed
Adds inline uniqueness validation for agreement title and nickname fields in the create/edit Agreement form. A new backend endpoint (
GET /agreements/check-unique) checks whether a candidate value is already in use, respecting existing DB constraints (case-insensitive name scoped by agreement_type, globally unique nick_name). The frontend calls this endpoint on blur and shows an inline error if a duplicate is detected. The check is debounced when agreement_type changes to avoid rapid API calls.Also adds
onBlursupport to the sharedInputUI component.Issue
#4636
How to test
A11y impact
Screenshots
Definition of Done Checklist