Questionnaire Enable When #12492
Questionnaire Enable When #12492
Conversation
|
Warning Rate limit exceeded@Jacobjeevan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes update English locale strings to include comparison operator labels and add UI enhancements for managing "enable_when" conditions in the questionnaire editor. This includes dependency tracking, hierarchical question selection via cascading dropdowns, dynamic operator and answer inputs based on question type, improved layout, and navigation for dependent questions. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QuestionnaireEditor
participant QuestionEditor
participant FormState
User->>QuestionnaireEditor: Open questionnaire editor
QuestionnaireEditor->>QuestionEditor: Render question editor with enable_when dependencies
QuestionEditor->>FormState: useWatch(root questions)
loop For each enable_when condition
QuestionEditor->>QuestionEditor: findQuestionPath(link_id)
QuestionEditor->>User: Render cascading Select dropdowns for question selection
User->>QuestionEditor: Select question/sub-question
QuestionEditor->>QuestionEditor: Update enable_when.question + state
QuestionEditor->>User: Render operator dropdown (filtered by question type)
User->>QuestionEditor: Select operator
QuestionEditor->>User: Render answer input (type-dependent)
User->>QuestionEditor: Input answer
QuestionEditor->>QuestionEditor: Update enable_when.answer
end
User->>QuestionEditor: Click dependent question button
QuestionEditor->>QuestionnaireEditor: handleEnableWhenDependentClick(path, targetId)
QuestionnaireEditor->>User: Expand and scroll to dependent question
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 project 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
🧹 Nitpick comments (2)
src/components/Questionnaire/QuestionnaireEditor.tsx (2)
1246-1271: Consider adding null checks for thefindQuestionPathresult.The path-finding algorithm is well-implemented using an iterative approach. However, ensure that all callers handle the potential
nullreturn value when a question is not found.
2188-2413: Excellent UI refactoring for enable_when conditions.The changes significantly improve the user experience with:
- Hierarchical question selection via cascading dropdowns
- Better visual layout with flex column design
- Clear labeling and structure
- Dynamic sub-question dropdowns for group questions
Consider extracting the inline onChange handlers in the Select components to separate functions for better performance and readability, especially for complex logic like in lines 2221-2241.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(5 hunks)src/components/Questionnaire/QuestionnaireEditor.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: parallel-job (4)
- GitHub Check: parallel-job (3)
- GitHub Check: parallel-job (2)
- GitHub Check: parallel-job (1)
🔇 Additional comments (10)
public/locale/en.json (5)
1353-1353: Add “equals” operator label
The new"equals": "Equals"entry correctly provides a human-readable label for the equality operator.
1387-1387: Add “exists” operator label
The new"exists": "Exists"entry correctly provides a human-readable label for the existence operator.
1530-1531: Add “greater” and “greater_or_equals” operator labels
The new"greater": "Greater Than"and"greater_or_equals": "Greater Than or Equal"entries correctly cover the comparison operators for greater-than semantics.
1693-1694: Add “less” and “less_or_equals” operator labels
The new"less": "Less Than"and"less_or_equals": "Less Than or Equal"entries correctly cover the comparison operators for less-than semantics.
2079-2079: Add “not_equals” operator label
The new"not_equals": "Not Equals"entry correctly provides a human-readable label for the inequality operator.src/components/Questionnaire/QuestionnaireEditor.tsx (5)
13-13: LGTM! Necessary import for the new functionality.The
useWatchhook is correctly imported and used for tracking root questions in the enable_when feature.
1198-1202: Good use ofuseWatchfor reactive form data.The implementation correctly watches the root questions to provide real-time updates for the enable_when dropdown functionality.
1206-1209: Well-structured state for tracking hierarchical question selections.The state correctly maps enable_when condition indices to their corresponding question paths, enabling the cascading dropdown functionality.
1287-1311: Excellent implementation of dynamic operator choices.The function correctly restricts operators based on question type, with boolean and choice questions limited to "equals" and "not_equals" as per the requirements.
1312-1416: Well-implemented dynamic answer inputs based on question type.The function correctly renders:
- Boolean dropdown for boolean questions
- Choice dropdown with available options for choice questions
- Appropriate input fields for other types
The logic aligns perfectly with the PR requirements.
| useEffect(() => { | ||
| if (question.enable_when && question.enable_when.length > 0) { | ||
| question.enable_when.forEach((condition, idx) => { | ||
| const path = findQuestionPath(rootQuestions, condition.question); | ||
| if (path) { | ||
| setEnableWhenQuestionAnswers((prev) => ({ | ||
| ...prev, | ||
| [idx]: path, | ||
| })); | ||
| } | ||
| }); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [question.enable_when]); | ||
|
|
There was a problem hiding this comment.
Add missing dependency to avoid stale closure issues.
The effect uses rootQuestions but doesn't include it in the dependency array, which could lead to stale data issues.
Apply this fix to include the missing dependency:
// eslint-disable-next-line react-hooks/exhaustive-deps
- }, [question.enable_when]);
+ }, [question.enable_when, rootQuestions]);Note: You can remove the eslint-disable comment after adding the dependency.
📝 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.
| useEffect(() => { | |
| if (question.enable_when && question.enable_when.length > 0) { | |
| question.enable_when.forEach((condition, idx) => { | |
| const path = findQuestionPath(rootQuestions, condition.question); | |
| if (path) { | |
| setEnableWhenQuestionAnswers((prev) => ({ | |
| ...prev, | |
| [idx]: path, | |
| })); | |
| } | |
| }); | |
| } | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [question.enable_when]); | |
| useEffect(() => { | |
| if (question.enable_when && question.enable_when.length > 0) { | |
| question.enable_when.forEach((condition, idx) => { | |
| const path = findQuestionPath(rootQuestions, condition.question); | |
| if (path) { | |
| setEnableWhenQuestionAnswers((prev) => ({ | |
| ...prev, | |
| [idx]: path, | |
| })); | |
| } | |
| }); | |
| } | |
| }, [question.enable_when, rootQuestions]); |
🤖 Prompt for AI Agents
In src/components/Questionnaire/QuestionnaireEditor.tsx around lines 1272 to
1286, the useEffect hook depends on rootQuestions but it is missing from the
dependency array, which can cause stale closure issues. Fix this by adding
rootQuestions to the dependency array of the useEffect hook and then remove the
eslint-disable-next-line comment related to react-hooks/exhaustive-deps.
|
Similarly any modification in the type or anything will not be fetched |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Questionnaire/QuestionnaireEditor.tsx (1)
1349-1362: This issue was flagged in previous reviews but still needs to be addressed.The effect uses
rootQuestionsbut doesn't include it in the dependency array, which can cause stale data issues and potentially explains the Answer dropdown refresh problem mentioned in the PR comments.Apply this fix to include the missing dependency:
// eslint-disable-next-line react-hooks/exhaustive-deps - }, [question.enable_when]); + }, [question.enable_when, rootQuestions]);Note: You can remove the eslint-disable comment after adding the dependency.
🧹 Nitpick comments (4)
src/components/Questionnaire/QuestionnaireEditor.tsx (4)
1630-1655: Remove unnecessary Fragment wrapper.The Fragment wrapper is redundant since it contains only one child element.
Apply this fix to remove the unnecessary Fragment:
- {(enableWhenDependencies.get(question.link_id)?.size || 0) > 0 && ( - <> - <div className="text-sm text-gray-500 flex flex-col gap-1"> - {t("questionnaire_question_dependent")} - <div className="flex flex-wrap gap-2"> - {Array.from( - enableWhenDependencies.get(question.link_id) || [], - ).map(({ question, path }) => ( - <Button - type="button" - variant="outline" - size="xs" - key={question.link_id} - onClick={(e) => { - e.preventDefault(); - handleEnableWhenDependentClick(path, question.link_id); - }} - className="text-primary hover:underline" - > - {question.text} - </Button> - ))} - </div> - {t("ensure_conditions_are_valid")} - </div> - </> - )} + {(enableWhenDependencies.get(question.link_id)?.size || 0) > 0 && ( + <div className="text-sm text-gray-500 flex flex-col gap-1"> + {t("questionnaire_question_dependent")} + <div className="flex flex-wrap gap-2"> + {Array.from( + enableWhenDependencies.get(question.link_id) || [], + ).map(({ question, path }) => ( + <Button + type="button" + variant="outline" + size="xs" + key={question.link_id} + onClick={(e) => { + e.preventDefault(); + handleEnableWhenDependentClick(path, question.link_id); + }} + className="text-primary hover:underline" + > + {question.text} + </Button> + ))} + </div> + {t("ensure_conditions_are_valid")} + </div> + )}🧰 Tools
🪛 Biome (1.9.4)
[error] 1630-1654: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
434-465: Optimize the dependency tracking algorithm.The nested
forEachcould be inefficient for large questionnaire structures. Consider optimizing for better performance.Consider using a more efficient approach:
useEffect(() => { if (!questionnaire) return; const newEnableWhenDependencies = new Map< string, Set<{ question: Question; path: string[] }> >(); - const processQuestions = ( - questions: Question[], - currentPath: string[] = [], - ) => { - questions.forEach((question) => { - question.enable_when?.forEach(({ question: dependentQuestionId }) => { - const deps = - newEnableWhenDependencies.get(dependentQuestionId) || new Set(); - deps.add({ - question: question, - path: [...currentPath, question.link_id], - }); - newEnableWhenDependencies.set(dependentQuestionId, deps); - }); - if (question.questions?.length) { - processQuestions(question.questions, [ - ...currentPath, - question.link_id, - ]); - } - }); - }; + const processQuestions = ( + questions: Question[], + currentPath: string[] = [], + ) => { + for (const question of questions) { + const questionPath = [...currentPath, question.link_id]; + + if (question.enable_when?.length) { + for (const { question: dependentQuestionId } of question.enable_when) { + const deps = newEnableWhenDependencies.get(dependentQuestionId) || new Set(); + deps.add({ question, path: questionPath }); + newEnableWhenDependencies.set(dependentQuestionId, deps); + } + } + + if (question.questions?.length) { + processQuestions(question.questions, questionPath); + } + } + }; processQuestions(questionnaire.questions); setEnableWhenDependencies(newEnableWhenDependencies); }, [questionnaire]);
1323-1347: Optimize the question path finding algorithm.The current implementation uses a stack-based approach which is good, but the path construction could be more efficient.
Consider this optimization to reduce array operations:
const findQuestionPath = ( questions: Question[], targetId: string, ): Question[] | null => { - const pathStack: [Question, Question[]][] = questions.map((q) => [q, []]); + const stack: { question: Question; path: Question[] }[] = questions.map((q) => ({ + question: q, + path: [] + })); - while (pathStack.length > 0) { - const [current, path] = pathStack.pop()!; + while (stack.length > 0) { + const { question: current, path } = stack.pop()!; if (current.link_id === targetId) { return [...path, current]; } if ( current.type === "group" && current.questions && current.questions.length > 0 ) { + const newPath = [...path, current]; current.questions.forEach((q) => { - pathStack.push([q, [...path, current]]); + stack.push({ question: q, path: newPath }); }); } } return null; };
1283-1285: Consider optimizing the state structure for enable_when answers.The current structure
Record<number, Question[]>could be improved for better type safety and performance.Consider using a more explicit type and structure:
- const [enableWhenQuestionAnswers, setEnableWhenQuestionAnswers] = useState< - Record<number, Question[]> - >({}); + const [enableWhenQuestionAnswers, setEnableWhenQuestionAnswers] = useState< + Map<number, Question[]> + >(new Map());This would require updating the related logic to use Map methods instead of object property access, but provides better performance for frequent updates and clearer semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(8 hunks)src/components/Questionnaire/QuestionnaireEditor.tsx(24 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/locale/en.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Questionnaire/QuestionnaireEditor.tsx
[error] 1630-1654: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Test
- GitHub Check: install
- GitHub Check: Header rules - care-ohc
- GitHub Check: OSSAR-Scan
- GitHub Check: CodeQL-Build
- GitHub Check: Pages changed - care-ohc
🔇 Additional comments (3)
src/components/Questionnaire/QuestionnaireEditor.tsx (3)
2270-2495: Excellent implementation of hierarchical question selection.The enable_when condition UI with cascading dropdowns and dynamic operator/answer rendering based on question type is well-implemented. The logic correctly handles different question types and provides appropriate input methods for each.
467-477: Good implementation of dependency navigation.The
handleEnableWhenDependentClickfunction correctly expands parent questions and scrolls to target questions, providing excellent UX for dependency management.
1364-1493: Well-structured dynamic rendering logic.The
getOperatorChoicesandgetAnswerChoicesfunctions properly restrict operators based on question types and provide appropriate answer inputs. This addresses the PR requirements for adaptive UI based on question type.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
|
@Jacobjeevan can you recheck everything is working, boolean issue still exist |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/Questionnaire/QuestionnaireEditor.tsx (3)
1493-1506: MissingrootQuestionsin effect dependencies risks stale pathsThe effect derives
findQuestionPath(rootQuestions, …)but doesn’t list
rootQuestionsin the dependency array, repeating the stale-closure problem
mentioned in the previous review. Add it to keep paths up-to-date.-// eslint-disable-next-line react-hooks/exhaustive-deps -}, [question.enable_when]); +}, [question.enable_when, rootQuestions]);
493-497: Guard againstundefinedfromuseWatchto prevent crashes
useWatchreturnsundefinedon the first render; calling.map,.forEach, etc.
on that value (which happens many times below) will throw.-const rootQuestions: Question[] = useWatch({ - control: form.control, - name: "questions", -}); +const rootQuestions: Question[] = + useWatch({ control: form.control, name: "questions" }) || [];This is exactly the issue flagged earlier and is still unresolved.
1554-1561:condition.answermay beundefined– calling.toString()will crashlet answer = condition.answer.toString().toLowerCase();If the user adds a new condition but hasn’t picked an answer yet,
condition.answerisundefined, leading toTypeError: undefined.toString.-let answer = condition.answer.toString().toLowerCase(); +let answer = String(condition.answer ?? "").toLowerCase();Same issue was pointed out previously but persists here.
🧹 Nitpick comments (1)
src/components/Questionnaire/QuestionnaireEditor.tsx (1)
1798-1824: Redundant React fragment – tighten the markupThe fragment wrapping the “Dependent questions” block contains a single parent
element and is not keyed; it can be removed to reduce DOM depth.-<> - <div className="text-sm …"> - … - </div> -</> +<div className="text-sm …"> + … +</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(8 hunks)src/components/Questionnaire/QuestionnaireEditor.tsx(26 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/Questionnaire/QuestionnaireEditor.tsx (7)
src/types/questionnaire/question.ts (2)
Question(126-158)EnableWhen(109-111)src/Utils/request/utils.ts (1)
swapElements(100-107)src/components/ui/select.tsx (5)
Select(192-192)SelectTrigger(200-200)SelectValue(201-201)SelectContent(193-193)SelectItem(195-195)src/components/ui/input.tsx (1)
Input(30-30)src/components/ui/button.tsx (1)
Button(83-83)src/CAREUI/icons/CareIcon.tsx (1)
CareIcon(22-73)src/components/ui/label.tsx (1)
Label(22-22)
🪛 Biome (1.9.4)
src/components/Questionnaire/QuestionnaireEditor.tsx
[error] 1799-1823: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: install
- GitHub Check: Test
- GitHub Check: CodeQL-Build
- GitHub Check: lint
- GitHub Check: OSSAR-Scan
public/locale/en.json
Outdated
| @@ -2428,6 +2436,7 @@ | |||
| "questionnaire_appointment_no_encounter": "Appointment cannot be recorded without an active encounter", | |||
| "questionnaire_created_successfully": "Questionnaire created successfully", | |||
| "questionnaire_diagnosis_no_encounter": "Diagnosis cannot be recorded without an active encounter", | |||
| "questionnaire_enabled_on": "Enabled on: ", | |||
There was a problem hiding this comment.
: shouldn't be part of translations right?
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
src/components/Questionnaire/QuestionnaireEditor.tsx (4)
1507-1520: Missing dependency inuseEffect– duplicate from previous review
rootQuestionsis read inside the effect but is still absent from the dependency list, leading to stale paths when the form mutates.This was already flagged in an earlier review comment and remains unresolved.
1569-1576:condition.answer.toString()can still crash whenanswerisundefinedThe boolean branch did not adopt the previously suggested safe-string conversion.
Accessing.toString()onundefinedwill blow up the component when a new condition row is added.Reuse the earlier fix:
-let answer = condition.answer.toString().toLowerCase(); +let answer = String(condition.answer ?? "").toLowerCase();
458-462: Guard againstundefinedfromuseWatchbefore first render
useWatchmay returnundefineduntil the form is initialized.
rootQuestions.map,rootQuestions.forEach, etc. are invoked in multiple render paths and will throw ifrootQuestionsisundefined.-const rootQuestions: Question[] = useWatch({ - control: form.control, - name: "questions", -}); +const rootQuestions: Question[] = + useWatch({ control: form.control, name: "questions" }) || [];Apply the same pattern wherever
useWatchis used to pull the questions array.
1428-1432: SameuseWatchnull-safety issue insideQuestionEditorThe local
rootQuestionshere suffers from the same race condition and should be defaulted to an empty array to prevent crashes when the component mounts.-const rootQuestions = useWatch({ - control: form.control, - name: "questions", -}) as Question[]; +const rootQuestions: Question[] = + (useWatch({ control: form.control, name: "questions" }) as Question[]) || [];
🧹 Nitpick comments (3)
src/components/Questionnaire/ManageQuestionnaireTagsSheet.tsx (1)
164-166: Minor:slug/tagswatchers rerender every keystroke
useWatchwithoutdefaultValueforces a re-render cascade while typing.
Unless you need the instantaneous feedback, supplyingdefaultValuecan cut unnecessary renders:const slug = useWatch({ control: form.control, name: "slug", defaultValue: "" }); const tags = useWatch({ control: form.control, name: "tags", defaultValue: [] });src/components/Questionnaire/QuestionnaireProperties.tsx (2)
354-356: Provide defaults for watched fields to avoid uncontrolled → controlled warnings
RadioGroupexpects a definedvalue. If the form has not produced a value yet React will log warnings. Supply sane defaults:const status = useWatch({ control: form.control, name: "status", defaultValue: "draft" }); const subjectType = useWatch({ control: form.control, name: "subject_type", defaultValue: "patient" });
401-408: Disabled input still has anonChangehandlerThe handler can never fire while the field is disabled and confuses static-analysis tools.
-<Input - ... - disabled={true} - onChange={(e) => updateQuestionnaireField("version", e.target.value)} -/> +<Input + ... + disabled +/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Questionnaire/CloneQuestionnaireSheet.tsx(4 hunks)src/components/Questionnaire/ManageQuestionnaireTagsSheet.tsx(7 hunks)src/components/Questionnaire/QuestionnaireEditor.tsx(36 hunks)src/components/Questionnaire/QuestionnaireProperties.tsx(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/components/Questionnaire/CloneQuestionnaireSheet.tsx (1)
src/types/questionnaire/questionnaire.ts (1)
QuestionnaireDetail(9-20)
src/components/Questionnaire/QuestionnaireProperties.tsx (3)
src/types/questionnaire/questionnaire.ts (1)
QuestionnaireDetail(9-20)src/components/Questionnaire/ManageQuestionnaireTagsSheet.tsx (1)
ManageQuestionnaireTagsSheet(147-396)src/components/Questionnaire/CloneQuestionnaireSheet.tsx (1)
CloneQuestionnaireSheet(47-270)
src/components/Questionnaire/ManageQuestionnaireTagsSheet.tsx (2)
src/types/questionnaire/questionnaire.ts (1)
QuestionnaireDetail(9-20)src/Utils/request/mutate.ts (1)
mutate(19-26)
🪛 Biome (1.9.4)
src/components/Questionnaire/QuestionnaireEditor.tsx
[error] 1813-1837: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: install
- GitHub Check: Test
- GitHub Check: CodeQL-Build
- GitHub Check: OSSAR-Scan
🔇 Additional comments (1)
src/components/Questionnaire/CloneQuestionnaireSheet.tsx (1)
252-254: UX: allow cloning a questionnaire without organisations?The CTA is disabled unless
selectedIds.length > 0. Cloning without pre-assigning organisations might be a valid workflow (e.g. draft in personal space). Confirm this requirement, otherwise drop the guard to avoid blocking power-users.
| const slug = useWatch({ control: form.control, name: "slug" }); | ||
| const tags = useWatch({ control: form.control, name: "tags" }); | ||
| const [newSlug, setNewSlug] = useState(slug + "-copy"); | ||
| const [error, setError] = useState<string | null>(null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
slug may be undefined – safeguard before concatenation
useWatch returns undefined until the first render completes and the form has a value.
useState(slug + "-copy") will therefore create "undefined-copy" in that first render for a brand-new form, leaking an invalid default into the UI.
-const [newSlug, setNewSlug] = useState(slug + "-copy");
+const [newSlug, setNewSlug] = useState(
+ slug ? `${slug}-copy` : "" // bail-out until slug is known
+);📝 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 slug = useWatch({ control: form.control, name: "slug" }); | |
| const tags = useWatch({ control: form.control, name: "tags" }); | |
| const [newSlug, setNewSlug] = useState(slug + "-copy"); | |
| const [error, setError] = useState<string | null>(null); | |
| const slug = useWatch({ control: form.control, name: "slug" }); | |
| const tags = useWatch({ control: form.control, name: "tags" }); | |
| const [newSlug, setNewSlug] = useState( | |
| slug ? `${slug}-copy` : "" // bail-out until slug is known | |
| ); | |
| const [error, setError] = useState<string | null>(null); |
🤖 Prompt for AI Agents
In src/components/Questionnaire/CloneQuestionnaireSheet.tsx around lines 51 to
54, the slug variable from useWatch may be undefined initially, causing the
newSlug state to be initialized with "undefined-copy". To fix this, add a
safeguard by checking if slug is defined before concatenation, and provide a
fallback default string when slug is undefined to ensure newSlug is always
initialized with a valid string.
| const clonedQuestionnaire = { | ||
| ...questionnaire, | ||
| ...form.getValues(), | ||
| slug: newSlug.trim(), | ||
| id: undefined, | ||
| status: "draft" as const, | ||
| title: `${questionnaire.title} (Clone)`, | ||
| title: `${form.getValues("title")} (Clone)`, | ||
| organizations: selectedIds, | ||
| tags: questionnaire.tags.map((tag) => tag.id), | ||
| tags: tags.map((tag) => tag.id), | ||
| }; |
There was a problem hiding this comment.
tags.map throws when the watcher has not emitted yet
handleClone assumes tags is always an array. When the sheet is opened before the form field is populated this will throw a TypeError.
- tags: tags.map((tag) => tag.id),
+ tags: (tags ?? []).map((tag) => tag.id),📝 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 clonedQuestionnaire = { | |
| ...questionnaire, | |
| ...form.getValues(), | |
| slug: newSlug.trim(), | |
| id: undefined, | |
| status: "draft" as const, | |
| title: `${questionnaire.title} (Clone)`, | |
| title: `${form.getValues("title")} (Clone)`, | |
| organizations: selectedIds, | |
| tags: questionnaire.tags.map((tag) => tag.id), | |
| tags: tags.map((tag) => tag.id), | |
| }; | |
| const clonedQuestionnaire = { | |
| ...form.getValues(), | |
| slug: newSlug.trim(), | |
| id: undefined, | |
| status: "draft" as const, | |
| title: `${form.getValues("title")} (Clone)`, | |
| organizations: selectedIds, | |
| tags: (tags ?? []).map((tag) => tag.id), | |
| }; |
🤖 Prompt for AI Agents
In src/components/Questionnaire/CloneQuestionnaireSheet.tsx around lines 92 to
100, the code assumes that 'tags' is always an array, but if the watcher has not
emitted yet, 'tags' may be undefined, causing a TypeError when calling tags.map.
To fix this, add a check to ensure 'tags' is defined and is an array before
mapping, or provide a default empty array fallback to safely handle the mapping
operation.
| // Initialize selected tags from questionnaire tags | ||
| useEffect(() => { | ||
| if (questionnaire.tags) { | ||
| setSelectedTags(questionnaire.tags); | ||
| if (tags) { | ||
| setSelectedTags(tags); | ||
| } | ||
| }, [questionnaire.tags]); | ||
| }, [tags]); | ||
|
|
There was a problem hiding this comment.
Guard against undefined when syncing watched tags
setSelectedTags(tags) will throw if tags is still undefined. A trivial default avoids the crash:
-if (tags) {
- setSelectedTags(tags);
-}
+setSelectedTags(tags ?? []);📝 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.
| // Initialize selected tags from questionnaire tags | |
| useEffect(() => { | |
| if (questionnaire.tags) { | |
| setSelectedTags(questionnaire.tags); | |
| if (tags) { | |
| setSelectedTags(tags); | |
| } | |
| }, [questionnaire.tags]); | |
| }, [tags]); | |
| // Initialize selected tags from questionnaire tags | |
| useEffect(() => { | |
| setSelectedTags(tags ?? []); | |
| }, [tags]); |
🤖 Prompt for AI Agents
In src/components/Questionnaire/ManageQuestionnaireTagsSheet.tsx around lines
195 to 201, the useEffect hook calls setSelectedTags(tags) without guarding
against tags being undefined, which can cause a crash. Fix this by providing a
default empty array when tags is undefined, such as calling setSelectedTags(tags
|| []), to ensure setSelectedTags always receives a defined array.
| const hasChanges = | ||
| new Set(questionnaire.tags.map((tag) => tag.id)).size !== | ||
| new Set(selectedTags).size || | ||
| !questionnaire.tags.every((tag) => | ||
| selectedTags.some((st) => st.id === tag.id), | ||
| ); | ||
| new Set(tags?.map((tag) => tag.id)).size !== new Set(selectedTags).size || | ||
| !tags?.every((tag) => selectedTags.some((st) => st.id === tag.id)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
hasChanges logic breaks on empty form & mismatches ID comparison
new Set(tags?.map(...))passesundefinedto theSetconstructor → runtime error.selectedTagsis compared as objects, not byid, so the size test is meaningless.
-const hasChanges =
- new Set(tags?.map((tag) => tag.id)).size !== new Set(selectedTags).size ||
- !tags?.every((tag) => selectedTags.some((st) => st.id === tag.id));
+const originalIds = (tags ?? []).map((t) => t.id);
+const selectedIds = selectedTags.map((t) => t.id);
+
+const hasChanges =
+ originalIds.length !== selectedIds.length ||
+ !originalIds.every((id) => selectedIds.includes(id));📝 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 hasChanges = | |
| new Set(questionnaire.tags.map((tag) => tag.id)).size !== | |
| new Set(selectedTags).size || | |
| !questionnaire.tags.every((tag) => | |
| selectedTags.some((st) => st.id === tag.id), | |
| ); | |
| new Set(tags?.map((tag) => tag.id)).size !== new Set(selectedTags).size || | |
| !tags?.every((tag) => selectedTags.some((st) => st.id === tag.id)); | |
| const originalIds = (tags ?? []).map((t) => t.id); | |
| const selectedIds = selectedTags.map((t) => t.id); | |
| const hasChanges = | |
| originalIds.length !== selectedIds.length || | |
| !originalIds.every((id) => selectedIds.includes(id)); |
🤖 Prompt for AI Agents
In src/components/Questionnaire/ManageQuestionnaireTagsSheet.tsx around lines
244 to 247, the hasChanges calculation breaks when tags is undefined because it
passes undefined to the Set constructor, causing a runtime error. Also,
selectedTags is an array of objects but is compared by size directly, which is
invalid since objects are not compared by id. To fix this, ensure tags defaults
to an empty array before mapping, and compare the sets by extracting ids from
both tags and selectedTags arrays before comparing their sizes and contents.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/Questionnaire/QuestionnaireEditor.tsx (3)
458-462:rootQuestionscan beundefined– crash on first render
useWatchreturnsundefineduntil the form is initialised, yet the rest of the component (map,forEach, etc.) assumes an array.
This was already flagged earlier but is still unfixed.-const rootQuestions: Question[] = useWatch({ - control: form.control, - name: "questions", -}); +const rootQuestions: Question[] = + useWatch({ control: form.control, name: "questions" }) || [];Failing to guard will throw on the very first render and break the editor.
1569-1574:condition.answermay beundefined–toString()will crashThis is the same issue previously highlighted; call
String()with a null-coalesce guard instead.-let answer = condition.answer.toString(); +let answer = String(condition.answer ?? "");
1507-1520: Missing dependency → stale closure insideQuestionEditor
useEffectrelies onrootQuestionsbut omits it from the dependency list, so the path resolution never reacts to live edits.-}, [question.enable_when]); +}, [question.enable_when, rootQuestions]);Remove the ESLint disable once the dependency is added.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Questionnaire/QuestionTypes/QuestionGroup.tsx(1 hunks)src/components/Questionnaire/QuestionnaireEditor.tsx(36 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Questionnaire/QuestionTypes/QuestionGroup.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/Questionnaire/QuestionnaireEditor.tsx (4)
src/types/questionnaire/question.ts (2)
Question(126-158)EnableWhen(109-111)src/types/questionnaire/questionnaire.ts (1)
QuestionnaireDetail(9-20)src/Utils/request/utils.ts (1)
swapElements(100-107)src/CAREUI/icons/CareIcon.tsx (1)
CareIcon(22-73)
🪛 Biome (1.9.4)
src/components/Questionnaire/QuestionnaireEditor.tsx
[error] 1813-1837: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test
- GitHub Check: CodeQL-Build
- GitHub Check: install
- GitHub Check: OSSAR-Scan
| handleValidatedChange("questions", [ | ||
| ...questionnaire.questions, | ||
| ...rootQuestions, | ||
| newQuestion, | ||
| ]); |
There was a problem hiding this comment.
Wrong helper used – validation never runs for newly-added questions
handleValidatedChange only enables validation for title | description | slug, but you pass questions through it.
The call silently bypasses validation and also violates the helper’s narrowing cast.
- handleValidatedChange("questions", [
- ...rootQuestions,
- newQuestion,
- ]);
+ updateQuestions([...rootQuestions, newQuestion]);Using the dedicated updateQuestions helper keeps the types intact and triggers the expected validation.
📝 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.
| handleValidatedChange("questions", [ | |
| ...questionnaire.questions, | |
| ...rootQuestions, | |
| newQuestion, | |
| ]); | |
| updateQuestions([...rootQuestions, newQuestion]); |
🤖 Prompt for AI Agents
In src/components/Questionnaire/QuestionnaireEditor.tsx around lines 1045 to
1048, replace the call to handleValidatedChange with the dedicated
updateQuestions helper when adding new questions. This is because
handleValidatedChange only triggers validation for title, description, and slug
fields, so using it with questions bypasses validation and breaks type
narrowing. Using updateQuestions will maintain correct types and ensure
validation runs on the newly added questions.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/Questionnaire/QuestionTypes/QuestionGroup.tsx (1)
41-46: Potential O(N²) scan when manyenable_whenrules
checkConditioncallsquestionnaireResponses.find(...)for every rule.
A simpleMap<link_id, ResponseValue[]>built once would drop the per-rule linear scan and keep the logic O(N). Worth considering if large questionnaires degrade performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Questionnaire/QuestionTypes/QuestionGroup.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: CodeQL
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: OSSAR-Scan
- GitHub Check: Test
- GitHub Check: install
- GitHub Check: lint
- GitHub Check: CodeQL-Build
| case "greater": | ||
| return ( | ||
| typeof dependentValue.value === "number" && | ||
| dependentValue.value > enableWhen.answer | ||
| return normalizedAnswers.some( | ||
| (v) => !isNaN(Number(v)) && Number(v) > enableWhen.answer, | ||
| ); | ||
|
|
||
| case "less": | ||
| return ( | ||
| typeof dependentValue.value === "number" && | ||
| dependentValue.value < enableWhen.answer | ||
| return normalizedAnswers.some( | ||
| (v) => !isNaN(Number(v)) && Number(v) < enableWhen.answer, | ||
| ); | ||
|
|
||
| case "greater_or_equals": | ||
| return ( | ||
| typeof dependentValue.value === "number" && | ||
| dependentValue.value >= enableWhen.answer | ||
| return normalizedAnswers.some( | ||
| (v) => !isNaN(Number(v)) && Number(v) >= enableWhen.answer, | ||
| ); | ||
|
|
||
| case "less_or_equals": | ||
| return ( | ||
| typeof dependentValue.value === "number" && | ||
| dependentValue.value <= enableWhen.answer | ||
| return normalizedAnswers.some( | ||
| (v) => !isNaN(Number(v)) && Number(v) <= enableWhen.answer, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Early toString() forces double parsing and can silently mis-compare
Since numbers were stringified in normalizeValue, every numeric comparison now relies on Number(v) inside some(...). This:
- Performs needless double conversion.
- Loses
NaNsignalling when the originalv.valueis already a number. - Keeps the asymmetry with
enableWhen.answer, which can be a string or number.
A cleaner, faster approach:
-function normalizeValue(value: unknown): unknown {
- if (typeof value === "boolean") return value ? "Yes" : "No";
- if (typeof value === "number") return value.toString();
- return value;
-}
+function normalizeValue(value: unknown): unknown {
+ if (typeof value === "boolean") return value;
+ return value; // keep numbers as numbers, strings as-is
+}…and cast both sides once at the numeric operator:
const numTarget = Number(enableWhen.answer);
return normalizedAnswers.some((v) =>
typeof v === "number" && !isNaN(numTarget) && v > numTarget,
);This removes double work and eliminates type-mismatch edge cases.
🤖 Prompt for AI Agents
In src/components/Questionnaire/QuestionTypes/QuestionGroup.tsx between lines 67
and 85, the current code converts each normalized answer to a number repeatedly
inside the some() calls, causing double parsing and potential silent
mis-comparisons. To fix this, convert enableWhen.answer to a number once before
the some() call, then inside some(), check if the answer is a number and compare
it directly to the pre-converted target number. This eliminates redundant
conversions and ensures consistent numeric comparisons.
| case "exists": | ||
| return dependentValue !== undefined && dependentValue !== null; | ||
| case "equals": { | ||
| return normalizeValue(dependentValue.value) === enableWhen.answer; | ||
| } | ||
| case "not_equals": { | ||
| return normalizeValue(dependentValue.value) !== enableWhen.answer; | ||
| } | ||
| return normalizedAnswers.length > 0; | ||
|
|
||
| case "equals": | ||
| return normalizedAnswers.includes(enableWhen.answer); | ||
|
|
There was a problem hiding this comment.
exists operator ignores the expected boolean value
FHIR semantics:
• exists == true → enable only when an answer is present
• exists == false → enable only when an answer is absent
Current code always returns normalizedAnswers.length > 0, effectively treating false the same as true.
-case "exists":
- return normalizedAnswers.length > 0;
+case "exists": {
+ const shouldExist = enableWhen.answer !== false; // default true
+ return shouldExist
+ ? normalizedAnswers.length > 0
+ : normalizedAnswers.length === 0;
+}Without this, rules like “enable when Question A does not exist” never trigger.
📝 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.
| case "exists": | |
| return dependentValue !== undefined && dependentValue !== null; | |
| case "equals": { | |
| return normalizeValue(dependentValue.value) === enableWhen.answer; | |
| } | |
| case "not_equals": { | |
| return normalizeValue(dependentValue.value) !== enableWhen.answer; | |
| } | |
| return normalizedAnswers.length > 0; | |
| case "equals": | |
| return normalizedAnswers.includes(enableWhen.answer); | |
| case "exists": { | |
| const shouldExist = enableWhen.answer !== false; // default true | |
| return shouldExist | |
| ? normalizedAnswers.length > 0 | |
| : normalizedAnswers.length === 0; | |
| } | |
| case "equals": | |
| return normalizedAnswers.includes(enableWhen.answer); |
🤖 Prompt for AI Agents
In src/components/Questionnaire/QuestionTypes/QuestionGroup.tsx around lines 58
to 63, the "exists" operator currently only checks if answers exist and ignores
the expected boolean value, causing it to treat both true and false the same.
Modify the code to check if enableWhen.answer is true or false: return true if
answers exist and enableWhen.answer is true, or return true if no answers exist
and enableWhen.answer is false. This will correctly handle enabling when answers
are present or absent as per FHIR semantics.
| if (typeof value === "boolean") return value ? "Yes" : "No"; | ||
| if (typeof value === "number") return value.toString(); | ||
| return value; | ||
| } | ||
|
|
||
| const normalizedAnswers = dependentValues.map((v) => | ||
| normalizeValue(v.value), | ||
| ); |
There was a problem hiding this comment.
Normalization mismatch breaks boolean / numeric equality checks
normalizeValue converts boolean → "Yes" | "No" and number → string, but enableWhen.answer is not normalized before the equality test below.
normalizedAnswers.includes(enableWhen.answer) will therefore never match for booleans ("Yes" ≠ true) and often fails for numbers ("5" ≠ 5). This makes equals / not_equals unreliable and is the root cause of the “boolean question still persists” bug reported in QA.
-const normalizedAnswers = dependentValues.map((v) =>
- normalizeValue(v.value),
-);
+const normalizedAnswers = dependentValues.map((v) =>
+ normalizeValue(v.value),
+);
+const targetAnswer = normalizeValue(enableWhen.answer);…and later use targetAnswer in includes / numeric branches (or, simpler, stop coercing primitives at all).
📝 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 (typeof value === "boolean") return value ? "Yes" : "No"; | |
| if (typeof value === "number") return value.toString(); | |
| return value; | |
| } | |
| const normalizedAnswers = dependentValues.map((v) => | |
| normalizeValue(v.value), | |
| ); | |
| if (typeof value === "boolean") return value ? "Yes" : "No"; | |
| if (typeof value === "number") return value.toString(); | |
| return value; | |
| } | |
| const normalizedAnswers = dependentValues.map((v) => | |
| normalizeValue(v.value), | |
| ); | |
| const targetAnswer = normalizeValue(enableWhen.answer); |
🤖 Prompt for AI Agents
In src/components/Questionnaire/QuestionTypes/QuestionGroup.tsx around lines 48
to 55, the function normalizeValue converts booleans and numbers to strings, but
enableWhen.answer is not normalized before comparison, causing mismatches in
equality checks. To fix this, apply the same normalizeValue function to
enableWhen.answer before using it in includes or equality comparisons, ensuring
both sides are consistently normalized for reliable boolean and numeric equality
checks.
|
LGTM, can we get it reviewed @rithviknishad Minor boolean spacing need to fixed |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
|
@Jacobjeevan 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! 🙌 |
Proposed Changes
- Operator will be restricted likewise for boolean and choice qns to just equal/not equals
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Localization
Bug Fixes
Style