Valueset UI #10590
Valueset UI #10590
Conversation
WalkthroughThis pull request adds new localization keys to the English locale file and refines the routing structure by introducing an Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant AR as AppRouter
participant ADM as AdminRouter
participant AS as AppSidebar
participant AN as AdminNav
U->>AR: Initiates navigation
AR->>ADM: Lookup admin route
ADM-->>AR: Return adminPages
AR->>AS: Pass sidebar context (ADMIN)
AS->>AN: Render admin-specific navigation
AN-->>U: Display admin dashboard
sequenceDiagram
participant U as User
participant CE as CodingEditor
participant API as valuesetApi.lookup
U->>CE: Enters code and selects system
U->>CE: Clicks Verify button
CE->>API: Request code verification
API-->>CE: Return display value or error
CE->>U: Show success/error toast
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 (
|
Deploying care-fe with
|
| Latest commit: |
d4f36fd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0679d7bf.care-fe.pages.dev |
| Branch Preview URL: | https://valueset-ui.care-fe.pages.dev |
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ 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: 7
🧹 Nitpick comments (21)
src/types/questionnaire/question.ts (2)
48-53: LGTM! Consider adding JSDoc comments.The addition of the optional
codeproperty toAnswerOptionis a good enhancement that aligns with medical coding standards while maintaining backward compatibility.Consider adding JSDoc comments to document the purpose and usage of the
codeproperty:export interface AnswerOption { value: string; display?: string; initialSelected?: boolean; + /** Optional medical coding information for the answer option */ code?: Code; }
55-59: Consider a more domain-specific interface name.While the interface structure is correct, the name
ObservationTypecould be more specific to indicate its medical context.Consider renaming and adding documentation:
-export interface ObservationType { +/** Represents a medical observation code with its coding system and display text */ +export interface MedicalObservationType { system: string; code: string; display: string; }src/components/ValueSet/ValueSetForm.tsx (3)
35-88: Validate input length constraints or domain limits.While the schema is comprehensive and ensures mandatory fields, consider if any upper-limit constraints or user-friendly patterns (e.g., matching only certain text for
code, limiting maximum length forname, etc.) are appropriate.
102-137: Enhance error handling in thelookupMutation.Currently, the
onErrorcallback simply shows a toast. It might be more helpful to log or display the actual error message returned from the server.- toast.error("Failed to verify code"); + toast.error(error instanceof Error ? error.message : "Failed to verify code");
399-497: Provide user feedback on form submission status.The component disables the submit button while
isSubmittingis true, but there is no visible indication of whether the submission was successful or if an error occurred. Consider adding toast notifications or inline success/error messages for better UX.src/components/Questionnaire/QuestionnaireEditor.tsx (8)
55-55: Remove unused imports if not needed.
ObservationTypeis imported but never referenced. Consider removing this import if the feature is not yet implemented.
95-95: Unused state variable.
observationis declared but never used. Evaluate whether this is a placeholder for future functionality or if it should be removed.
200-200: Consider removing debug console statement.
console.logis helpful for development but can clutter production logs.- console.log("Update Questionnaire", questionnaire);
711-716: Efficient data fetching withvaluesetApi.list.Fetching value sets to populate choice options or structured questions is a good approach. Ensure caching or pagination is used if the number of value sets grows large.
904-911: Include code editor only when relevant.Rendering
CodingEditorfor non-structuredand non-groupquestion types offers flexibility. Consider lazy-loadingCodingEditorif performance is a concern for large forms.
912-955: Leverage separate components for settings.This block configures "Question Settings" (e.g., required, repeatable). For cleaner code, consider extracting into a child component.
957-1020: Clarify data collection toggles.The collection toggles (time, performer, body site, method) are domain-specific. Consider adding popover or help text explaining each toggle’s effect.
1054-1161: Choice-based question answer options.The ability to add and remove custom options is well-implemented. Consider validating or sanitizing user input to enforce consistent display text or unique values.
src/components/Common/DebugPreview.tsx (1)
9-28: Consider adding a max width constraint.The component is well-implemented with proper environment checks and overflow handling. However, for better UI consistency, consider adding a max-width constraint.
- <Card className={className}> + <Card className={`max-w-4xl ${className ?? ''}`}>src/Routers/routes/adminRoutes.tsx (1)
9-17: Consider grouping related routes using nested objects.The routes are well-defined but could benefit from better organization. Consider grouping related routes (questionnaire and valuesets) using nested objects for better maintainability.
const AdminRoutes: AppRoutes = { - "/admin/questionnaire": () => <QuestionnaireList />, - "/admin/questionnaire/create": () => <QuestionnaireEditor />, - "/admin/questionnaire/:id": ({ id }) => <QuestionnaireShow id={id} />, - "/admin/questionnaire/:id/edit": ({ id }) => <QuestionnaireEditor id={id} />, - "/admin/valuesets": () => <ValueSetList />, - "/admin/valuesets/create": () => <ValueSetEditor />, - "/admin/valuesets/:slug/edit": ({ slug }) => <ValueSetEditor slug={slug} />, + questionnaire: { + "/admin/questionnaire": () => <QuestionnaireList />, + "/admin/questionnaire/create": () => <QuestionnaireEditor />, + "/admin/questionnaire/:id": ({ id }) => <QuestionnaireShow id={id} />, + "/admin/questionnaire/:id/edit": ({ id }) => <QuestionnaireEditor id={id} />, + }, + valuesets: { + "/admin/valuesets": () => <ValueSetList />, + "/admin/valuesets/create": () => <ValueSetEditor />, + "/admin/valuesets/:slug/edit": ({ slug }) => <ValueSetEditor slug={slug} />, + }, };src/types/valueset/valuesetApi.ts (1)
12-49: Consider adding error response types for better error handling.The API configuration is well-structured with proper request/response types. However, it would benefit from explicit error response types for better error handling in the UI.
export default { list: { path: "/api/v1/valueset/", method: HttpMethod.GET, TRes: Type<PaginatedResponse<ValuesetBase>>(), + TError: Type<{ message: string; code: string }>(), }, // Add TError to other endpoints similarly } as const;src/types/valueset/valueset.ts (2)
1-5: Consider adding enum for filter operations.The
ValuesetFilterinterface could benefit from using an enum for theopproperty to ensure valid operations.+export enum FilterOperation { + EQUALS = "=", + NOT_EQUALS = "!=", + IN = "in", + NOT_IN = "not-in", +} + export interface ValuesetFilter { - op: string; + op: FilterOperation; value: string; property: string; }
18-21: Consider making arrays optional with default empty arrays.The
ValuesetComposeinterface requires both arrays to be present. Consider making them optional with default empty arrays for better flexibility.interface ValuesetCompose { - exclude: ValuesetInclude[]; - include: ValuesetInclude[]; + exclude?: ValuesetInclude[]; + include?: ValuesetInclude[]; }src/Routers/AppRouter.tsx (1)
92-92: Consider adding a loading state.The fallback to
ErrorPageis immediate. Consider adding a loading state when routes are being resolved.Apply this diff:
- const pages = appPages || adminPages || <ErrorPage />; + const pages = appPages || adminPages || ( + <Suspense fallback={<Loading />}> + <ErrorPage /> + </Suspense> + );src/components/ValueSet/ValueSetList.tsx (2)
43-114: Enhance table accessibility.The table implementation looks clean, but could benefit from accessibility improvements.
-<table className="min-w-full divide-y divide-gray-200"> +<table + className="min-w-full divide-y divide-gray-200" + role="grid" + aria-label="Value Sets">
89-93: Consider adding a tooltip for truncated descriptions.Since the description is truncated, users might need to see the full text.
-<div className="max-w-md truncate text-sm text-gray-900"> +<div + className="max-w-md truncate text-sm text-gray-900" + title={valueset.description}> {valueset.description} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
eslint.config.mjs(1 hunks)public/locale/en.json(4 hunks)src/Routers/AppRouter.tsx(5 hunks)src/Routers/routes/adminRoutes.tsx(1 hunks)src/Routers/routes/questionnaireRoutes.tsx(0 hunks)src/components/Common/DebugPreview.tsx(1 hunks)src/components/Questionnaire/CloneQuestionnaireSheet.tsx(1 hunks)src/components/Questionnaire/CodingEditor.tsx(1 hunks)src/components/Questionnaire/QuestionnaireEditor.tsx(11 hunks)src/components/Questionnaire/QuestionnaireForm.tsx(2 hunks)src/components/Questionnaire/QuestionnaireList.tsx(2 hunks)src/components/Questionnaire/show.tsx(2 hunks)src/components/ValueSet/ValueSetEditor.tsx(1 hunks)src/components/ValueSet/ValueSetForm.tsx(1 hunks)src/components/ValueSet/ValueSetList.tsx(1 hunks)src/components/ui/sidebar/admin-nav.tsx(1 hunks)src/components/ui/sidebar/app-sidebar.tsx(4 hunks)src/pages/UserDashboard.tsx(2 hunks)src/types/questionnaire/question.ts(1 hunks)src/types/valueset/valueset.ts(1 hunks)src/types/valueset/valuesetApi.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/Routers/routes/questionnaireRoutes.tsx
✅ Files skipped from review due to trivial changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (18)
src/components/Questionnaire/CloneQuestionnaireSheet.tsx (1)
67-67:❓ Verification inconclusive
LGTM! Verify admin route consistency.
The navigation path update aligns with the broader changes to move questionnaire management to the admin interface.
Let's verify the consistency of admin routes across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Check for any inconsistencies in questionnaire route patterns # Search for questionnaire route patterns echo "Checking questionnaire route patterns..." rg -A 1 "questionnaire/\$\{.*?\.slug\}" --type ts --type tsx # Search for potential mixed usage of admin and non-admin routes echo "Checking for mixed route usage..." rg "/questionnaire/(?!admin)" --type ts --type tsxLength of output: 553
LGTM – Navigation route update looks aligned with the overall admin refactoring.
The change to the
navigate(/admin/questionnaire/${data.slug})call insrc/components/Questionnaire/CloneQuestionnaireSheet.tsxis consistent with the intended migration of questionnaire management to the admin interface.
- The updated route now follows the expected pattern.
- Automated searches for inconsistent route usage returned errors (due to PCRE2 issues), so please manually verify that no leftover references to the non-admin
/questionnairepath exist elsewhere.src/components/Questionnaire/QuestionnaireEditor.tsx (6)
2-2: Confirm translation usage.The
timport from "i18next" is used primarily within the component. Ensure all user-visible strings that need localization are wrapped with the translation function as needed.
48-48: DebugPreview usage is good.The debug view is a helpful addition for development. Just ensure it’s removed or toggled off in production builds if performance or confidentiality is a concern.
66-66: Valueset API import is consistent.Importing
valuesetApiis consistent with the new usage for listing value sets below.
68-68: CodingEditor import usage.The
CodingEditorimport is properly referenced later.
212-212: Recheck route consistency.Navigating to
/admin/questionnaire/${id}or/admin/questionnaireis valid, but ensure these routes are correctly handled in the admin router definition.
1162-1185: Value set selection for answer options.Allowing a switch between custom list and value sets is helpful. Confirm how you handle user data if they switch from a value set back to custom. Potentially prompt for a fallback or a merge of custom entries.
src/components/ui/sidebar/admin-nav.tsx (1)
1-34: Navigation creation approach looks solid.You’re generating a concise list of admin links. Ensure internationalization covers “Valuesets” or consider using
t("valuesets")for consistency with the rest of the UI.src/components/Common/DebugPreview.tsx (1)
3-7: LGTM! Well-structured props interface.The
DebugPreviewPropsinterface is well-defined with proper TypeScript types and optional properties.src/types/valueset/valueset.ts (1)
64-70: LGTM! Well-defined terminology systems.The terminology systems are well-defined as constant values with proper typing.
eslint.config.mjs (1)
69-69: Verify the impact of relaxing TypeScript rules.Changing
no-unused-varsandno-unused-expressionsfrom "error" to "warn" might lead to code quality issues being missed during development.Please confirm if this change aligns with the team's code quality standards. Consider:
- Will this make it easier to accidentally merge code with unused variables?
- Could this lead to accumulation of dead code over time?
Also applies to: 77-77
src/components/Questionnaire/QuestionnaireList.tsx (1)
44-44: LGTM! Navigation paths updated consistently.The navigation paths have been correctly updated to use the admin prefix, maintaining consistency with the new routing structure.
Also applies to: 71-73
src/Routers/AppRouter.tsx (1)
90-90:❓ Verification inconclusive
Verify sidebar context determination logic.
The sidebar context is determined solely by the presence of
appPages. Consider if there are edge cases where this might not be sufficient.
🏁 Script executed:
#!/bin/bash # Search for other uses of SidebarFor enum to ensure consistent usage rg -A 3 "SidebarFor\." src/Length of output: 1406
Revisit Sidebar Context Determination in AppRouter.tsx
The current logic chooses
SidebarFor.FACILITYwhenappPagesis truthy and defaults toSidebarFor.ADMINotherwise. Our repository search confirms that while Patient-related routes explicitly passSidebarFor.PATIENT, AppRouter only distinguishes between Facility and Admin. Please double-check that relying solely onappPagesis sufficient for your intended behavior—especially considering if there could be cases where bothappPagesandadminPagesare available or additional context might be needed. If extra criteria are required, adjust the condition accordingly.src/components/Questionnaire/CodingEditor.tsx (1)
36-41:❓ Verification inconclusive
Consider adding validation for the initial code value.
When adding a new coding, there's no validation for the initial system value.
🏁 Script executed:
#!/bin/bash # Check if TERMINOLOGY_SYSTEMS is properly exported and has values ast-grep --pattern 'export const TERMINOLOGY_SYSTEMS = $_'Length of output: 381
Action Required: Add Validation for New Coding Entries in CodingEditor
We've verified that
TERMINOLOGY_SYSTEMSis correctly exported (seesrc/types/valueset/valueset.ts), and its first value is consistently used as the default system. However, the component still issues an initial coding object with an empty string for thecode(anddisplay) without any validation. To prevent invalid or unintended values when a new coding is added, please implement validation logic for the initial code value (and consider extending this to the display field as well).
- File:
src/components/Questionnaire/CodingEditor.tsx(lines 36–41)- Suggestion: Add validation to ensure the empty string isn’t accepted inadvertently—e.g., prompt the user or enforce a required format once input is received.
src/components/ui/sidebar/app-sidebar.tsx (1)
129-129: LGTM! Clean integration of admin navigation.The AdminNav component is properly integrated into the sidebar structure.
src/pages/UserDashboard.tsx (1)
65-67: LGTM! Clear and consistent UI update.The navigation changes align well with the new admin routes structure.
src/components/Questionnaire/show.tsx (1)
108-108: LGTM! Navigation paths updated consistently.The navigation paths have been consistently updated to use the new admin context, aligning with the routing system restructuring.
Also applies to: 152-155, 159-159
src/components/Questionnaire/QuestionnaireForm.tsx (1)
14-14: LGTM! Improved modularity with DebugPreview component.The debug preview functionality has been properly encapsulated in a dedicated component, making the code more maintainable and reusable.
Also applies to: 531-535
| function FilterFields({ | ||
| nestIndex, | ||
| type, | ||
| }: { | ||
| nestIndex: number; | ||
| type: "include" | "exclude"; | ||
| }) { | ||
| const form = useForm(); | ||
| const { fields, append, remove } = useFieldArray({ | ||
| control: form.control, | ||
| name: `compose.${type}.${nestIndex}.filter`, | ||
| }); | ||
|
|
||
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center justify-between"> | ||
| <h4 className="text-sm font-medium">Filters</h4> | ||
| <Button | ||
| type="button" | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={() => append({ property: "", op: "", value: "" })} | ||
| > | ||
| <PlusIcon className="h-4 w-4 mr-2" /> | ||
| Add Filter | ||
| </Button> | ||
| </div> | ||
| {fields.map((field, index) => ( | ||
| <div key={field.id} className="flex gap-4 items-start"> | ||
| <FormField | ||
| control={form.control} | ||
| name={`compose.${type}.${nestIndex}.filter.${index}.property`} | ||
| render={({ field }) => ( | ||
| <FormItem className="flex-1"> | ||
| <FormControl> | ||
| <Input {...field} placeholder="Property" /> | ||
| </FormControl> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <FormField | ||
| control={form.control} | ||
| name={`compose.${type}.${nestIndex}.filter.${index}.op`} | ||
| render={({ field }) => ( | ||
| <FormItem className="flex-1"> | ||
| <FormControl> | ||
| <Input {...field} placeholder="Operator" /> | ||
| </FormControl> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <FormField | ||
| control={form.control} | ||
| name={`compose.${type}.${nestIndex}.filter.${index}.value`} | ||
| render={({ field }) => ( | ||
| <FormItem className="flex-1"> | ||
| <FormControl> | ||
| <Input {...field} placeholder="Value" /> | ||
| </FormControl> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <Button | ||
| type="button" | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => remove(index)} | ||
| > | ||
| <TrashIcon className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Use the same form instance to manage filter fields.
In FilterFields, a new form is created via useForm(). This is inconsistent with how ConceptFields reuses the parent's form. If you intend these filter fields to be part of the same data submission, consider passing the parent's form instead.
-function FilterFields({
nestIndex,
type,
}: {
nestIndex: number;
type: "include" | "exclude";
}) {
const form = useForm();
const { fields, append, remove } = useFieldArray({
control: form.control,
name: `compose.${type}.${nestIndex}.filter`,
});
...
}
+function FilterFields({
nestIndex,
type,
parentForm,
}: {
nestIndex: number;
type: "include" | "exclude";
parentForm: ReturnType<typeof useForm<ValuesetFormType>>;
}) {
const { fields, append, remove } = useFieldArray({
control: parentForm.control,
name: `compose.${type}.${nestIndex}.filter`,
});
...
}📝 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.
| function FilterFields({ | |
| nestIndex, | |
| type, | |
| }: { | |
| nestIndex: number; | |
| type: "include" | "exclude"; | |
| }) { | |
| const form = useForm(); | |
| const { fields, append, remove } = useFieldArray({ | |
| control: form.control, | |
| name: `compose.${type}.${nestIndex}.filter`, | |
| }); | |
| return ( | |
| <div className="space-y-4"> | |
| <div className="flex items-center justify-between"> | |
| <h4 className="text-sm font-medium">Filters</h4> | |
| <Button | |
| type="button" | |
| variant="outline" | |
| size="sm" | |
| onClick={() => append({ property: "", op: "", value: "" })} | |
| > | |
| <PlusIcon className="h-4 w-4 mr-2" /> | |
| Add Filter | |
| </Button> | |
| </div> | |
| {fields.map((field, index) => ( | |
| <div key={field.id} className="flex gap-4 items-start"> | |
| <FormField | |
| control={form.control} | |
| name={`compose.${type}.${nestIndex}.filter.${index}.property`} | |
| render={({ field }) => ( | |
| <FormItem className="flex-1"> | |
| <FormControl> | |
| <Input {...field} placeholder="Property" /> | |
| </FormControl> | |
| </FormItem> | |
| )} | |
| /> | |
| <FormField | |
| control={form.control} | |
| name={`compose.${type}.${nestIndex}.filter.${index}.op`} | |
| render={({ field }) => ( | |
| <FormItem className="flex-1"> | |
| <FormControl> | |
| <Input {...field} placeholder="Operator" /> | |
| </FormControl> | |
| </FormItem> | |
| )} | |
| /> | |
| <FormField | |
| control={form.control} | |
| name={`compose.${type}.${nestIndex}.filter.${index}.value`} | |
| render={({ field }) => ( | |
| <FormItem className="flex-1"> | |
| <FormControl> | |
| <Input {...field} placeholder="Value" /> | |
| </FormControl> | |
| </FormItem> | |
| )} | |
| /> | |
| <Button | |
| type="button" | |
| variant="ghost" | |
| size="icon" | |
| onClick={() => remove(index)} | |
| > | |
| <TrashIcon className="h-4 w-4" /> | |
| </Button> | |
| </div> | |
| ))} | |
| </div> | |
| ); | |
| } | |
| function FilterFields({ | |
| nestIndex, | |
| type, | |
| parentForm, | |
| }: { | |
| nestIndex: number; | |
| type: "include" | "exclude"; | |
| parentForm: ReturnType<typeof useForm<ValuesetFormType>>; | |
| }) { | |
| const { fields, append, remove } = useFieldArray({ | |
| control: parentForm.control, | |
| name: `compose.${type}.${nestIndex}.filter`, | |
| }); | |
| return ( | |
| <div className="space-y-4"> | |
| <div className="flex items-center justify-between"> | |
| <h4 className="text-sm font-medium">Filters</h4> | |
| <Button | |
| type="button" | |
| variant="outline" | |
| size="sm" | |
| onClick={() => append({ property: "", op: "", value: "" })} | |
| > | |
| <PlusIcon className="h-4 w-4 mr-2" /> | |
| Add Filter | |
| </Button> | |
| </div> | |
| {fields.map((field, index) => ( | |
| <div key={field.id} className="flex gap-4 items-start"> | |
| <FormField | |
| control={parentForm.control} | |
| name={`compose.${type}.${nestIndex}.filter.${index}.property`} | |
| render={({ field }) => ( | |
| <FormItem className="flex-1"> | |
| <FormControl> | |
| <Input {...field} placeholder="Property" /> | |
| </FormControl> | |
| </FormItem> | |
| )} | |
| /> | |
| <FormField | |
| control={parentForm.control} | |
| name={`compose.${type}.${nestIndex}.filter.${index}.op`} | |
| render={({ field }) => ( | |
| <FormItem className="flex-1"> | |
| <FormControl> | |
| <Input {...field} placeholder="Operator" /> | |
| </FormControl> | |
| </FormItem> | |
| )} | |
| /> | |
| <FormField | |
| control={parentForm.control} | |
| name={`compose.${type}.${nestIndex}.filter.${index}.value`} | |
| render={({ field }) => ( | |
| <FormItem className="flex-1"> | |
| <FormControl> | |
| <Input {...field} placeholder="Value" /> | |
| </FormControl> | |
| </FormItem> | |
| )} | |
| /> | |
| <Button | |
| type="button" | |
| variant="ghost" | |
| size="icon" | |
| onClick={() => remove(index)} | |
| > | |
| <TrashIcon className="h-4 w-4" /> | |
| </Button> | |
| </div> | |
| ))} | |
| </div> | |
| ); | |
| } |
| <div className="space-y-4"> | ||
| <div className="grid grid-cols-2 gap-4"> | ||
| <div> | ||
| <Label>Structured Type</Label> | ||
| <Label>Type</Label> | ||
| <Select | ||
| value={structured_type ?? "allergy_intolerance"} | ||
| onValueChange={(val: StructuredQuestionType) => | ||
| updateField("structured_type", val) | ||
| } | ||
| value={type} | ||
| onValueChange={(val: QuestionType) => { | ||
| if (val !== "group") { | ||
| updateField("type", val, { questions: [] }); | ||
| } else { | ||
| updateField("type", val); | ||
| } | ||
| }} | ||
| > | ||
| <SelectTrigger> | ||
| <SelectValue placeholder="Select structured type" /> | ||
| <SelectValue placeholder="Select question type" /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {STRUCTURED_QUESTION_TYPES.map((type) => ( | ||
| <SelectItem key={type.value} value={type.value}> | ||
| {type.label} | ||
| </SelectItem> | ||
| ))} | ||
| <SelectItem value="group">Group</SelectItem> | ||
| <SelectItem value="boolean">Boolean</SelectItem> | ||
| <SelectItem value="decimal">Decimal</SelectItem> | ||
| <SelectItem value="integer">Integer</SelectItem> | ||
| <SelectItem value="date">Date</SelectItem> | ||
| <SelectItem value="dateTime">DateTime</SelectItem> | ||
| <SelectItem value="time">Time</SelectItem> | ||
| <SelectItem value="string">String</SelectItem> | ||
| <SelectItem value="text">Text</SelectItem> | ||
| <SelectItem value="url">URL</SelectItem> | ||
| <SelectItem value="choice">Choice</SelectItem> | ||
| <SelectItem value="quantity">Quantity</SelectItem> | ||
| <SelectItem value="structured">Structured</SelectItem> | ||
| </SelectContent> | ||
| </Select> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Streamline question type transitions.
When switching from "group" to a non-group type, existing sub-questions are cleared. Confirm this UX is desired or prompt the user for confirmation to avoid accidental data loss.
| const { data: existingValueset, isLoading } = useQuery({ | ||
| queryKey: ["valueset", slug], | ||
| queryFn: query(valuesetApi.get, { | ||
| pathParams: { slug: slug! }, |
There was a problem hiding this comment.
Potential runtime error with non-null assertion.
The non-null assertion (!) on slug could lead to runtime errors if slug is undefined.
Apply this diff to add a type guard:
- pathParams: { slug: slug! },
+ pathParams: { slug: slug ?? '' },📝 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.
| pathParams: { slug: slug! }, | |
| pathParams: { slug: slug ?? '' }, |
| const handleSubmit = (data: ValuesetFormType) => { | ||
| if (slug && existingValueset) { | ||
| const updateData: UpdateValuesetModel = { | ||
| ...data, | ||
| id: existingValueset.id, | ||
| }; | ||
| updateMutation.mutate(updateData); | ||
| } else { | ||
| const createData: CreateValuesetModel = data; | ||
| createMutation.mutate(createData); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for mutations.
The mutation handlers only handle success cases. Add error handling to improve user experience.
Add error handlers:
const handleSubmit = (data: ValuesetFormType) => {
if (slug && existingValueset) {
const updateData: UpdateValuesetModel = {
...data,
id: existingValueset.id,
};
- updateMutation.mutate(updateData);
+ updateMutation.mutate(updateData, {
+ onError: (error) => {
+ toast.error("Failed to update ValueSet: " + error.message);
+ },
+ });
} else {
const createData: CreateValuesetModel = data;
- createMutation.mutate(createData);
+ createMutation.mutate(createData, {
+ onError: (error) => {
+ toast.error("Failed to create ValueSet: " + error.message);
+ },
+ });
}
};📝 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 handleSubmit = (data: ValuesetFormType) => { | |
| if (slug && existingValueset) { | |
| const updateData: UpdateValuesetModel = { | |
| ...data, | |
| id: existingValueset.id, | |
| }; | |
| updateMutation.mutate(updateData); | |
| } else { | |
| const createData: CreateValuesetModel = data; | |
| createMutation.mutate(createData); | |
| } | |
| }; | |
| const handleSubmit = (data: ValuesetFormType) => { | |
| if (slug && existingValueset) { | |
| const updateData: UpdateValuesetModel = { | |
| ...data, | |
| id: existingValueset.id, | |
| }; | |
| updateMutation.mutate(updateData, { | |
| onError: (error) => { | |
| toast.error("Failed to update ValueSet: " + error.message); | |
| }, | |
| }); | |
| } else { | |
| const createData: CreateValuesetModel = data; | |
| createMutation.mutate(createData, { | |
| onError: (error) => { | |
| toast.error("Failed to create ValueSet: " + error.message); | |
| }, | |
| }); | |
| } | |
| }; |
| navigate(`/valuesets/${data.slug}`); | ||
| }, |
There was a problem hiding this comment.
Navigation path inconsistency detected.
The success handler navigates to /valuesets/${data.slug} while the update handler navigates to /admin/valuesets. This inconsistency could lead to confusion.
Apply this diff to maintain consistent navigation:
- navigate(`/valuesets/${data.slug}`);
+ navigate(`/admin/valuesets/${data.slug}`);📝 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.
| navigate(`/valuesets/${data.slug}`); | |
| }, | |
| navigate(`/admin/valuesets/${data.slug}`); | |
| }, |
| onClick={async () => { | ||
| if (!code.system || !code.code) { | ||
| toast.error("Please select a system and enter a code first"); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const response = await mutate<typeof valuesetApi.lookup>( | ||
| valuesetApi.lookup, | ||
| )({ | ||
| system: code.system, | ||
| code: code.code, | ||
| }); | ||
|
|
||
| if (response.metadata) { | ||
| onChange({ | ||
| ...code, | ||
| display: response.metadata.display, | ||
| }); | ||
| toast.success("Code verified successfully"); | ||
| } | ||
| } catch (error) { | ||
| toast.error("Failed to verify code"); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in code verification.
The error handling could be more specific to help users understand what went wrong.
try {
const response = await mutate<typeof valuesetApi.lookup>(
valuesetApi.lookup,
)({
system: code.system,
code: code.code,
});
if (response.metadata) {
onChange({
...code,
display: response.metadata.display,
});
toast.success("Code verified successfully");
}
} catch (error) {
- toast.error("Failed to verify code");
+ toast.error(
+ error instanceof Error
+ ? error.message
+ : "Failed to verify code. Please try again."
+ );
}📝 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.
| onClick={async () => { | |
| if (!code.system || !code.code) { | |
| toast.error("Please select a system and enter a code first"); | |
| return; | |
| } | |
| try { | |
| const response = await mutate<typeof valuesetApi.lookup>( | |
| valuesetApi.lookup, | |
| )({ | |
| system: code.system, | |
| code: code.code, | |
| }); | |
| if (response.metadata) { | |
| onChange({ | |
| ...code, | |
| display: response.metadata.display, | |
| }); | |
| toast.success("Code verified successfully"); | |
| } | |
| } catch (error) { | |
| toast.error("Failed to verify code"); | |
| } | |
| }} | |
| onClick={async () => { | |
| if (!code.system || !code.code) { | |
| toast.error("Please select a system and enter a code first"); | |
| return; | |
| } | |
| try { | |
| const response = await mutate<typeof valuesetApi.lookup>( | |
| valuesetApi.lookup, | |
| )({ | |
| system: code.system, | |
| code: code.code, | |
| }); | |
| if (response.metadata) { | |
| onChange({ | |
| ...code, | |
| display: response.metadata.display, | |
| }); | |
| toast.success("Code verified successfully"); | |
| } | |
| } catch (error) { | |
| toast.error( | |
| error instanceof Error | |
| ? error.message | |
| : "Failed to verify code. Please try again." | |
| ); | |
| } | |
| }} |
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
valueset_ui
|
| Run status |
|
| Run duration | 06m 52s |
| Commit |
|
| Committer | Amjith Titus |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
11
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Questionnaire/QuestionnaireEditor.tsx (1)
1058-1064: Consider preserving custom options when switching to value set.When switching between custom options and value set, the custom options are lost. Consider preserving them in case the user wants to switch back.
Apply this diff to preserve custom options:
onValueChange={(val: string) => updateField( "answer_value_set", - val === "custom" ? undefined : "valueset", + val === "custom" ? undefined : "valueset", + { + // Preserve the current answer options in a separate field + _preserved_answer_options: val === "custom" ? undefined : answer_option + } ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Questionnaire/QuestionnaireEditor.tsx(11 hunks)src/types/questionnaire/question.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (6)
src/types/questionnaire/question.ts (3)
19-34: LGTM! Well-structured mapping of question types.The constant provides a comprehensive mapping between machine-readable values and human-readable display names, covering all types from the
QuestionTypeunion type.
69-70: LGTM! Optional code property maintains backward compatibility.The addition of the optional
codeproperty allows answer options to have associated codes while maintaining backward compatibility with existing code.
72-76: LGTM! Well-structured observation type interface.The interface follows a standard structure for observation types with all required properties clearly defined.
src/components/Questionnaire/QuestionnaireEditor.tsx (3)
2-2: LGTM! Well-organized imports and state management.The new imports are properly organized, and the state management for observation type is well-typed.
Also applies to: 48-48, 55-55, 58-58, 67-67, 69-69, 96-96
125-125: LGTM! Consistent navigation path updates.Navigation paths are consistently updated to use the admin context, aligning with the admin interface restructuring.
Also applies to: 212-212
711-716: LGTM! Safe value set query implementation.The value set query is properly configured with safe access to results and appropriate fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/components/Questionnaire/QuestionTypes/QuestionInput.tsx (4)
127-132: LGTM! Consider enhancing the user feedback.The message clearly explains why medication requests cannot be recorded. Consider adding a consistent styling class to all error messages for better visual feedback.
- return ( - <span> + return ( + <span className="text-muted-foreground"> Medication requests cannot be recorded without an active encounter </span> );
142-147: Consider extracting common error messages into a reusable component.Multiple structured question types show similar error messages. Consider creating a reusable component to maintain consistency and reduce duplication.
+const NoEncounterMessage = ({ type }: { type: string }) => ( + <span className="text-muted-foreground"> + {type} cannot be recorded without an active encounter + </span> +); // Usage: - return ( - <span> - Medication statement cannot be recorded without an active - encounter - </span> - ); + return <NoEncounterMessage type="Medication statement" />;
160-162: Maintain consistent message formatting.The symptom error message is shorter and has different spacing compared to other messages. Consider maintaining consistent message formatting across all error states.
- return ( - <span> Symptoms cannot be recorded without an encounter </span> + return ( + <span> + Symptoms cannot be recorded without an active encounter + </span> + );
186-188: Align message with established pattern.The encounter error message uses different wording compared to other messages. Consider maintaining a consistent message pattern.
- return ( - <span> Create an encounter first in order to update it </span> + return ( + <span> + Encounter cannot be updated without an active encounter + </span> + );src/components/Questionnaire/QuestionnaireEditor.tsx (2)
1313-1321: Improve type safety for operator values.Consider using a union type for the operator values to improve type safety and maintainability.
- val: - | "equals" - | "not_equals" - | "exists" - | "greater" - | "less" - | "greater_or_equals" - | "less_or_equals", + val: EnableWhenOperator,Add this type definition to your types file:
type EnableWhenOperator = | "equals" | "not_equals" | "exists" | "greater" | "less" | "greater_or_equals" | "less_or_equals";
1419-1446: Simplify value conversion logic.The value conversion logic for different operator types can be simplified using a more declarative approach.
- if ( - [ - "greater", - "less", - "greater_or_equals", - "less_or_equals", - ].includes(condition.operator) - ) { - newCondition = { - question: condition.question, - operator: condition.operator as - | "greater" - | "less" - | "greater_or_equals" - | "less_or_equals", - answer: Number(value), - }; - } else { - newCondition = { - question: condition.question, - operator: condition.operator as - | "equals" - | "not_equals", - answer: value, - }; - } + const isNumericOperator = ["greater", "less", "greater_or_equals", "less_or_equals"].includes(condition.operator); + newCondition = { + question: condition.question, + operator: condition.operator, + answer: isNumericOperator ? Number(value) : value, + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Questionnaire/QuestionTypes/QuestionInput.tsx(5 hunks)src/components/Questionnaire/QuestionnaireEditor.tsx(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (5)
src/components/Questionnaire/QuestionTypes/QuestionInput.tsx (2)
169-173: LGTM! Message is clear and consistent.The diagnosis error message follows the established pattern and provides clear feedback.
200-204: LGTM! Message is clear and consistent.The location error message follows the established pattern and provides clear feedback.
src/components/Questionnaire/QuestionnaireEditor.tsx (3)
126-126: LGTM! Consistent routing structure.The navigation paths have been correctly updated to use the admin context, maintaining consistency across the application.
Also applies to: 213-213
642-646: LGTM! Enhanced debugging capabilities.The integration of the DebugPreview component provides a better debugging experience for questionnaire data.
907-949: LGTM! Well-structured settings UI.The organization of question settings and data collection details into separate sections with clear labels and descriptions improves the user experience.
Also applies to: 951-1014
| @@ -89,6 +94,7 @@ export default function QuestionnaireEditor({ id }: QuestionnaireEditorProps) { | |||
| ); | |||
| const [selectedOrgIds, setSelectedOrgIds] = useState<string[]>([]); | |||
| const [orgSearchQuery, setOrgSearchQuery] = useState(""); | |||
| const [observation, setObservation] = useState<ObservationType | undefined>(); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused state variable.
The observation state variable is declared but never used in the component.
- const [observation, setObservation] = useState<ObservationType | undefined>();📝 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 [observation, setObservation] = useState<ObservationType | undefined>(); |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/components/ValueSet/ValueSetForm.tsx (1)
239-313:⚠️ Potential issueUse the same form instance to manage filter fields.
The FilterFields component creates a new form instance via
useForm(). This breaks the form hierarchy and will prevent filter data from being included in the parent form submission.Apply this diff to fix the issue:
function FilterFields({ nestIndex, type, + parentForm, }: { nestIndex: number; type: "include" | "exclude"; + parentForm: ReturnType<typeof useForm<ValuesetFormType>>; }) { - const form = useForm(); const { fields, append, remove } = useFieldArray({ - control: form.control, + control: parentForm.control, name: `compose.${type}.${nestIndex}.filter`, }); // Update form control references <FormField - control={form.control} + control={parentForm.control} name={`compose.${type}.${nestIndex}.filter.${index}.property`} />src/components/Questionnaire/CodingEditor.tsx (1)
124-149: 🛠️ Refactor suggestionImprove error handling and remove console.error.
The error handling could be more specific, and console.error should be removed from production code.
} catch (error) { - console.error(error); - toast.error("Failed to verify code"); + toast.error( + error instanceof Error + ? error.message + : "Failed to verify code. Please try again." + ); }
🧹 Nitpick comments (3)
src/components/ValueSet/ValueSetForm.tsx (1)
37-38: Enhance slug validation rules.The slug field should be validated to ensure it contains only URL-safe characters.
Apply this diff to enhance the validation:
- slug: z.string().min(1, "Slug is required"), + slug: z + .string() + .min(1, "Slug is required") + .regex(/^[a-z0-9]+(?:-[a-z0-9]+)*$/, "Slug must contain only lowercase letters, numbers, and hyphens"),src/components/Questionnaire/CodingEditor.tsx (1)
98-108: Add input validation and loading state.Consider adding:
- Input validation for the code field
- Loading state during code verification
<Input value={code.code} + disabled={isVerifying} onChange={(e) => { + const sanitizedValue = e.target.value.trim(); onChange({ ...code, - code: e.target.value, + code: sanitizedValue, display: "", }); }} placeholder="Enter code" />src/components/Questionnaire/QuestionnaireEditor.tsx (1)
1260-1486: Improve type safety and code organization in enable conditions.The enable conditions implementation could benefit from:
- Separate type definitions for different operator types
- Extract complex condition rendering logic into separate components
Consider creating an interface for each operator type:
interface NumericCondition { operator: 'greater' | 'less' | 'greater_or_equals' | 'less_or_equals'; answer: number; } interface BooleanCondition { operator: 'exists'; answer: boolean; } interface StringCondition { operator: 'equals' | 'not_equals'; answer: string; } type EnableWhenCondition = (NumericCondition | BooleanCondition | StringCondition) & { question: string; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Questionnaire/CodingEditor.tsx(1 hunks)src/components/Questionnaire/QuestionnaireEditor.tsx(11 hunks)src/components/ValueSet/ValueSetEditor.tsx(1 hunks)src/components/ValueSet/ValueSetForm.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ValueSet/ValueSetEditor.tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
- GitHub Check: OSSAR-Scan
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (4)
src/components/ValueSet/ValueSetForm.tsx (2)
36-101: LGTM! Well-structured schema and interfaces.The schema validation is comprehensive and properly handles optional fields. The interfaces are well-defined with appropriate types.
102-237: LGTM! Well-implemented concept fields management.The component effectively manages dynamic concept fields with proper error handling, validation, and user feedback. The code verification feature is well-implemented with appropriate UI state management.
src/components/Questionnaire/CodingEditor.tsx (1)
23-26: LGTM! Props interface is well-defined.The interface is clear, type-safe, and handles both creation and edit modes appropriately.
src/components/Questionnaire/QuestionnaireEditor.tsx (1)
124-124: LGTM! Navigation paths updated consistently.The navigation paths have been properly updated to include the admin prefix.
Also applies to: 211-211
| </Button> | ||
| </div> | ||
| <ConceptFields nestIndex={index} type={type} parentForm={form} /> | ||
| <FilterFields nestIndex={index} type={type} /> |
There was a problem hiding this comment.
Pass parentForm to FilterFields component.
The FilterFields component requires the parent form instance, but it's not being passed here.
Apply this diff to fix the issue:
- <FilterFields nestIndex={index} type={type} />
+ <FilterFields nestIndex={index} type={type} parentForm={form} />📝 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.
| <FilterFields nestIndex={index} type={type} /> | |
| <FilterFields nestIndex={index} type={type} parentForm={form} /> |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/ValueSet/ValueSetForm.tsx (2)
234-308:⚠️ Potential issueUse the same form instance to manage filter fields.
The FilterFields component creates a new form instance via
useForm()which is inconsistent with how ConceptFields reuses the parent's form. This will cause the filter fields to be managed separately from the main form.Apply this diff to fix the issue:
-function FilterFields({ - nestIndex, - type, -}: { - nestIndex: number; - type: "include" | "exclude"; -}) { - const form = useForm(); +function FilterFields({ + nestIndex, + type, + parentForm, +}: { + nestIndex: number; + type: "include" | "exclude"; + parentForm: ReturnType<typeof useForm<ValuesetFormType>>; +}) { const { fields, append, remove } = useFieldArray({ - control: form.control, + control: parentForm.control, name: `compose.${type}.${nestIndex}.filter`, });
386-386:⚠️ Potential issuePass parentForm to FilterFields component.
The FilterFields component requires the parent form instance, but it's not being passed here.
Apply this diff to fix the issue:
- <FilterFields nestIndex={index} type={type} /> + <FilterFields nestIndex={index} type={type} parentForm={form} />
🧹 Nitpick comments (3)
src/components/Questionnaire/QuestionnaireEditor.tsx (2)
1076-1176: Consider extracting answer options into a separate component.The answer options logic is complex enough to warrant its own component. This would improve maintainability and reusability.
+ // AnswerOptionsEditor.tsx + interface AnswerOptionsEditorProps { + options: AnswerOption[]; + valueSet?: string; + onChange: (options: AnswerOption[]) => void; + onValueSetChange: (valueSet?: string) => void; + } + + function AnswerOptionsEditor({ options, valueSet, onChange, onValueSetChange }: AnswerOptionsEditorProps) { + // Extract the answer options logic here + } // In QuestionnaireEditor.tsx - {!question.answer_value_set ? ( - <CardContent className="space-y-4"> - // ... current answer options logic - </CardContent> - ) : ( - // ... current valueset logic - )} + <AnswerOptionsEditor + options={answer_option || []} + valueSet={question.answer_value_set} + onChange={(newOptions) => updateField("answer_option", newOptions)} + onValueSetChange={(newValueSet) => updateField("answer_value_set", newValueSet)} + />
1259-1485: Consider extracting enable when conditions into a separate component.The enable when conditions logic is complex enough to warrant its own component. This would improve maintainability and reusability.
+ // EnableWhenConditionsEditor.tsx + interface EnableWhenConditionsEditorProps { + conditions: EnableWhen[]; + enableBehavior?: 'all' | 'any'; + onChange: (conditions: EnableWhen[]) => void; + onEnableBehaviorChange: (behavior: 'all' | 'any') => void; + } + + function EnableWhenConditionsEditor({ conditions, enableBehavior, onChange, onEnableBehaviorChange }: EnableWhenConditionsEditorProps) { + // Extract the enable when conditions logic here + } // In QuestionnaireEditor.tsx - <div className="space-y-4"> - <Label>Enable When Conditions</Label> - // ... current enable when conditions logic - </div> + <EnableWhenConditionsEditor + conditions={question.enable_when || []} + enableBehavior={question.enable_behavior} + onChange={(newConditions) => updateField("enable_when", newConditions)} + onEnableBehaviorChange={(newBehavior) => updateField("enable_behavior", newBehavior)} + />src/components/ValueSet/ValueSetForm.tsx (1)
431-443: Consider auto-generating the slug from the name.To improve user experience and maintain consistency, consider auto-generating the slug from the name field while allowing manual override.
Example implementation:
<FormField control={form.control} name="slug" render={({ field }) => ( <FormItem> <FormLabel>Slug</FormLabel> <FormControl> - <Input {...field} /> + <Input + {...field} + onChange={(e) => { + field.onChange(e); + }} + onBlur={(e) => { + if (!e.target.value) { + const nameValue = form.getValues("name"); + field.onChange( + nameValue + .toLowerCase() + .replace(/[^a-z0-9]+/g, "-") + .replace(/^-+|-+$/g, "") + ); + } + }} + /> </FormControl> <FormMessage /> </FormItem> )} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Questionnaire/CodingEditor.tsx(1 hunks)src/components/Questionnaire/QuestionnaireEditor.tsx(10 hunks)src/components/ValueSet/ValueSetEditor.tsx(1 hunks)src/components/ValueSet/ValueSetForm.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ValueSet/ValueSetEditor.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: CodeQL-Build
- GitHub Check: OSSAR-Scan
- GitHub Check: OSSAR-Scan
🔇 Additional comments (8)
src/components/Questionnaire/CodingEditor.tsx (4)
1-30: LGTM! Well-organized imports and clean interface definition.The code demonstrates good organization of imports and a clear, type-safe interface definition.
32-48: Improve error handling in code verification.The error handling could be more specific to help users understand what went wrong.
try { const response = await mutate<typeof valuesetApi.lookup>( valuesetApi.lookup, )({ system: code.system, code: code.code, }); if (response.metadata) { onChange({ ...code, display: response.metadata.display, }); toast.success("Code verified successfully"); } } catch (error) { - toast.error("Failed to verify code"); + toast.error( + error instanceof Error + ? error.message + : "Failed to verify code. Please try again." + ); }
57-59: Add safety check for TERMINOLOGY_SYSTEMS.The default system selection assumes
TERMINOLOGY_SYSTEMShas at least one value. Add a safety check to prevent potential runtime errors.- system: Object.values(TERMINOLOGY_SYSTEMS)[0], + system: Object.values(TERMINOLOGY_SYSTEMS)[0] || '',
71-165: LGTM! Well-structured UI with proper validation.The component demonstrates good practices:
- Clear section organization for system, code, and display
- Proper form validation before verification
- User-friendly UI with clear labels and placeholders
src/components/Questionnaire/QuestionnaireEditor.tsx (3)
1-87: LGTM! Well-organized imports and type definitions.The code demonstrates good organization of imports and proper typing of constants.
639-643: Conditionally render DebugPreview in development.Debug information should not be exposed in production builds.
- <DebugPreview - data={questionnaire} - title="Questionnaire" - className="mt-4" - /> + {process.env.NODE_ENV === 'development' && ( + <DebugPreview + data={questionnaire} + title="Questionnaire" + className="mt-4" + /> + )}
124-124: LGTM! Consistent routing structure.The navigation paths have been properly updated to use the new admin-focused routing structure.
Also applies to: 210-210
src/components/ValueSet/ValueSetForm.tsx (1)
37-95: LGTM! Well-structured schema validation.The form schema is well-defined with appropriate validation rules and optional fields. The props interface is clear and type-safe.
| if (conceptIndex && conceptIndex !== -1) { | ||
| parentForm.setValue( | ||
| `compose.${type}.${nestIndex}.concept.${conceptIndex}.display`, | ||
| response.metadata.display, | ||
| { shouldValidate: true }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix the conceptIndex check condition.
The condition if (conceptIndex && conceptIndex !== -1) is problematic because:
conceptIndexis a number (including 0), not a nullable value- The check for
conceptIndexwill fail when the index is 0
Apply this diff to fix the condition:
- if (conceptIndex && conceptIndex !== -1) {
+ if (conceptIndex !== -1) {📝 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.
| if (conceptIndex && conceptIndex !== -1) { | |
| parentForm.setValue( | |
| `compose.${type}.${nestIndex}.concept.${conceptIndex}.display`, | |
| response.metadata.display, | |
| { shouldValidate: true }, | |
| ); | |
| } | |
| if (conceptIndex !== -1) { | |
| parentForm.setValue( | |
| `compose.${type}.${nestIndex}.concept.${conceptIndex}.display`, | |
| response.metadata.display, | |
| { shouldValidate: true }, | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Common/SkeletonLoading.tsx (2)
113-119: Add test coverage for the new FormSkeleton component.Based on the PR objectives, specifications demonstrating new features are required. Please add test coverage for this component.
Would you like me to help generate test cases for the FormSkeleton component?
113-119: Consider enhancing the FormSkeleton component for better form representation.The component could be more flexible and semantically correct with these improvements:
-export const FormSkeleton = ({ rows }: { rows: number }) => ( +interface FormSkeletonProps { + rows: number; + spacing?: 'sm' | 'md' | 'lg'; // Configurable gap sizes + layout?: 'stack' | 'grid'; // Support different layouts +} + +export const FormSkeleton = ({ + rows, + spacing = 'md', + layout = 'stack' +}: FormSkeletonProps) => ( - <div className="flex flex-col gap-4"> + <form className={` + ${layout === 'stack' ? 'flex flex-col' : 'grid grid-cols-2'} + ${spacing === 'sm' ? 'gap-2' : spacing === 'md' ? 'gap-4' : 'gap-6'} + `}> {Array.from({ length: rows }).map((_, index) => ( - <Skeleton key={index} className="h-10 w-full" /> + <div key={index} className="form-field-skeleton"> + <Skeleton className="h-4 w-24 mb-1" /> {/* Label skeleton */} + <Skeleton className="h-10 w-full" /> {/* Input skeleton */} + </div> ))} - </div> + </form> );This enhancement:
- Uses semantic
<form>element- Adds configurable spacing
- Supports grid layout
- Includes label skeletons for better form representation
src/components/ValueSet/ValueSetEditor.tsx (1)
55-66: Enhance type safety in the submission handler.Consider using a discriminated union type to make the create/update logic more type-safe.
+type ValueSetAction = + | { type: 'create'; data: CreateValuesetModel } + | { type: 'update'; data: UpdateValuesetModel; slug: string }; const handleSubmit = (data: ValuesetFormType) => { - if (slug && existingValueset) { - const updateData: UpdateValuesetModel = { - ...data, - id: existingValueset.id, - }; - updateMutation.mutate(updateData); - } else { - const createData: CreateValuesetModel = data; - createMutation.mutate(createData); - } + const action: ValueSetAction = slug && existingValueset + ? { + type: 'update', + data: { ...data, id: existingValueset.id }, + slug, + } + : { type: 'create', data }; + + switch (action.type) { + case 'create': + createMutation.mutate(action.data); + break; + case 'update': + updateMutation.mutate(action.data); + break; + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Common/SkeletonLoading.tsx(1 hunks)src/components/ValueSet/ValueSetEditor.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (5)
src/components/ValueSet/ValueSetEditor.tsx (5)
1-21: LGTM! Well-organized imports and clear type definitions.The imports are properly organized, and the types are well-defined. The
ValueSetEditorPropsinterface is clear and well-documented.
30-30: Potential runtime error with non-null assertion.The non-null assertion (
!) onslugcould lead to runtime errors ifslugis undefined.
40-41: Navigation path inconsistency detected.The success handler navigates to
/valuesets/${data.slug}while the update handler navigates to/admin/valuesets. This inconsistency could lead to confusion.Also applies to: 51-51
55-66: Consider adding error handling for mutations.The mutation handlers only handle success cases. Add error handling to improve user experience.
68-85: LGTM! Clean and well-structured rendering logic.The component's rendering logic is well-implemented with proper loading states and clear UI structure. The conditional rendering based on the loading state and the form component integration are handled appropriately.
|
@amjithtitus09 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit