Upgrading old UI components with new ones #10049
Conversation
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🔭 Outside diff range comments (1)
src/pages/PublicAppointments/PatientRegistration.tsx (1)
Line range hint
89-89: Improve date validation in the schema.The current schema validation for
date_of_birthneeds improvement:
- Remove the string type to maintain type consistency.
- Add validation for minimum date.
Apply this diff to improve the schema:
-date_of_birth: z.date().or(z.string()).optional(), +date_of_birth: z.date() + .optional() + .refine( + (date) => { + if (!date) return true; + const minDate = new Date(); + minDate.setFullYear(minDate.getFullYear() - 150); + return date >= minDate && date <= new Date(); + }, + { message: t("invalid_date_range") } + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/PublicAppointments/PatientRegistration.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/pages/PublicAppointments/PatientRegistration.tsx (2)
11-11: LGTM!The import statement for the new DatePicker component follows the project's conventions.
Line range hint
193-193: Verify date formatting in form submission.The
dateQueryStringutility is used to format the date, but there's no validation to ensure the date is properly formatted.Run this script to verify the implementation of
dateQueryString:
| <DatePicker | ||
| date={ | ||
| field.value ? new Date(field.value) : undefined | ||
| } | ||
| onChange={(dateObj: { | ||
| name: string; | ||
| value: Date; | ||
| }) => { | ||
| if (dateObj?.value instanceof Date) { | ||
| field.onChange(dateObj.value.toISOString()); | ||
| } else { | ||
| field.onChange(null); | ||
| } | ||
| }} | ||
| disableFuture | ||
| min={new Date(1900, 0, 1)} | ||
| className="-mb-6" | ||
| onChange={(date) => field.onChange(date)} |
There was a problem hiding this comment.
Add date validation and improve type safety.
The DatePicker implementation needs several improvements:
- Add validation to prevent future dates (previously handled by
disableFuture). - Add type validation for the date conversion.
- Sanitize the date input before form mutation.
- Add accessibility attributes.
Apply this diff to address these issues:
<DatePicker
- date={field.value ? new Date(field.value) : undefined}
- onChange={(date) => field.onChange(date)}
+ date={field.value ? new Date(field.value) : undefined}
+ onChange={(date) => {
+ if (date && date > new Date()) {
+ return;
+ }
+ field.onChange(date);
+ }}
+ disableFuture
+ aria-label={t("date_of_birth")}
/>Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/ui/date-picker.tsx (1)
48-48: Consider adding aria-label for disabled dates.The disabled prop is correctly passed to the Calendar component, but for better accessibility, consider adding an aria-label to explain why certain dates are disabled.
<Calendar mode="single" selected={date} onSelect={(date) => { onChange?.(date); setOpen(false); }} initialFocus - disabled={disabled} + disabled={disabled} + disabledDayAriaLabel="This date is not available for selection" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/date-picker.tsx(2 hunks)src/pages/PublicAppointments/PatientRegistration.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/PublicAppointments/PatientRegistration.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/components/ui/date-picker.tsx (2)
19-19: LGTM! Well-typed disabled prop.The disabled prop is correctly typed as an optional function that takes a Date and returns a boolean, following TypeScript best practices.
Line range hint
22-51: Verify date handling in patient registration.Since this component is replacing DateFormField in the patient registration process, ensure that:
- The date format matches the expected format in the backend API
- The disabled function correctly prevents selection of future dates for date of birth
- The component handles null/undefined values gracefully
✅ Verification successful
Date handling implementation verified successfully
The DatePicker component in the patient registration process correctly:
- Prevents selection of future dates and dates before 1900
- Handles null/undefined values with a "Pick a date" placeholder
- Uses standard Date objects for API compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the date format and validation in the patient registration component rg -A 10 "DatePicker.*date.*birth" --type ts --type tsx # Check for any date format constants or utilities rg "date.*format" --type ts --type tsxLength of output: 154
Script:
#!/bin/bash # Find DatePicker usage in components ast-grep --pattern 'DatePicker date={$_} onChange={$_}' # Find patient registration form components rg -l "registration.*form|form.*registration" # Look for Calendar component implementation ast-grep --pattern 'Calendar mode="single" $_' # Search for API calls with dates rg -l "api.*patient|patient.*api"Length of output: 444
Script:
#!/bin/bash # Find DatePicker usage with broader pattern rg -l "DatePicker" # Check date formats in API files rg -A 5 "date.*birth|birth.*date" src/types/**/*Api.ts src/Utils/request/api.tsx # Find Calendar component usage and implementation rg -l "Calendar.*mode|mode.*Calendar" # Look for date-related types rg -A 3 "interface.*date|type.*date" src/types/Length of output: 1471
Script:
#!/bin/bash # Check PatientRegistration implementation rg -A 10 "DatePicker" src/pages/PublicAppointments/PatientRegistration.tsx # Look for birth date validation rg -A 5 "birth.*valid|valid.*birth" # Check patient API request formatting rg -A 10 "patient.*create|create.*patient"Length of output: 22492
| } | ||
|
|
||
| export function DatePicker({ date, onChange }: DatePickerProps) { | ||
| export function DatePicker({ date, onChange, disabled }: DatePickerProps) { |
There was a problem hiding this comment.
💡 Codebase verification
Add JSDoc documentation for the DatePicker component and its props
The component lacks documentation for its props, particularly the disabled prop's behavior. Please add JSDoc documentation to explain that the disabled prop is a function that returns true to disable a specific date.
Example documentation:
/**
* A date picker component that allows selecting a date from a calendar.
*/
interface DatePickerProps {
/** The currently selected date */
date?: Date;
/** Callback fired when the date changes */
onChange?: (date?: Date) => void;
/** Function to determine if a date should be disabled. Return true to disable the date. */
disabled?: (date: Date) => boolean;
}🔗 Analysis chain
Verify disabled prop documentation.
While the implementation looks good, ensure that the component's documentation or comments describe the expected behavior of the disabled function (e.g., return true to disable a date).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's any documentation for the DatePicker component
rg -B 5 -A 1 "DatePicker" --type ts --type tsx | grep -i "doc\|comment"
Length of output: 99
Script:
#!/bin/bash
# Search for DatePicker without file type filtering
rg -B 5 -A 1 "DatePicker"
# Use ast-grep to find interface/type definitions and any JSDoc
ast-grep --pattern 'interface DatePickerProps {
$$$
}'
# Also search for any type definition
ast-grep --pattern 'type DatePickerProps = {
$$$
}'
# Look for any JSDoc comments before DatePicker
ast-grep --pattern '/**
$$$
*/
$_DatePicker'
Length of output: 15231
|
@NikhilA8606 Is this work in progress 🤔 Only seeing DateFormField here. Or are you planning for new PRs for each component? |
|
I'm currently working on this PR |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/ui/datefield.tsx (1)
6-9: Consider adding validation props for date constraints.The component could benefit from additional props to handle date validation constraints (min/max date, disabled dates, etc.).
interface DateFieldProps { value?: string; onChange?: (value: string) => void; + minDate?: string; + maxDate?: string; + disabledDates?: string[]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Patient/PatientRegistration.tsx(2 hunks)src/components/ui/datefield.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Patient/PatientRegistration.tsx (1)
516-519: LGTM! Clean integration of the DateField component.The DateField component is properly integrated with form control and handles value/onChange correctly.
| <Input | ||
| type="number" | ||
| placeholder="DD" | ||
| value={day} | ||
| onChange={handleDayChange} | ||
| data-cy="dob-day-input" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add min/max attributes to input fields.
The numeric inputs should have min/max constraints to prevent invalid values.
<Input
type="number"
placeholder="DD"
+ min="1"
+ max="31"
value={day}
onChange={handleDayChange}
data-cy="dob-day-input"
/>
<Input
type="number"
placeholder="MM"
+ min="1"
+ max="12"
value={month}
onChange={handleMonthChange}
data-cy="dob-month-input"
/>
<Input
type="number"
placeholder="YYYY"
+ min="1900"
+ max={new Date().getFullYear()}
value={year}
onChange={handleYearChange}
data-cy="dob-year-input"
/>Also applies to: 51-57, 62-68
| const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newDay = e.target.value; | ||
| if (onChange) { | ||
| onChange(`${year || ""}-${month || ""}-${newDay}`); | ||
| } | ||
| }; | ||
|
|
||
| const handleMonthChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newMonth = e.target.value; | ||
| if (onChange) { | ||
| onChange(`${year || ""}-${newMonth}-${day || ""}`); | ||
| } | ||
| }; | ||
|
|
||
| const handleYearChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newYear = e.target.value; | ||
| if (onChange) { | ||
| onChange(`${newYear}-${month || ""}-${day || ""}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add date validation in the handlers.
The handlers should validate the input values to ensure they form a valid date (e.g., prevent 31st for months with 30 days).
const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const newDay = e.target.value;
+ if (Number(newDay) > 31 || Number(newDay) < 1) return;
if (onChange) {
onChange(`${year || ""}-${month || ""}-${newDay}`);
}
};
const handleMonthChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const newMonth = e.target.value;
+ if (Number(newMonth) > 12 || Number(newMonth) < 1) return;
if (onChange) {
onChange(`${year || ""}-${newMonth}-${day || ""}`);
}
};
const handleYearChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const newYear = e.target.value;
+ if (Number(newYear) < 1900 || Number(newYear) > new Date().getFullYear()) return;
if (onChange) {
onChange(`${newYear}-${month || ""}-${day || ""}`);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newDay = e.target.value; | |
| if (onChange) { | |
| onChange(`${year || ""}-${month || ""}-${newDay}`); | |
| } | |
| }; | |
| const handleMonthChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newMonth = e.target.value; | |
| if (onChange) { | |
| onChange(`${year || ""}-${newMonth}-${day || ""}`); | |
| } | |
| }; | |
| const handleYearChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newYear = e.target.value; | |
| if (onChange) { | |
| onChange(`${newYear}-${month || ""}-${day || ""}`); | |
| } | |
| }; | |
| const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newDay = e.target.value; | |
| if (Number(newDay) > 31 || Number(newDay) < 1) return; | |
| if (onChange) { | |
| onChange(`${year || ""}-${month || ""}-${newDay}`); | |
| } | |
| }; | |
| const handleMonthChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newMonth = e.target.value; | |
| if (Number(newMonth) > 12 || Number(newMonth) < 1) return; | |
| if (onChange) { | |
| onChange(`${year || ""}-${newMonth}-${day || ""}`); | |
| } | |
| }; | |
| const handleYearChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newYear = e.target.value; | |
| if (Number(newYear) < 1900 || Number(newYear) > new Date().getFullYear()) return; | |
| if (onChange) { | |
| onChange(`${newYear}-${month || ""}-${day || ""}`); | |
| } | |
| }; |
| export default function DateField({ value = "", onChange }: DateFieldProps) { | ||
| const [year, month, day] = value.split("-"); | ||
| const { t } = useTranslation(); |
There was a problem hiding this comment.
Add validation for the date string format.
The value.split("-") operation assumes a valid date string format. Add validation to handle invalid or malformed input.
- const [year, month, day] = value.split("-");
+ const [year, month, day] = value?.match(/^\d{4}-\d{2}-\d{2}$/) ? value.split("-") : ["", "", ""];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default function DateField({ value = "", onChange }: DateFieldProps) { | |
| const [year, month, day] = value.split("-"); | |
| const { t } = useTranslation(); | |
| export default function DateField({ value = "", onChange }: DateFieldProps) { | |
| const [year, month, day] = value?.match(/^\d{4}-\d{2}-\d{2}$/) ? value.split("-") : ["", "", ""]; | |
| const { t } = useTranslation(); |
| interface DateFieldProps { | ||
| value?: string; | ||
| onChange?: (value: string) => void; | ||
| } |
There was a problem hiding this comment.
can we stick with Date type instead of string in the prop's interface for value and onChange?
| placeholder="DD" | ||
| value={day} | ||
| onChange={handleDayChange} | ||
| data-cy="dob-day-input" |
There was a problem hiding this comment.
forgot to remove this?
| data-cy="dob-day-input" |
| <Input | ||
| type="number" | ||
| placeholder="DD" | ||
| value={day} | ||
| onChange={handleDayChange} | ||
| data-cy="dob-day-input" | ||
| /> |
There was a problem hiding this comment.
can we restrict the individual inputs to it's respective possible values?
day - min: 1, max: 31, similarly for month and year too respectively.
| <Input | |
| type="number" | |
| placeholder="DD" | |
| value={day} | |
| onChange={handleDayChange} | |
| data-cy="dob-day-input" | |
| /> | |
| <Input | |
| type="number" | |
| placeholder="DD" | |
| value={day} | |
| onChange={handleDayChange} | |
| min={0} | |
| max={31} | |
| data-cy="dob-day-input" | |
| /> |
| const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newDay = e.target.value; | ||
| if (onChange) { | ||
| onChange(`${year || ""}-${month || ""}-${newDay}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
the implementation of date input in existing PatientRegistration form had auto focus change once a valid value is inputted, can we preserve that logic in this too?
ref:
care_fe/src/components/Patient/PatientRegistration.tsx
Lines 570 to 580 in 23806f6
| interface DateFieldProps { | ||
| value?: string; | ||
| onChange?: (value: string) => void; | ||
| } |
There was a problem hiding this comment.
let's also add disabled: boolean and id: string props too.
id is required for cypress tests to recognise and use these inputs.
There was a problem hiding this comment.
What is the need of disabled props is here?
@rithviknishad
There was a problem hiding this comment.
let's follow similar file naming conventions on how other reusable components are named here.
date-field.tsx instead of datefield.tsx.
I'd go with date-text-input.tsx since that'd more confidently convey what the component exactly is.
|
👋 Hi, @NikhilA8606, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
Replaced DateFormField with DatePicker ui component
DatePickercomponent for date of birth selection in patient registrationThese are the components to be replaced.
Merge Checklist
@rithviknishad
Summary by CodeRabbit
New Features
DateFieldcomponent for streamlined date of birth input in the patient registration form.DatePickercomponent with newdisabledfunctionality for improved date selection flexibility.Bug Fixes