[ENG-761] Draft support for Questionnaires with Structured Questions - #16573
[ENG-761] Draft support for Questionnaires with Structured Questions#16573gigincg wants to merge 3 commits into
Conversation
|
WalkthroughStructured questionnaire responses now preserve fetched record context, reconcile restored drafts with current server data, identify added, removed, and updated records, and show localized changes before continuation. ChangesStructured draft reconciliation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Deploying care-preview with
|
| Latest commit: |
59cd45c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7a3eeb6f.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://eng-761-structured-questionn.care-preview-a7w.pages.dev |
gigincg
changed the title
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Questionnaire/DraftContextChangesScreen.tsx`:
- Around line 33-76: Update ChangeGroup’s dynamic t(meta.labelKey) call to use
explicit branches for each ChangeKind, with statically detectable
t("draft_changes_added"), t("draft_changes_removed"), and
t("draft_changes_updated") calls. Preserve the existing KIND_META styling and
label behavior while ensuring all three translation keys remain discoverable by
i18n cleanup.
- Around line 105-143: Update the blocking screen around the component rendering
this questionnaire change content to use the Shadcn/Radix Dialog primitive
instead of a standalone fixed layout. Render the existing title and description
through DialogTitle and DialogDescription, keep the changes and Continue action
inside DialogContent, and configure it as a non-dismissible modal so focus is
trapped and the background questionnaire is inert while preserving onContinue
behavior.
In `@src/components/Questionnaire/QuestionnaireForm.tsx`:
- Around line 694-700: Update the reconciliation flow in QuestionnaireForm so a
fetch timeout with any applicable response still lacking context does not
complete reconciliation. Do not allow the existing changes/finalization logic
around the responses mapping and lines 735-738 to clear the pending state;
instead retain the blocked state and expose the existing retry or error state
until every applicable context has been reconciled.
In `@src/components/Questionnaire/structured/contextMatch.test.ts`:
- Around line 92-108: Remove the performance.now timing setup and the 16ms
assertion from the “scales to 100 large records well under a frame” test, while
retaining the large-record inputs and the functional d.matches assertion. Leave
timing validation to a separate benchmark.
- Around line 8-9: Convert the pure fixture helpers rec and big in the
questionnaire context-match tests from arrow-function assignments to function
declarations, preserving their parameters, return values, and behavior.
In `@src/components/Questionnaire/structured/contextMatch.ts`:
- Around line 3-8: Replace the RecordLike type alias with an interface while
preserving its optional id and dirty properties and its string-indexed unknown
values. Keep the existing RecordLike name and documentation unchanged.
- Around line 10-19: Remove "recorded_date" from VOLATILE_FIELDS so changes to
this clinical date are treated as content drift, while continuing to ignore the
remaining server-managed audit fields. Add a regression test for the
context-matching flow where only recorded_date differs and assert that
changed.length equals 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05015638-ecc5-4b46-9e1b-2c781bf7d28b
📒 Files selected for processing (16)
public/locale/en.jsonsrc/components/Questionnaire/DraftContextChangesScreen.tsxsrc/components/Questionnaire/QuestionRenderer.tsxsrc/components/Questionnaire/QuestionTypes/AllergyQuestion.tsxsrc/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsxsrc/components/Questionnaire/QuestionTypes/EncounterQuestion.tsxsrc/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsxsrc/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsxsrc/components/Questionnaire/QuestionTypes/QuestionGroup.tsxsrc/components/Questionnaire/QuestionTypes/QuestionInput.tsxsrc/components/Questionnaire/QuestionTypes/SymptomQuestion.tsxsrc/components/Questionnaire/QuestionnaireForm.tsxsrc/components/Questionnaire/structured/contextMatch.test.tssrc/components/Questionnaire/structured/contextMatch.tssrc/pages/Encounters/tabs/overview/FormSubmissionDrafts.tsxsrc/types/questionnaire/form.ts
| const KIND_META: Record< | ||
| ChangeKind, | ||
| { labelKey: string; icon: IconName; dot: string; label: string } | ||
| > = { | ||
| added: { | ||
| labelKey: "draft_changes_added", | ||
| icon: "l-plus-circle", | ||
| dot: "bg-green-500", | ||
| label: "text-green-700", | ||
| }, | ||
| removed: { | ||
| labelKey: "draft_changes_removed", | ||
| icon: "l-minus-circle", | ||
| dot: "bg-red-500", | ||
| label: "text-red-700", | ||
| }, | ||
| changed: { | ||
| labelKey: "draft_changes_updated", | ||
| icon: "l-sync", | ||
| dot: "bg-amber-500", | ||
| label: "text-amber-700", | ||
| }, | ||
| }; | ||
|
|
||
| function ChangeGroup({ | ||
| kind, | ||
| records, | ||
| }: { | ||
| kind: ChangeKind; | ||
| records: RecordLike[]; | ||
| }) { | ||
| const { t } = useTranslation(); | ||
| if (!records.length) return null; | ||
| const meta = KIND_META[kind]; | ||
| return ( | ||
| <div className="space-y-1.5"> | ||
| <div | ||
| className={cn( | ||
| "flex items-center gap-1.5 text-xs font-semibold uppercase tracking-wide", | ||
| meta.label, | ||
| )} | ||
| > | ||
| <CareIcon icon={meta.icon} className="size-3.5" /> | ||
| {t(meta.labelKey)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use literal t() calls so these keys survive i18n cleanup.
t(meta.labelKey) prevents remove-unused-i18n.js from statically discovering the three translation keys. Resolve the label with explicit branches containing t("draft_changes_added"), t("draft_changes_removed"), and t("draft_changes_updated").
Based on learnings, translation keys must be passed through statically detectable t() calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Questionnaire/DraftContextChangesScreen.tsx` around lines 33 -
76, Update ChangeGroup’s dynamic t(meta.labelKey) call to use explicit branches
for each ChangeKind, with statically detectable t("draft_changes_added"),
t("draft_changes_removed"), and t("draft_changes_updated") calls. Preserve the
existing KIND_META styling and label behavior while ensuring all three
translation keys remain discoverable by i18n cleanup.
Source: Learnings
| return ( | ||
| <div className="flex min-h-screen items-center justify-center p-4"> | ||
| <div className="w-full max-w-lg space-y-6 rounded-xl border border-gray-200 bg-white p-6 shadow-lg sm:p-8"> | ||
| <div className="space-y-3 text-center"> | ||
| <div className="mx-auto flex size-12 items-center justify-center rounded-full bg-amber-100"> | ||
| <CareIcon | ||
| icon="l-exclamation-triangle" | ||
| className="size-6 text-amber-600" | ||
| /> | ||
| </div> | ||
| <h2 className="text-lg font-semibold text-gray-900"> | ||
| {t("draft_data_changed_title")} | ||
| </h2> | ||
| <p className="text-sm text-gray-500"> | ||
| {t("draft_data_changed_description")} | ||
| </p> | ||
| </div> | ||
|
|
||
| <div className="space-y-3"> | ||
| {changes.map((change) => ( | ||
| <div | ||
| key={change.questionId} | ||
| className="space-y-3 rounded-lg border border-gray-200 bg-gray-50 p-4" | ||
| > | ||
| <h3 className="text-sm font-semibold text-gray-900"> | ||
| {change.title} | ||
| </h3> | ||
| <ChangeGroup kind="added" records={change.added} /> | ||
| <ChangeGroup kind="removed" records={change.removed} /> | ||
| <ChangeGroup kind="changed" records={change.changed} /> | ||
| </div> | ||
| ))} | ||
| </div> | ||
|
|
||
| <Button onClick={onContinue} className="w-full"> | ||
| {t("continue")} | ||
| </Button> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Implement this blocking screen as an accessible modal.
The fixed overlay has no dialog semantics, initial focus, focus trap, or background inertness. Keyboard users can tab into and activate the questionnaire underneath. Use the Shadcn/Radix Dialog primitive with an accessible title and description.
As per coding guidelines, components must use Shadcn primitives and provide proper ARIA attributes and keyboard navigation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Questionnaire/DraftContextChangesScreen.tsx` around lines 105
- 143, Update the blocking screen around the component rendering this
questionnaire change content to use the Shadcn/Radix Dialog primitive instead of
a standalone fixed layout. Render the existing title and description through
DialogTitle and DialogDescription, keep the changes and Continue action inside
DialogContent, and configure it as a non-dismissible modal so focus is trapped
and the background questionnaire is inert while preserving onContinue behavior.
Source: Coding guidelines
| if (!allPopulated && !reconcileTimedOut) return; | ||
|
|
||
| const changes: DraftContextChange[] = []; | ||
| const responses = form.responses.map((r) => { | ||
| const snap = snapshot[r.question_id]; | ||
| // Skip if not applicable or fresh context never arrived (keep draft as-is). | ||
| if (!snap || r.context === undefined) return r; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not silently complete reconciliation after a fetch timeout.
When context remains undefined, Line 700 retains the stale draft records, but Lines 735-738 still end reconciliation. The user can then submit records that were removed or changed on the server without any warning. Keep the flow blocked and expose a retry/error state until every applicable context is reconciled.
Also applies to: 735-738
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Questionnaire/QuestionnaireForm.tsx` around lines 694 - 700,
Update the reconciliation flow in QuestionnaireForm so a fetch timeout with any
applicable response still lacking context does not complete reconciliation. Do
not allow the existing changes/finalization logic around the responses mapping
and lines 735-738 to clear the pending state; instead retain the blocked state
and expose the existing retry or error state until every applicable context has
been reconciled.
| const rec = (id: string, extra: Record<string, unknown> = {}) => | ||
| ({ id, code: { display: id }, ...extra }) as unknown as ResponseContext; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use function declarations for pure fixtures.
rec and big are pure helpers and should use the required declaration style.
As per coding guidelines: “Use the function keyword for pure functions.”
Also applies to: 93-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Questionnaire/structured/contextMatch.test.ts` around lines 8
- 9, Convert the pure fixture helpers rec and big in the questionnaire
context-match tests from arrow-function assignments to function declarations,
preserving their parameters, return values, and behavior.
Source: Coding guidelines
| test("scales to 100 large records well under a frame", () => { | ||
| const big = (id: string) => | ||
| rec(id, { | ||
| code: { display: id, system: "http://x", code: id }, | ||
| created_by: { id: "u", username: "u", first_name: "a", last_name: "b" }, | ||
| created_date: "2020", | ||
| modified_date: "2020", | ||
| onset: { onset_datetime: "2020", note: "x".repeat(50) }, | ||
| nested: { a: [1, 2, 3], b: { c: { d: id } } }, | ||
| }); | ||
| const draft = Array.from({ length: 100 }, (_, i) => big(String(i))); | ||
| const fresh = Array.from({ length: 100 }, (_, i) => big(String(i))); | ||
| const start = performance.now(); | ||
| const d = diffResponseContext(draft, fresh); | ||
| const ms = performance.now() - start; | ||
| assert.equal(d.matches, true); | ||
| assert.ok(ms < 16, `diff took ${ms.toFixed(2)}ms (expected < 16ms)`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the wall-clock assertion from this unit test.
The 16ms limit can fail under CI contention or GC despite identical behavior. Keep the functional assertion and move timing checks to a dedicated benchmark.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Questionnaire/structured/contextMatch.test.ts` around lines 92
- 108, Remove the performance.now timing setup and the 16ms assertion from the
“scales to 100 large records well under a frame” test, while retaining the
large-record inputs and the functional d.matches assertion. Leave timing
validation to a separate benchmark.
| /** A structured value record (`*Request`) or read record — both carry these. */ | ||
| export type RecordLike = { | ||
| id?: string; | ||
| dirty?: boolean; | ||
| [key: string]: unknown; | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use an interface for RecordLike.
This is an object shape and should follow the repository convention.
Proposed fix
-export type RecordLike = {
+export interface RecordLike {
id?: string;
dirty?: boolean;
[key: string]: unknown;
-};
+}As per coding guidelines: “Use interface for object type definitions.”
📝 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.
| /** A structured value record (`*Request`) or read record — both carry these. */ | |
| export type RecordLike = { | |
| id?: string; | |
| dirty?: boolean; | |
| [key: string]: unknown; | |
| }; | |
| /** A structured value record (`*Request`) or read record — both carry these. */ | |
| export interface RecordLike { | |
| id?: string; | |
| dirty?: boolean; | |
| [key: string]: unknown; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Questionnaire/structured/contextMatch.ts` around lines 3 - 8,
Replace the RecordLike type alias with an interface while preserving its
optional id and dirty properties and its string-indexed unknown values. Keep the
existing RecordLike name and documentation unchanged.
Source: Coding guidelines
| // Server-managed audit fields (who/when) that must not count as content drift. | ||
| const VOLATILE_FIELDS = new Set([ | ||
| "modified_date", | ||
| "created_date", | ||
| "updated_date", | ||
| "recorded_date", | ||
| "created_by", | ||
| "updated_by", | ||
| "modified_by", | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not ignore recorded_date changes.
recorded_date is present on diagnosis and symptom records. Excluding it makes a server-side clinical-date change return matches: true, so draft recovery will not notify the user.
Proposed fix
"updated_date",
- "recorded_date",
"created_by",Add a regression test where only recorded_date differs and assert changed.length === 1.
Based on PR objective: server-side record changes must be surfaced during draft recovery.
📝 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.
| // Server-managed audit fields (who/when) that must not count as content drift. | |
| const VOLATILE_FIELDS = new Set([ | |
| "modified_date", | |
| "created_date", | |
| "updated_date", | |
| "recorded_date", | |
| "created_by", | |
| "updated_by", | |
| "modified_by", | |
| ]); | |
| // Server-managed audit fields (who/when) that must not count as content drift. | |
| const VOLATILE_FIELDS = new Set([ | |
| "modified_date", | |
| "created_date", | |
| "updated_date", | |
| "created_by", | |
| "updated_by", | |
| "modified_by", | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Questionnaire/structured/contextMatch.ts` around lines 10 -
19, Remove "recorded_date" from VOLATILE_FIELDS so changes to this clinical date
are treated as content drift, while continuing to ignore the remaining
server-managed audit fields. Add a regression test for the context-matching flow
where only recorded_date differs and assert that changed.length equals 1.
There was a problem hiding this comment.
Pull request overview
Adds draft-recovery support for structured questionnaire questions by snapshotting “fresh” server context into drafts, diffing it on recovery, and reconciling draft edits against current server records (with a user-facing warning screen when drift is detected).
Changes:
- Extend questionnaire response typing to include a draft-only
contextsnapshot and a canonical list of context-bearing structured types. - Wire
setResponseContextthrough the questionnaire renderer so structured question components can populate draft context from their fetches. - Add context diff/merge helpers plus a recovery overlay screen that summarizes server-side changes; add i18n strings for the new UI.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/questionnaire/form.ts | Adds ResponseContext, CONTEXT_STRUCTURED_TYPES, and optional QuestionnaireResponse.context. |
| src/pages/Encounters/tabs/overview/FormSubmissionDrafts.tsx | Passes the new required setResponseContext prop to QuestionRenderer (noop for read-only draft preview). |
| src/components/Questionnaire/structured/contextMatch.ts | Introduces context diffing (ignoring volatile audit fields) and value-merge logic for draft recovery. |
| src/components/Questionnaire/structured/contextMatch.test.ts | Adds unit tests for the context diff/merge behavior. |
| src/components/Questionnaire/QuestionTypes/SymptomQuestion.tsx | Populates response context from fetched symptom records. |
| src/components/Questionnaire/QuestionTypes/QuestionInput.tsx | Threads setResponseContext down to question components via shared props. |
| src/components/Questionnaire/QuestionTypes/QuestionGroup.tsx | Propagates setResponseContext through grouped/sub questions. |
| src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx | Populates response context from fetched medication statement records. |
| src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx | Populates response context; explicitly sets empty context when no prescription exists. |
| src/components/Questionnaire/QuestionTypes/EncounterQuestion.tsx | Stores fetched encounter record into response context. |
| src/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsx | Populates response context from fetched diagnosis records. |
| src/components/Questionnaire/QuestionTypes/AllergyQuestion.tsx | Populates response context from fetched allergy intolerance records. |
| src/components/Questionnaire/QuestionRenderer.tsx | Adds and forwards setResponseContext prop. |
| src/components/Questionnaire/QuestionnaireForm.tsx | Implements draft recovery reconciliation flow, timeout handling, and change-warning overlay UI. |
| src/components/Questionnaire/DraftContextChangesScreen.tsx | New UI for summarizing “added/removed/changed” server-side drift when recovering drafts. |
| public/locale/en.json | Adds English strings for the draft-context change screen. |
| import assert from "node:assert/strict"; | ||
| import { test } from "node:test"; | ||
|
|
||
| import { ResponseContext } from "@/types/questionnaire/form"; | ||
|
|
||
| import { diffResponseContext, mergeRecoveredValues } from "./contextMatch"; | ||
|
|
| const d = diffResponseContext(draft, fresh); | ||
| const ms = performance.now() - start; | ||
| assert.equal(d.matches, true); | ||
| assert.ok(ms < 16, `diff took ${ms.toFixed(2)}ms (expected < 16ms)`); |
🎭 Playwright Test ResultsStatus: ✅ Passed
📊 Detailed results are available in the playwright-final-report artifact. Run: #10487 |
Greptile SummaryThis PR adds draft-save support for questionnaires that contain structured questions (diagnosis, symptom, allergy, medication request/statement, encounter). Previously these forms were excluded from drafts entirely. The new approach saves a context snapshot alongside the draft, and on recovery mounts the form underneath a loading overlay, lets each structured component re-fetch fresh server data, then diffs the saved snapshot against live data — showing a change-summary screen if server-side records drifted while the draft was held.
Confidence Score: 4/5Safe to merge with the noted caveats addressed; the core reconciliation logic is correct and well-tested, and the changes are isolated to the draft-recovery path. The reconciliation state machine and the pure utility functions in contextMatch.ts are well-reasoned and covered by unit tests. The main concerns are: the inline setResponseContext callback defeats QuestionGroup's memo causing excess re-renders during recovery, EncounterQuestion never signals context on a failed/absent fetch causing an 8-second penalty, and the context field on QuestionnaireResponse lacks an explicit strip before form submission. None of these break correctness for the happy path, but the submission concern warrants verification before shipping to production. QuestionnaireForm.tsx (inline callback and reconcile effect deps) and EncounterQuestion.tsx (missing context signal on failed fetch) deserve a second look before merge. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| setIsDirty(true); | ||
| } | ||
| }} | ||
| setResponseContext={( | ||
| questionId: string, | ||
| context: ResponseContext[], | ||
| ) => { | ||
| // Derived-from-fetch snapshot: preserve values/note, and don't | ||
| // mark the form dirty (would trip the unsaved-changes prompt). | ||
| setQuestionnaireForms((existingForms) => | ||
| existingForms.map((formItem) => | ||
| formItem.questionnaire.id === form.questionnaire.id | ||
| ? { | ||
| ...formItem, | ||
| responses: formItem.responses.map((r) => |
There was a problem hiding this comment.
setResponseContext defeats QuestionGroup memoization
The callback is defined inline without useCallback, so a new function reference is created on every render of QuestionnaireForm. During the reconcile phase, each setResponseContext call triggers setQuestionnaireForms, which re-renders QuestionnaireForm, which produces a fresh reference — passed all the way down to QuestionGroup (which is wrapped in memo), causing every group to re-render. For N structured questions this means O(N²) unnecessary question-group re-renders. Wrap the callback in useCallback with a stable functional-update helper that doesn't close over questionnaireForms at all.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| setReconcile( | ||
| changes.length ? { phase: "changed", changes } : { phase: "done" }, | ||
| ); | ||
| }, [reconcile.phase, questionnaireForms, reconcileTimedOut]); |
There was a problem hiding this comment.
questionnaireForms in reconcile effect deps causes redundant intermediate runs
The effect depends on questionnaireForms, which is updated by every setResponseContext call. This means the effect runs once per structured question as each fetch completes, hitting the allPopulated check and returning early N-1 times before the final run does real work. After the final run calls both setQuestionnaireForms and setReconcile, React 18 batches those updates, so the effect fires one extra time with reconcile.phase === "done" where it early-returns. A useRef flag (hasReconciled) set before the state updates would eliminate the extra run and make the intent explicit.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/locale/en.json (1)
2052-2056: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAppend new locale keys to the end of the file.
These newly added draft keys are inserted in the middle of
public/locale/en.json. Move them to the end of the English locale object; do not edit non-English locale files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/locale/en.json` around lines 2052 - 2056, Move the draft-related locale keys draft_changes_added, draft_changes_removed, draft_changes_updated, draft_data_changed_description, and draft_data_changed_title from their current position to the end of the English locale object in public/locale/en.json. Preserve their keys and values exactly, and do not modify any non-English locale files.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@public/locale/en.json`:
- Around line 2052-2056: Move the draft-related locale keys draft_changes_added,
draft_changes_removed, draft_changes_updated, draft_data_changed_description,
and draft_data_changed_title from their current position to the end of the
English locale object in public/locale/en.json. Preserve their keys and values
exactly, and do not modify any non-English locale files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c85e91b7-a123-479c-9473-c21c7531dde7
📒 Files selected for processing (1)
public/locale/en.json
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/components/Questionnaire/structured/contextMatch.test.ts:6
- This unit test file won’t be executed by the current unit test runner (
npm run test:unitonly runsnode --test "plugins/**/*.test.ts"). As-is, it won’t provide CI coverage for the new reconciliation logic.
import assert from "node:assert/strict";
import { test } from "node:test";
import { ResponseContext } from "@/types/questionnaire/form";
import { diffResponseContext, mergeRecoveredValues } from "./contextMatch";
src/components/Questionnaire/structured/contextMatch.ts:19
diffResponseContextis intended to ignore server-managed audit fields, but AllergyIntolerance read records includeedited_by(not present in the request type) and it currently counts as a meaningful change. This can cause false “Updated on the server” warnings when onlyedited_bychanges.
// Server-managed audit fields (who/when) that must not count as content drift.
const VOLATILE_FIELDS = new Set([
"modified_date",
"created_date",
"updated_date",
"recorded_date",
"created_by",
"updated_by",
"modified_by",
]);
src/components/Questionnaire/QuestionnaireForm.tsx:588
- Now that drafts can include structured-question context snapshots, the “continue draft → submit” path will upload/persist that context unless it’s explicitly stripped when converting the draft to a submitted form_submission. Since
contextis meant for draft reconciliation only, consider omitting it (and any other draft-only fields) when building the submitted payload/response_dump to avoid transmitting/storing raw fetched records.
// Single-questionnaire drafts (structured questions supported via context
// reconciliation on recovery). Structured edit questionnaires (diagnosis,
// service_request, allergy_intolerance, …) are frontend-only — their slug is
// not a real backend questionnaire — so they can't be saved as drafts.
const isDraftSaveable = useMemo(() => {
if (!careConfig.enableQuestionnaireDraft) {
return false;
}
if (!questionnaireSlug || FIXED_QUESTIONNAIRES[questionnaireSlug]) {
return false;
}
return questionnaireForms.length <= 1;
ohc-project-manager
Bot
added
Changes Required
and removed
needs review
needs testing
labels
nihal467
left a comment
nihal467
left a comment
There was a problem hiding this comment.
When a user adds Common Cold to the symptoms and saves the form as a draft, then adds Common Cold again directly in the Symptoms section, reopening the draft triggers the duplicate detection pop-up indicating that the symptoms have changed.
After clicking Continue, the Symptoms section displays Common Cold twice. The user can then submit the form successfully, allowing the frontend duplicate validation for symptoms to be bypassed.
Proposed Changes
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit