DOB Values Limit in patient registration form #9735
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Patient/PatientRegistration.tsx (2)
458-464: Consider handling partial date states for the day field.Clamping day values between 1 and 31 is helpful, but if the user deletes the field contents or enters partial digits, it will produce intermediate states like “YYYY-MM-” (missing day). In some edge cases, these partial states might be confused with a valid date. Consider adopting a unified approach (e.g. a single date picker or a fully validated [DD-MM-YYYY] input) to handle partial states more gracefully.
477-483: Consider extracting repeated logic for day/month handlers into a helper function.The month field’s clamping logic mirrors the day field approach. Extracting common logic for day/month updates into a helper can improve maintainability, reduce duplication, and simplify testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Patient/PatientRegistration.tsx(3 hunks)
🔇 Additional comments (1)
src/components/Patient/PatientRegistration.tsx (1)
496-504: Verify numeric clamping for the year field.Although the HTML attributes
min={1900}andmax={new Date().getFullYear()}visually guide many browsers, they don’t fully prevent out-of-range values (e.g., 1800 or 3000) in less strict or older browsers. Consider adding a numeric clamp (similar to day/month) or displaying a validation error if the year is outside [1900, currentYear].Run the following script to search for any references to year validations or test coverage across the codebase:
✅ Verification successful
Let me search for more specific validation patterns.
Let me search for the specific validation in the patient registration component.
Let me try a different search pattern to find the validation logic.
Let me try one final search pattern to locate the validation logic.
The year input already has proper numeric clamping.
The code shows that the year input field has both
minandmaxattributes properly set:
min={1900}: Prevents years before 1900max={new Date().getFullYear()}: Dynamically sets the maximum to the current yearAdditionally, there's a length validation that only allows changes when
value.length <= 4, preventing invalid year formats. The HTML5 number input type also provides built-in validation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to year validations or existing date-of-birth tests rg -A 5 "year"Length of output: 42745
Script:
#!/bin/bash # Search for date validation patterns and year-related constraints rg -A 5 "(min|max).*year|year.*validation" # Search for specific year validation in the patient registration component ast-grep --pattern 'if ($$$year$$$) { $$$ }'Length of output: 4994
Script:
#!/bin/bash # Search for date of birth validation in PatientRegistration.tsx rg -A 10 "date_of_birth.*onChange" src/components/Patient/PatientRegistration.tsx # Search for any min/max attributes on the year input rg "min.*max.*YYYY|YYYY.*min.*max" src/components/Patient/PatientRegistration.tsxLength of output: 167
Script:
#!/bin/bash # Search for the input field and its validation in PatientRegistration.tsx rg -A 10 "type=\"number\".*placeholder=\"YYYY\"" src/components/Patient/PatientRegistration.tsx # Search for any validation functions or error handling related to date/year rg -A 5 "setForm.*date_of_birth|errors\[.*date_of_birth\]" src/components/Patient/PatientRegistration.tsxLength of output: 201
Script:
#!/bin/bash # Search for the year input field and surrounding context rg -B 5 -A 10 "placeholder=\"YYYY\"" src/components/Patient/PatientRegistration.tsx # Search for any form validation schema or rules ast-grep --pattern 'const $_ = z.object({ $$$ date_of_birth: $$$, $$$ })'Length of output: 998
|
The implementation doesn't look clean. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Patient/PatientRegistration.tsx (3)
284-290: Consider defining monthDays as a constant.The monthDays array should be defined as a constant outside the function to avoid recreation on each function call.
+const MONTH_DAYS = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]; + const isValidDate = (day: number, month: number, year: number): boolean => { - const monthDays = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]; if (month === 2) { return day <= (isLeapYear(year) ? 29 : 28); } - return day <= monthDays[month - 1]; + return day <= MONTH_DAYS[month - 1]; };
292-365: Consider refactoring handleDateOfBirth for better maintainability.The function handles multiple responsibilities and contains nested conditionals. Consider breaking it down into smaller, focused functions:
- Separate validation logic
- Separate error message generation
- Use early returns to reduce nesting
+const validateDatePart = ( + part: "day" | "month" | "year", + value: number, + min: number, + max: number +): string => { + if (part === "year" && (value < min || value > max)) { + return t("INVALID_YEAR", { min: MIN_YEAR, max: MAX_YEAR }); + } + if (value < min || value > max) { + return t(`INVALID_${part.toUpperCase()}`); + } + return ""; +}; + +const validateCompleteDate = ( + day: number, + month: number, + year: number +): string => { + if (!isValidDate(day, month, year)) { + return t("INVALID_DATE"); + } + + const selectedDate = new Date(year, month - 1, day); + if (selectedDate > new Date()) { + return t("DATE_NOT_ALLOWED"); + } + + return ""; +}; + const handleDateOfBirth = ( part: "day" | "month" | "year", value: string, min: number, max: number, maxLength: number, ) => { if (value.length > maxLength) { value = value.slice(0, maxLength); } const numericValue = Number(value.slice(0, maxLength)); + let errorMessage = validateDatePart(part, numericValue, min, max); + let formattedValue = errorMessage ? "" : value; setForm((prevState) => { const [currentYear, currentMonth, currentDay] = prevState.date_of_birth?.split("-") || ["", "", ""]; - let formattedValue = value; - let errorMessage = ""; - - if (part === "day") { - if (numericValue < min || numericValue > max) { - formattedValue = ""; - errorMessage = t("INVALID_DAY"); - } - } else if (part === "month") { - if (numericValue < min || numericValue > max) { - formattedValue = ""; - errorMessage = t("INVALID_MONTH"); - } - } else if (part === "year") { - if (formattedValue.length === 4) { - const yearValue = Number(formattedValue); - - if (yearValue < min || yearValue > max) { - formattedValue = ""; - errorMessage = t("INVALID_YEAR", { min: MIN_YEAR, max: MAX_YEAR }); - } - } - } - const updatedDay = part === "day" ? formattedValue : currentDay; const updatedMonth = part === "month" ? formattedValue : currentMonth; const updatedYear = part === "year" ? formattedValue : currentYear; if (updatedDay && updatedMonth && updatedYear) { - const day = Number(updatedDay); - const month = Number(updatedMonth); - const year = Number(updatedYear); - - if (!isValidDate(day, month, year)) { - errorMessage = t("INVALID_DATE"); - } else { - const today = new Date(); - const selectedDate = new Date(year, month - 1, day); - - if (selectedDate > today) { - errorMessage = t("DATE_NOT_ALLOWED"); - } - } + const completeDateError = validateCompleteDate( + Number(updatedDay), + Number(updatedMonth), + Number(updatedYear) + ); + errorMessage = completeDateError || errorMessage; }
539-583: Consider UX improvements for date input fields.While the implementation is clean, consider these improvements:
- Use
type="text" inputMode="numeric" pattern="[0-9]*"instead oftype="number"to prevent the spinner buttons and ensure better mobile experience- Add ARIA labels for better accessibility
<Input - type="number" + type="text" + inputMode="numeric" + pattern="[0-9]*" placeholder={placeholders[key]} + aria-label={t(`enter_${key}`)} value={ form.date_of_birth?.split("-")[ ["year", "month", "day"].indexOf(key) ] || "" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(2 hunks)src/components/Patient/PatientRegistration.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
🔇 Additional comments (3)
public/locale/en.json (1)
32-32: Well-defined localization messages for date validation!The error messages are clear, specific, and match the validation logic in the code:
- Invalid date format
- Day/Month/Year range validation
- Future date validation
Also applies to: 74-77
src/components/Patient/PatientRegistration.tsx (2)
83-84: LGTM! Well-defined year boundaries.The year range constants are appropriately defined with reasonable bounds.
280-282: LGTM! Correct leap year implementation.The leap year calculation follows the standard algorithm: divisible by 4 AND (not divisible by 100 OR divisible by 400).
|
@bodhish @rithviknishad Can you please review updated changes |
|
👋 Hi, @Rishith25, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@Rishith25 What's the update here? As discussed in the call, use dayjs to simplify the logic. |
|
@Jacobjeevan I will update it |
|
@bodhish @Jacobjeevan @rithviknishad
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/Patient/PatientRegistration.tsx (2)
296-297: Move constants to the top of the file.Consider moving these constants to the top of the file with other constant declarations for better code organization and maintainability.
// Move to top of file with other constants - const MIN_YEAR = 1900; - const MAX_YEAR = new Date().getFullYear(); // At the top of the file after imports +const MIN_YEAR = 1900; +const MAX_YEAR = new Date().getFullYear();
525-571: Improve date input accessibility and user experience.The date input implementation could be enhanced with better accessibility and user experience features.
Consider these improvements:
{["day", "month", "year"].map((part) => { const key = part as "day" | "month" | "year"; const placeholders = { day: "DD", month: "MM", year: "YYYY", }; const maxLengths = { day: 2, month: 2, year: 4 }; const limits = { day: { min: 1, max: 31 }, month: { min: 1, max: 12 }, year: { min: MIN_YEAR, max: MAX_YEAR }, }; return ( <div className="flex-1" key={key}> <Label className="mb-2"> {t(key)} <span className="text-red-500">*</span> </Label> <Input placeholder={placeholders[key]} type="text" + aria-label={t(key)} + inputMode="numeric" + pattern="[0-9]*" value={ form.date_of_birth?.split("-")[ ["year", "month", "day"].indexOf(key) ] || "" } maxLength={maxLengths[key]} onChange={(e) => { const value = e.target.value; if ( value.length <= maxLengths[key] && Number(value) <= limits[key].max ) { handleDateOfBirth( key, value, limits[key].min, limits[key].max, maxLengths[key], ); + // Auto-focus next field when current field is complete + if (value.length === maxLengths[key]) { + const nextField = { + day: "month", + month: "year", + year: null + }[key]; + if (nextField) { + const nextInput = document.querySelector( + `input[aria-label="${t(nextField)}"]` + ) as HTMLInputElement; + nextInput?.focus(); + } + } } }} + onKeyDown={(e) => { + // Handle backspace on empty field to focus previous field + if (e.key === "Backspace" && !e.currentTarget.value) { + const prevField = { + year: "month", + month: "day", + day: null + }[key]; + if (prevField) { + const prevInput = document.querySelector( + `input[aria-label="${t(prevField)}"]` + ) as HTMLInputElement; + prevInput?.focus(); + } + } + }} /> </div> ); })}public/locale/en.json (1)
31-33: Standardize translation key naming and improve message clarity.The date validation messages could be more user-friendly and follow a consistent naming pattern.
Consider these improvements:
- "DATE_FORMAT": "Note: Please enter the date in DD-MM-YYYY format.", - "DATE_NOT_ALLOWED": "Date must be on or before today's date. Please correct the date.", - "INVALID_DATE": "Invalid day for the selected month and year. Please correct the date.", + "DATE__FORMAT": "Please enter the date as DD-MM-YYYY", + "DATE__FUTURE_NOT_ALLOWED": "The date cannot be in the future. Please enter a date on or before today.", + "DATE__INVALID": "The date is not valid for the selected month and year. Please check and try again.", + "DATE__INVALID_DAY": "Please enter a valid day (1-31)", + "DATE__INVALID_MONTH": "Please enter a valid month (1-12)", + "DATE__INVALID_YEAR": "Please enter a valid year (1900-present)"Also applies to: 76-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(2 hunks)src/components/Patient/PatientRegistration.tsx(4 hunks)
github-actions
bot
added
merge conflict
|
👋 Hi, @Rishith25, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Patient/PatientRegistration.tsx (1)
585-587: Consider showing format hint proactively.The date format hint is currently only shown when there's an error. Consider displaying it proactively below the input fields to guide users before they make mistakes.
<div className="flex items-center gap-2"> {/* ... date input fields ... */} </div> +<p className="mt-2 text-xs text-gray-500"> + {t("DATE_FORMAT")} +</p> {errors["date_of_birth"] && ( <div className="mt-1" data-input-error> {errors["date_of_birth"].map((error, i) => ( <div key={i} className="text-red-500 text-xs"> {error} </div> ))} - <p className="mt-2 text-xs text-warning-500"> - {t("DATE_FORMAT")} - </p> </div> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(2 hunks)src/components/Patient/PatientRegistration.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: CodeQL-Build
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/components/Patient/PatientRegistration.tsx (3)
106-107: LGTM! Well-defined date validation boundaries.The constants provide reasonable bounds for date validation with MIN_YEAR set to 1900 and MAX_YEAR dynamically set to the current year.
529-576: LGTM! Well-structured date input implementation.The date input UI implementation is clean and maintainable with:
- Proper separation of day, month, and year inputs
- Clear placeholders and validation limits
- Type-safe implementation
299-339: 🛠️ Refactor suggestionEnhance date validation robustness.
The date validation function has several areas for improvement:
- Add input sanitization for non-numeric values
- Use a callback in setState to avoid potential race conditions
- Add more specific error messages for different validation cases
Consider this improved implementation:
const handleDateOfBirth = ( part: "day" | "month" | "year", value: string, min: number, max: number, maxLength: number, ) => { + // Sanitize input to allow only numbers + const sanitizedValue = value.replace(/[^0-9]/g, ''); const trimmedValue = value.slice(0, maxLength); setForm((prevState) => { const [currentYear, currentMonth, currentDay] = prevState.date_of_birth?.split("-") || ["", "", ""]; const updatedDateParts = { day: part === "day" ? trimmedValue : currentDay, month: part === "month" ? trimmedValue : currentMonth, year: part === "year" ? trimmedValue : currentYear, }; const { day, month, year } = updatedDateParts; let errorMessage = ""; if (day && month && year && year.length === 4) { + // Validate individual parts before creating date + if (parseInt(month) > 12) { + errorMessage = t("INVALID_MONTH"); + } else if (parseInt(day) > 31) { + errorMessage = t("INVALID_DAY"); + } else { const dateString = `${year}-${month}-${day}`; const selectedDate = dayjs(dateString, "YYYY-MM-DD", true); if (!selectedDate.isValid()) { errorMessage = t("INVALID_DATE"); } else if (selectedDate.isAfter(dayjs())) { errorMessage = t("DATE_NOT_ALLOWED"); } + } } - setFeErrors((errors) => ({ + setFeErrors((prevErrors) => ({ - ...errors, + ...prevErrors, date_of_birth: errorMessage ? [errorMessage] : [], })); return { ...prevState, date_of_birth: `${year}-${month}-${day}`, }; }); };Likely invalid or redundant comment.
Jacobjeevan
added
waiting for related PR
|
Switching to react hook forms in #9854. Wait for that to get merged and update as necessary. |
|
👋 Hi, @Rishith25, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
rithviknishad
removed
the
waiting for related PR
| value={ | ||
| form.watch("date_of_birth")?.split("-")[2] | ||
| } | ||
| onChange={(e) => { | ||
| form.setValue( | ||
| "date_of_birth", | ||
| `${form.watch("date_of_birth")?.split("-")[0]}-${form.watch("date_of_birth")?.split("-")[1]}-${e.target.value}`, | ||
| `${form.watch("date_of_birth")?.split("-")[0]}-${form.watch("date_of_birth")?.split("-")[1]}-${e.target.value.slice(0, 2)}`, |
There was a problem hiding this comment.
are the slice here and below necessary 🤔
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit