Questionnaire actions - #16618
Conversation
…utes Entire-Checkpoint: cfd207b56c6f
Entire-Checkpoint: 7d9092f73d99
…y to writable fields - useForm: add resetOptions.keepDirtyValues so a background refetch (e.g. triggered by a reorder save invalidating ["questionnairesV2"]) no longer wipes an in-progress unsaved title/slug/description/status edit. - buildUpdateBody: construct the PUT body explicitly from only the writable QuestionnaireUpdate/QuestionnaireBase fields instead of spreading the full QuestionnaireRead, so read-only fields (id, auth_context, internal_revision, created_by, updated_by, modified_date) are never sent. - QuestionOverviewList: disable the reorder up/down buttons while the save mutation is pending to avoid a double-click lost-update race. - QuestionOverviewList: use a translated action label for the expand/collapse chevron instead of the question text; add toggle_sub_questions key. - Add a "versions" i18n key and use it for the versions tab trigger instead of the singular "version".
Entire-Checkpoint: 1ef0119d1d60
…to satisfy unique component auto-registration Entire-Checkpoint: a02b0db6d749
…, self-moves, and nested-selection duplication Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ree-wide move targets Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…enderer Adds the QuestionnaireBuilderPage route (Edit|Preview toggle, validated save flow, sticky footer navigation) and mounts it above the detail route in both the facility settings and admin routers. Preview renders the real QuestionnaireRenderer against the in-progress draft. Also: extracts buildUpdateBody into a shared manage/ module reused by both the detail page and the builder; fixes the Question Title label/input association in QuestionEditorCard so it has a proper accessible name; and hides QuestionnaireRenderer's tree nav when there is only one top-level question, since a single-item nav duplicated the active question's title next to its own field label.
…rd dirty resets, dedupe tree helpers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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)
388-558: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAppend new English locale keys at the end of the file.
The new keys are inserted into alphabetical positions throughout
public/locale/en.json. Move the added keys to the end of the English locale file. Keep non-English locale files unchanged.As per coding guidelines, “When an i18n key is missing, append the key to the end of the English locale file; do not directly edit non-English locale files.”
Also applies to: 648-714, 1008-1008, 1189-1312, 1392-1432, 1536-1578, 1717-1789, 1828-1829, 2010-2049, 2117-2394, 2541-2595, 2698-2864, 3110-3134, 3284-3303, 3549-3561, 3860-3971, 4135-4230, 4459-4460, 4547-4752, 4773-4910, 5183-5401, 5531-5639, 5732-5786, 5819-5945, 6082-6172, 6293-6320, 6374-6380, 6458-6458, 6562-6563
🤖 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 388 - 558, Move all newly added translation keys in public/locale/en.json from their alphabetical insertion points to the end of the file, preserving each key-value pair and valid JSON formatting; leave all non-English locale files unchanged.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.
Inline comments:
In `@tests/facility/settings/questionnaires/questionnaireBuilderMatrix.spec.ts`:
- Around line 298-343: Replace the styling-dependent locator in the “Preview
shows the required asterisk” step with a semantic or accessible selector that
identifies the required-field marker. Update the assertion around the question
label without relying on the span.text-red-500 Tailwind class, preserving
verification that the required marker is visible.
In
`@tests/facility/settings/questionnaires/questionnaireConditionsAuthoring.spec.ts`:
- Around line 34-37: Update conditionGrid to locate the condition editor using a
stable semantic selector or accessible test hook from the rendered condition UI
instead of matching the Tailwind class substring sm:grid-cols-2. Keep the
helper’s scope limited to identifying the field grid and preserve its existing
Page locator behavior.
In `@tests/facility/settings/questionnaires/questionnaireDetailActions.spec.ts`:
- Around line 77-103: Replace the generic div and Tailwind-class row selectors
in the questionnaire ordering test with stable data-slot-based selectors,
including rows, firstRow, and firstOrdinalRow. Reuse the existing data-slot
selector convention and preserve the current text and ordinal filtering
behavior.
In
`@tests/facility/settings/questionnaires/questionnaireEnableWhenMatrix.spec.ts`:
- Around line 19-36: Extract openKitchenSinkPreview, navRow, and jumpTo into
tests/helper/questionnaireV2.ts, then import and reuse them in
tests/facility/settings/questionnaires/questionnaireEnableWhenMatrix.spec.ts
(lines 19-36) and remove its local definitions. In
tests/facility/settings/questionnaires/questionnairePreviewTypes.spec.ts (lines
18-33), remove the duplicated openKitchenSinkPreview and jumpTo helpers and
import the shared implementations.
In `@tests/helper/questionnaireV2.ts`:
- Around line 38-65: Update getQuestionnaireIdBySlug to follow the questionnaire
list endpoint’s pagination, checking each page’s results for the requested slug
and stopping immediately when found. Continue requesting the next page until
next is absent, then throw the existing not-found error only after all pages
have been searched; preserve the existing response-error handling and cache
behavior.
---
Outside diff comments:
In `@public/locale/en.json`:
- Around line 388-558: Move all newly added translation keys in
public/locale/en.json from their alphabetical insertion points to the end of the
file, preserving each key-value pair and valid JSON formatting; leave all
non-English locale files unchanged.
🪄 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 Plus
Run ID: 55d93f98-cf57-4dbe-a48a-db1d6a75143a
📒 Files selected for processing (29)
public/locale/en.jsonsrc/components/Questionnaire/CodingEditor.tsxsrc/components/QuestionnaireV2/README.mdsrc/components/QuestionnaireV2/builder/QuestionCodingCard.tsxsrc/components/QuestionnaireV2/builder/QuestionEditorCard.tsxsrc/components/QuestionnaireV2/useCanWriteQuestionnaire.tssrc/pages/Facility/utils/useCurrentFacility.tsxtests/admin/questionnaires/questionnaireOrganizations.spec.tstests/admin/questionnaires/questionnairePermissions.spec.tstests/admin/questionnaires/questionnaireScoping.spec.tstests/admin/questionnaires/questionnaireVersionsDepth.spec.tstests/facility/patient/encounter/forms/enableWhen/boolean.spec.tstests/facility/patient/encounter/forms/enableWhen/string.spec.tstests/facility/settings/questionnaires/questionnaireBuilder.spec.tstests/facility/settings/questionnaires/questionnaireBuilderGroups.spec.tstests/facility/settings/questionnaires/questionnaireBuilderMatrix.spec.tstests/facility/settings/questionnaires/questionnaireBuilderNavigation.spec.tstests/facility/settings/questionnaires/questionnaireConditionsAuthoring.spec.tstests/facility/settings/questionnaires/questionnaireCreateValidation.spec.tstests/facility/settings/questionnaires/questionnaireDetailActions.spec.tstests/facility/settings/questionnaires/questionnaireEnableWhenMatrix.spec.tstests/facility/settings/questionnaires/questionnaireErrorHandling.spec.tstests/facility/settings/questionnaires/questionnaireImportUrl.spec.tstests/facility/settings/questionnaires/questionnaireMobile.spec.tstests/facility/settings/questionnaires/questionnairePagination.spec.tstests/facility/settings/questionnaires/questionnairePreviewTypes.spec.tstests/helper/questionnaireV2.tstests/setup/questionnaire.setup.tstests/support/questionnaireId.ts
💤 Files with no reviewable changes (1)
- src/components/Questionnaire/CodingEditor.tsx
| test("required flag renders an asterisk; repeats hides for boolean and clears on type change", async ({ | ||
| page, | ||
| }) => { | ||
| const facilityId = getFacilityId(); | ||
| const questionTitle = `Mandatory field ${Date.now()}`; | ||
|
|
||
| await createQuestionnaireAndOpenBuilder(page, { | ||
| basePath: `/facility/${facilityId}/settings/questionnaires`, | ||
| title: `QV2 Flags ${Date.now()}`, | ||
| }); | ||
| await addQuestion(page, questionTitle); | ||
|
|
||
| const repeatable = page.getByRole("checkbox", { name: "Repeatable" }); | ||
|
|
||
| await test.step("Mark Required and Repeatable", async () => { | ||
| await page.getByRole("button", { name: BEHAVIOUR_CARD_NAME }).click(); | ||
| await page.getByRole("checkbox", { name: "Required" }).click(); | ||
| await repeatable.click(); | ||
| await expect(repeatable).toHaveAttribute("aria-checked", "true"); | ||
| await expect(page.getByText("Configured • 2")).toBeVisible(); | ||
| }); | ||
|
|
||
| await test.step("Boolean never offers Repeats — and clears the flag", async () => { | ||
| await pickType(page, "boolean"); | ||
| await expect(repeatable).not.toBeVisible(); | ||
| await expect( | ||
| page.getByRole("checkbox", { name: "Required" }), | ||
| ).toHaveAttribute("aria-checked", "true"); | ||
|
|
||
| await pickType(page, "string"); | ||
| await expect(repeatable).toBeVisible(); | ||
| await expect(repeatable).toHaveAttribute("aria-checked", "false"); | ||
| await expect(page.getByText("Configured • 1")).toBeVisible(); | ||
| }); | ||
|
|
||
| await test.step("Preview shows the required asterisk", async () => { | ||
| await page.getByRole("button", { name: "Save Changes" }).click(); | ||
| await expectToast(page, "Questionnaire updated successfully"); | ||
| await page.getByRole("button", { name: "Preview" }).click(); | ||
| await expect( | ||
| page.locator("label").filter({ hasText: questionTitle }), | ||
| ).toBeVisible(); | ||
| await expect(page.locator("span.text-red-500")).toBeVisible(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial
Selecting the required-field marker by raw Tailwind class is brittle.
Line 340 locates the required asterisk with page.locator("span.text-red-500"). This ties test correctness to a styling implementation detail. This finding is addressed together with the conditionGrid selector in questionnaireConditionsAuthoring.spec.ts in the consolidated comment below.
🤖 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 `@tests/facility/settings/questionnaires/questionnaireBuilderMatrix.spec.ts`
around lines 298 - 343, Replace the styling-dependent locator in the “Preview
shows the required asterisk” step with a semantic or accessible selector that
identifies the required-field marker. Update the assertion around the question
label without relying on the span.text-red-500 Tailwind class, preserving
verification that the required marker is visible.
Source: Learnings
| /** The condition editor's field grid (question/operator/answer). */ | ||
| function conditionGrid(page: Page) { | ||
| return page.locator('div[class*="sm:grid-cols-2"]'); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial
Selecting elements by raw Tailwind class substring is brittle.
conditionGrid locates the condition row by matching the class attribute against sm:grid-cols-2. A future layout change to VisibilityConditionsCard.tsx (e.g., a different Tailwind breakpoint or grid column count) breaks this test without any change to test intent. This finding is addressed together with a similar pattern in questionnaireBuilderMatrix.spec.ts in the consolidated comment below.
🤖 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
`@tests/facility/settings/questionnaires/questionnaireConditionsAuthoring.spec.ts`
around lines 34 - 37, Update conditionGrid to locate the condition editor using
a stable semantic selector or accessible test hook from the rendered condition
UI instead of matching the Tailwind class substring sm:grid-cols-2. Keep the
helper’s scope limited to identifying the field grid and preserve its existing
Page locator behavior.
Source: Learnings
| const rows = page.locator("div").filter({ hasText: /^1\./ }); | ||
|
|
||
| await test.step("Move the first question down via its kebab", async () => { | ||
| await expect(page.getByText(first)).toBeVisible(); | ||
| const firstRow = page | ||
| .locator("div") | ||
| .filter({ has: page.getByText(first, { exact: true }) }) | ||
| .filter({ has: page.getByRole("button", { name: "More options" }) }) | ||
| .last(); | ||
| await firstRow.getByRole("button", { name: "More options" }).click(); | ||
| await page.getByRole("menuitem", { name: "Move Down" }).click(); | ||
| await expectToast(page, "Questionnaire updated successfully"); | ||
| }); | ||
|
|
||
| await test.step("The order is persisted after reload", async () => { | ||
| await page.reload(); | ||
| await expect(rows.last()).toContainText(second); | ||
| const orderedTitles = page.locator( | ||
| '[data-slot="badge"]:has-text("String")', | ||
| ); | ||
| await expect(orderedTitles).toHaveCount(2); | ||
| // Row 1 now holds the previously-second question. | ||
| const firstOrdinalRow = page | ||
| .locator("div.min-w-0.flex-1", { hasText: /^1\./ }) | ||
| .first(); | ||
| await expect(firstOrdinalRow).toContainText(second); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use stable data-slot selectors instead of generic div/Tailwind-class chains.
The row locators at lines 77, 81-85, and 99-102 match on generic div elements filtered by text content, or on Tailwind utility classes (div.min-w-0.flex-1). Both approaches are fragile: nested ancestor divs all satisfy hasText/has filters, and any future Tailwind class rename breaks the test without a real behavior change.
The same test already uses a stable pattern at lines 94-97 ([data-slot="badge"]:has-text("String")). Apply the same data-slot convention to the row locators.
Based on learnings, "In the ohcnetwork/care_fe repository, prefer using data-slot attributes for test selectors over data-testid... Ensure selectors query by data-slot".
🤖 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 `@tests/facility/settings/questionnaires/questionnaireDetailActions.spec.ts`
around lines 77 - 103, Replace the generic div and Tailwind-class row selectors
in the questionnaire ordering test with stable data-slot-based selectors,
including rows, firstRow, and firstOrdinalRow. Reuse the existing data-slot
selector convention and preserve the current text and ordinal filtering
behavior.
Source: Learnings
| async function openKitchenSinkPreview(page: Page): Promise<void> { | ||
| const facilityId = getFacilityId(); | ||
| const id = await getQuestionnaireIdBySlug(KITCHEN_SINK_FACILITY_SLUG); | ||
| await page.goto( | ||
| `/facility/${facilityId}/settings/questionnaires/${id}/edit?mode=preview`, | ||
| ); | ||
| await expect(page.getByRole("navigation")).toBeVisible(); | ||
| } | ||
|
|
||
| function navRow(page: Page, questionText: string): Locator { | ||
| return page | ||
| .getByRole("navigation") | ||
| .getByRole("button", { name: questionText }); | ||
| } | ||
|
|
||
| async function jumpTo(page: Page, questionText: string): Promise<void> { | ||
| await navRow(page, questionText).click(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the duplicated kitchen-sink preview navigation helpers to the shared test helper module. Both files define openKitchenSinkPreview with an identical body, and both define a jumpTo helper that performs the same tree-nav button click; both files already import other helpers (getQuestionnaireIdBySlug, slug constants) from tests/helper/questionnaireV2, so the shared root cause is that this navigation logic was copy-pasted instead of being added there.
tests/facility/settings/questionnaires/questionnaireEnableWhenMatrix.spec.ts#L19-L36: moveopenKitchenSinkPreview,navRow, andjumpTointotests/helper/questionnaireV2.tsand import them here.tests/facility/settings/questionnaires/questionnairePreviewTypes.spec.ts#L18-L33: drop the localopenKitchenSinkPreview/jumpTodefinitions and import the same shared helpers instead of re-declaring them.
📍 Affects 2 files
tests/facility/settings/questionnaires/questionnaireEnableWhenMatrix.spec.ts#L19-L36(this comment)tests/facility/settings/questionnaires/questionnairePreviewTypes.spec.ts#L18-L33
🤖 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 `@tests/facility/settings/questionnaires/questionnaireEnableWhenMatrix.spec.ts`
around lines 19 - 36, Extract openKitchenSinkPreview, navRow, and jumpTo into
tests/helper/questionnaireV2.ts, then import and reuse them in
tests/facility/settings/questionnaires/questionnaireEnableWhenMatrix.spec.ts
(lines 19-36) and remove its local definitions. In
tests/facility/settings/questionnaires/questionnairePreviewTypes.spec.ts (lines
18-33), remove the duplicated openKitchenSinkPreview and jumpTo helpers and
import the shared implementations.
| const slugCache = new Map<string, string>(); | ||
|
|
||
| /** | ||
| * Resolves a fixture questionnaire's id from its slug. The detail endpoint | ||
| * looks up by external_id only (ENG-737 dropped slug lookup) and the list | ||
| * has no slug filter, so this lists broadly and matches client-side. | ||
| */ | ||
| export async function getQuestionnaireIdBySlug(slug: string): Promise<string> { | ||
| const cached = slugCache.get(slug); | ||
| if (cached) return cached; | ||
| const res = await fetch(`${apiBaseUrl()}/api/v1/questionnaire/?limit=100`, { | ||
| headers: adminApiHeaders(), | ||
| }); | ||
| if (!res.ok) { | ||
| throw new Error(`Failed to list questionnaires: ${res.status}`); | ||
| } | ||
| const data = (await res.json()) as { | ||
| results: { id: string; slug: string }[]; | ||
| }; | ||
| const match = data.results.find((entry) => entry.slug === slug); | ||
| if (!match) { | ||
| throw new Error( | ||
| `Fixture questionnaire "${slug}" not found — reload backend E2E fixtures`, | ||
| ); | ||
| } | ||
| slugCache.set(slug, match.id); | ||
| return match.id; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Django REST Framework's default LimitOffsetPagination response include a "next" field alongside "results"?
💡 Result:
Yes, the default response for Django REST Framework's LimitOffsetPagination includes a next field alongside the results [1][2]. When using LimitOffsetPagination, the API response is formatted as a dictionary containing the following keys: count, next, previous, and results [2][3][4]. The next field provides the URL for the next page of results, or null if no further results are available [2][3][4]. The structure of the response is as follows: { "count": 1023, "next": "https://api.example.org/accounts/?limit=100&offset=500", "previous": "https://api.example.org/accounts/?limit=100&offset=300", "results": [... ] } [1]
Citations:
- 1: https://www.django-rest-framework.org/api-guide/pagination/
- 2: https://github.com/tomchristie/django-rest-framework/blob/3.14.0/rest_framework/pagination.py
- 3: https://www.cdrf.co/3.16/rest_framework.pagination/LimitOffsetPagination.html
- 4: https://www.cdrf.co/3.15/rest_framework.pagination/LimitOffsetPagination.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -a 'questionnaireV2\.ts|questionnaire|Questionnaire|questionnaire' . | sed 's#^\./##' | head -200
echo
echo "== read helper =="
if [ -f tests/helper/questionnaireV2.ts ]; then
cat -n tests/helper/questionnaireV2.ts
else
fd -a 'questionnaireV2.ts' . | while read -r f; do echo "--- $f"; cat -n "$f"; done
fi
echo
echo "== questionnaire serializer/view files =="
rg -n "class .*Questionnaire|QuestionnaireSerializer|LimitOffsetPagination|DEFAULT_PAGINATION_CLASS|questionnaire|limit=100|offset" -S --glob '*.py' . | head -300Repository: ohcnetwork/care_fe
Length of output: 10137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== questionnaire helper usage and create helpers =="
cat -n tests/helper/questionnaire.ts
echo
rg -n "getQuestionnaireIdBySlug|createQuestionnaire|PAGINATION_TITLE_PREFIX|PAGINATION_FIXTURE_COUNT|fixture questionnaire|Date\.now\(|faker" tests -S | head -400
echo
echo "== search Django/Questionnaire serializer/view =="
git ls-files | rg 'questionnaires?\.py|questionnaire' | head -200
rg -n "class .*Questionnaire|Questionnaire.*Serializer|QuestionnaireViewSet|LIMIT_OFFSET|Pagination|results|next|offset|limit=100" -S --glob '*.py' . | head -400 || trueRepository: ohcnetwork/care_fe
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all git-tracked questionnaires files =="
git ls-files | rg '(^|/)(questionnaires|questionnaire).*\.py$|questionnaire.*\.py$' | head -200
echo
echo "== exact questionnaires serializer/views with context =="
while IFS= read -r f; do
if [ ${`#f`} -lt 400 ]; then
echo "--- $f"
rg -n "class .*Questionnaire|Questionnaire.*Serializer|QuestionnaireViewSet|DEFAULT_PAGINATION_CLASS|Pagination|pagination|limit=100|offset|results|next" -S "$f" | head -200
fi
done < <(git ls-files | rg '(^|/)(questionnaires|questionnaire).*\.py$|questionnaire.*\.py$') || true
echo
echo "== exact questionnaires test usages of getQuestionnaireIdBySlug =="
for f in tests/admin/questionnaires/questionnairePermissions.spec.ts tests/admin/questionnaires/questionnaireVersionsDepth.spec.ts; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,140p' "$f" | cat -n
fi
doneRepository: ohcnetwork/care_fe
Length of output: 199
Page through all questionnaires before declaring the fixture missing.
getQuestionnaireIdBySlug only reads the first 100 results, but the list endpoint supports pagination. In a shared or accumulated E2E environment, a fixture can be on a later page, so this helper can throw while the fixture still exists. Read pages until next is absent or the slug is found.
🤖 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 `@tests/helper/questionnaireV2.ts` around lines 38 - 65, Update
getQuestionnaireIdBySlug to follow the questionnaire list endpoint’s pagination,
checking each page’s results for the requested slug and stopping immediately
when found. Continue requesting the next page until next is absent, then throw
the existing not-found error only after all pages have been searched; preserve
the existing response-error handling and cache behavior.
Restores the legacy fill-flow contract the Task-7 simplification dropped:
the person answering a quantity question picks a UCUM unit per answer via
a ValueSetSelect (system-ucum-units), instead of the unit being pinned to
the author's question.unit. The author-configured question.unit is now the
pre-selected default (backend persists `unit` only; `answer_unit` is
dropped by the Question spec, so nothing may read it), a picked unit is
written into the response entry as [{type:"quantity", value, unit: Code}]
exactly as the legacy QuantityQuestion did, and changing the value
preserves the picked unit. Repeats keep one unit per entry via the
positional withEntryAt writes.
The unit picker (and the coding picker for valueset-bound questions)
merges into the bordered input+note row; widths sit on wrapper divs
because the global button[role=combobox]{width:100%} rule overrides any
width utility on the trigger. The builder's unit row is relabeled
"Default Unit" with a hint, keeping its write to question.unit.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Preview (kitchen-sink fixture, read-only): the unit picker pre-selects the seeded default (milligram), entering a value keeps it, and picking a different unit through the live terminology search sticks while preserving the value — with a picker-opens fallback annotation if the sandbox returns no results (it matches display names, so the search term is "kilogram", not "kg"). New round-trip spec pins the backend contract: author a quantity question, set a Default Unit, save, full page reload (fresh GET) before asserting the builder shows it, then an API read asserts unit.system/ code/display round-trip verbatim and that answer_unit stays absent; the preview picker must seed from that reloaded state. Builder matrix assertions updated for the merged unit row and the Default Unit accessible name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 134 out of 135 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/facility/patient/encounter/forms/enableWhen/string.spec.ts:27
test.describe.configure({ mode: "default" })does not force serial execution underfullyParallel: true; it keeps the default (parallel) behavior. To prevent shared-encounter submissions from leaking across tests in this file, this should bemode: "serial".
tests/facility/patient/encounter/forms/enableWhen/boolean.spec.ts:23test.describe.configure({ mode: "default" })does not opt out offullyParallel: true(it keeps the default behavior, which remains parallel). This undermines the comment’s intent and can still allow cross-test leakage via shared encounter submissions.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large, well-structured PR. The reducer pattern, per-instance Jotai store, dirty-guard on refetch, and the legacy boolean enable_when migration are all done correctly. The save-validation module is a clean, extensible design.
Three issues worth fixing before merge:
-
Actual bug (
VisibilityConditionsCard): group questions are valid targets in the condition picker, but the renderer never records responses for groups → selecting one permanently hides the dependent question with no feedback. Filtertype !== "group"fromavailableTargets. -
Performance (
CloneQuestionnaireDialog): the Zod schema is reconstructed on every render. Wrap inuseMemo. -
Docs (
QuestionnaireDetailPage): the JSDoc block describingdownloadQuestionnaireJsonis orphaned abovemoveElement, leavingdownloadQuestionnaireJsonundocumented. Functions need reordering.
Everything else — the enable-when evaluation port from the legacy evaluator, the navigation prompt, the optimistic org toggle, the move-into-self guard in the reducer — looks solid. 🔥
Generated by Grumpy PR Reviewer for issue #16618 · 98.3 AIC · ⌖ 6.21 AIC · ⊞ 6.3K
| const enableBehavior = question.enable_behavior ?? "all"; | ||
| const excludedIds = new Set(collectIds(question)); | ||
| const availableTargets = flattenQuestions(allQuestions).filter( | ||
| (candidate) => !excludedIds.has(candidate.id), |
There was a problem hiding this comment.
Bug: group questions can be selected as visibility targets, permanently hiding the dependent question.
flattenQuestions includes every question, groups included. But initializeResponses skips groups (they have no answers), so responsesAtom never has an entry for a group's id. When the renderer evaluates enableWhen for a condition pointing at a group, response is undefined → evaluateEnableWhen returns false → the question is hidden forever. The author would have no way to know why their question disappeared.
Filter out group questions here:
const availableTargets = flattenQuestions(allQuestions).filter(
(candidate) =>
!excludedIds.has(candidate.id) && candidate.type !== "group",
);| const { t } = useTranslation(); | ||
| const queryClient = useQueryClient(); | ||
|
|
||
| const cloneSchema = questionnaireBasicSchema(t).pick({ |
There was a problem hiding this comment.
Performance: schema reconstructed on every render.
questionnaireBasicSchema(t).pick(...) runs a full Zod schema build + pick call on every render cycle of this dialog. Wrap it in useMemo:
const cloneSchema = useMemo(
() => questionnaireBasicSchema(t).pick({ title: true, slug: true }),
[t],
);While you're at it, the defaultValues object in useForm also re-creates every render — same fix applies.
| import { VersionsTab } from "./VersionsTab"; | ||
|
|
||
| /** | ||
| * Serializes only the questionnaire *definition* — the same writable field |
There was a problem hiding this comment.
Documentation: orphaned JSDoc attached to the wrong function.
The long comment at line 52 describes downloadQuestionnaireJson (data-URI export, created_by exclusion, etc.) but it sits directly above moveElement. The moveElement JSDoc at line 60 then overrides it as the actual attached doc. downloadQuestionnaireJson at line 70 ends up with no JSDoc at all.
Swap the declaration order — move downloadQuestionnaireJson before moveElement, or at least move the JSDoc comment to sit directly above the function it describes.
…he renderer
Verified legacy contract (develop QuestionnaireEditor/QuestionLabel):
question.unit exists on integer/decimal/choice/quantity and renders as a
({code}) suffix next to the question label; number inputs never had an
answer-time unit picker.
Quantity per owner UX directive: answer_value_set is the unit-choice
source. A bounded expansion (<=8 codes via the terminology $expand,
cached one-shot per valueset) renders every unit as a visible ChoiceChip
with the author's default pre-selected; larger or unexpandable sets keep
the search popover, now scoped to the question's valueset (global UCUM
only as fallback). The legacy renderer's answer_value_set-driven 'Type'
coding picker is removed — saved coding values still pass through
response entries untouched.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… for number and choice types Legacy contract (develop QuestionnaireEditor): the Custom/ValueSet mode switcher only ever rendered for choice — quantity always configured its answers through SelectOrCreateValueset. The v2 AnswerOptionsEditor wrongly offered a Custom Options table for quantity; it now renders a valueset-only Unit Options section with a live chips preview of what answerers will see and the existing Default Unit row. Picking a valueset actively clears grandfathered answer_option data. Integer/decimal/choice get the legacy unit row (UNIT_TYPES parity): a UCUM ValueSetSelect writing question.unit, surfaced next to the question label in fill mode. Save guard: the backend rejects quantity questions carrying neither answer_option nor answer_value_set (Question spec), so the builder now blocks that save with a clear message instead of a raw 400 — grandfathered custom-option quantities still pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: c3ea2537e36c
…ecs go valueset-only New questionnaireUnitSemantics spec: e2e-units fixture preview (integer /min and decimal Cel label suffixes; bounded e2e-dose-units quantity renders mg/g/kg as inline chips with the default pre-selected, picking a chip writes the unit, no popover offered), integer+decimal unit round-trip through save + hard reload + fresh API GET, and the client-side quantity-needs-valueset save guard. Updated quantity authoring specs to the valueset-only contract: assert Custom Options mode is NOT offered for quantity, author via the UCUM Units valueset, and pin answer_value_set (and empty answer_option) in the API round-trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- globalSetup now refreshes tokens for EVERY .auth storage state, not just user.json — with --no-deps the setup project never runs, so a stale nurse token failed all five permission specs once the JWT aged out while admin specs kept passing. - Nurse permission specs re-verified against a freshly seeded backend (migrate + fixtures + questionnaire E2E script): the nurse reads the org-tagged fixture questionnaires in the facility list (the old 'heigh-weight' slug only existed in a stale dev DB), and the admin mount lists none of them. Assertions updated to that deterministic state. - New pickValuesetFromAutocomplete helper scopes the valueset search to the opened popover/dialog — a bare [data-slot=command-input] first() can resolve to a different cmdk input on the builder page, leaving the list unfiltered (observed flake). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the cramped Versions-tab dialog with a dedicated read-only page at
{basePath}/{id}/versions/{revisionId} (both mounts) so a past revision
renders with the renderer's full layout. Back returns to the Versions tab
via ?tab=versions, now honored by the detail page.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: 9a5977bd3edd
…flow Open now navigates to the revision page: assert past-revision indicator, old content with disabled inputs, and Back landing on the Versions tab. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…up/display questions The renderer never records responses for group questions (initializeResponses skips them) and DisplayText never writes values, so an enable_when condition targeting either evaluates to "unanswered" forever and permanently hides the dependent question with zero feedback. - The target picker excludes non-response-producing types (group, display). Structured questions stay eligible: they DO record values (StructuredQuestionSlot writes through updateResponse), matching the legacy editor which offered every non-group question as a target. - Legacy/imported data already targeting a group/display surfaces in the builder as an invalid (error-styled, disabled) target with an inline hint, and a new save-time check blocks the save until it's retargeted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
questionnaireBasicSchema builds fresh Zod objects on every call, and all three consumers (clone dialog .pick, detail page as-is, create page .extend) called it inline in the component body — reallocating the whole schema chain every render. Each now wraps the call in useMemo(..., [t]), and the schema module's doc states the pattern for future consumers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The export-serialization doc block sat above moveElement, leaving downloadQuestionnaireJson undocumented and moveElement double-documented. Pure reorder: moveElement (with its own doc) now precedes the download doc + function. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The editor card's header strip (ordinal + title text + "Question Type" badge) duplicated the Title/Type fields directly below it. The fields now lead instead: the rich type picker sits first with the kebab menu aligned top-right, followed by a roomy borderless title input (ordinal kept as a small prefix, focus underline, visually-hidden label so the "Question Title" textbox name is unchanged for a11y and specs). Every field below is untouched. Intentional divergence from the Figma header strip, per owner direction. The now-unlabeled type picker carries an explicit aria-label. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 22687d8f9cfa
Each nav row now shows the question's small tinted type icon between the ordinal and the title, in both the builder nav and the renderer preview nav (same shared component). The type→icon+tint map moves from builder/QuestionTypePicker.tsx to shared/questionTypeIcons.ts because shared/QuestionTreeNav renders it and shared must not import from builder/; the picker now consumes the shared map. Icons are decorative (aria-hidden), so row accessible names are unchanged. The mobile Select fallback stays text-only. README pointer updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 139 out of 140 changed files in this pull request and generated no new comments.
Suppressed comments (4)
tests/facility/patient/encounter/forms/enableWhen/string.spec.ts:27
test.describe.configure({ mode: "default" })does not opt this file out offullyParallel: true, so these stateful tests can still run in parallel and leak shared encounter responses. Usemode: "serial"to guarantee sequential execution.
tests/support/questionnaireId.ts:12- The docstring says the setup auto-runs when the meta file is "missing or invalid", but the implementation only auto-runs when the file is missing (invalid JSON throws). Either implement the retry-on-invalid behavior or update the comment to match actual behavior.
tests/facility/patient/encounter/forms/enableWhen/boolean.spec.ts:23 test.describe.configure({ mode: "default" })does not opt this file out offullyParallel: true, so these stateful tests can still run in parallel and leak shared encounter responses. Usemode: "serial"(as used elsewhere in the test suite) to guarantee sequential execution.
src/Routers/routes/adminRoutes.tsx:30- The PR description currently contains placeholder text (e.g. "Fixes #issue_number", "Change 1/2"), but this diff introduces substantial questionnaire/v2 routing & management changes. Please update the PR description to accurately summarize the changes and link the real issue/requirement for traceability.
"/admin/questionnaires": () => (
<QuestionnaireListPage scope={INSTANCE_SCOPE} />
),
There was a problem hiding this comment.
Review: Questionnaire v2 Builder & Renderer
140 files, 13k+ additions — this is a big chunk of work. The architecture is reasonably thought-out: the reducer-based builder state, Jotai store for renderer, and the shared questionTree.ts utilities are solid foundations. The detailed inline comments throughout (especially the normalizeBooleanConditionAnswer explanation and the useState vs useMemo justification in RendererContext) are genuinely good — the reasoning is written down where it belongs.
Issues (inline comments above):
- Firefox download bug (QuestionnaireDetailPage.tsx:81) — anchor click without DOM attachment. Breaks silently in Firefox. Fix before shipping.
- Missing memoization (VisibilityConditionsCard.tsx:96) —
flattenQuestionsrebuilt every render; extract to shared util and memoize. errorsAtomhas no writer (store.ts:23) — intentional per the comment, but QA needs to be explicitly told validation errors won't appear in preview/fill until the submit path lands.migrateLegacyBooleanEnableWhenalways allocates (builderReducer.ts:72) — new array references on every reset meanspreviewQuestionnaireuseMemo always invalidates, wiping preview state on background refetches.
The Firefox download bug is the only hard blocker. The rest are code quality issues that should be addressed but won't necessarily break users (on Chrome, anyway).
Generated by Grumpy PR Reviewer for issue #16618 · 69.1 AIC · ⌖ 8.77 AIC · ⊞ 6.3K
| const linkElement = document.createElement("a"); | ||
| linkElement.setAttribute("href", dataUri); | ||
| linkElement.setAttribute("download", `${questionnaire.slug}.json`); | ||
| linkElement.click(); |
There was a problem hiding this comment.
This silently fails in Firefox. An anchor element not attached to the DOM can't be programmatically clicked in Firefox — you'll get zero download, zero error, just confusion.
document.body.appendChild(linkElement);
linkElement.click();
document.body.removeChild(linkElement);Alternatively use URL.createObjectURL with a Blob. Either way, test in Firefox before shipping this.
| const enableWhen = question.enable_when ?? []; | ||
| const enableBehavior = question.enable_behavior ?? "all"; | ||
| const excludedIds = new Set(collectIds(question)); | ||
| const flatQuestions = flattenQuestions(allQuestions); |
There was a problem hiding this comment.
flattenQuestions(allQuestions) runs on every render. For a questionnaire with 50 questions this is cheap, but there's already a findFirstQuestion tree-walker in shared/questionTree.ts. This is also a good candidate to extract there and wrap in useMemo here. As written, the whole flat array is rebuilt every time the parent re-renders.
|
|
||
| export const questionnaireAtom = atom<QuestionnaireRead | null>(null); | ||
| export const responsesAtom = atom<Record<string, QuestionnaireResponse>>({}); | ||
| export const errorsAtom = atom<QuestionValidationError[]>([]); |
There was a problem hiding this comment.
errorsAtom has readers (QuestionField, StructuredQuestionSlot) but no writer in this PR. The store comment explains this is intentional — fill/submit is coming later. That's fine architecture-wise, but those readers will see an empty array forever and silently never show validation errors. Make sure the incomplete submit path is understood by QA so they don't mark "validation working" when it isn't yet.
| * boolean answers at evaluation time so already-saved questionnaires work | ||
| * without a re-save. | ||
| */ | ||
| export function migrateLegacyBooleanEnableWhen( |
There was a problem hiding this comment.
migrateLegacyBooleanEnableWhen calls mapTree which unconditionally builds new arrays via .map() — so reset always hands a fresh array reference to the state even when zero questions needed migration. Not a correctness bug, but it means previewQuestionnaire will be rebuilt by useMemo on every background refetch, wiping in-progress preview answers. The useMemo deps include state.questions, which will be a new reference after every reset. Worth an identity-preserving short-circuit: if no questions matched needsMigration, return the original array.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Proposed Changes
Fixes #issue_number
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit