Questionnaire v2: one header per structured question + rendering coverage for all types - #16631
Conversation
The adapted widgets each rendered their own QuestionLabel above their
content — on the v2 fill page that duplicated the renderer's block
header ("1. Symptom *" followed by a second, differently-styled
"Symptom *"), and the label's ornament bar sat misaligned beside the
block's own marker. The legacy fill stack that needed the internal
label is deleted, and the v2 adapters are these components' only
consumers, so the widgets drop it: the renderer's numbered header is
the one header, and the history/template affordances keep their
right-aligned row. QuestionLabel itself is dead code now and goes with
it (allowlist updated).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: dfd0dc6f96dd
… type One parametrized spec over all 11 core structured types via their fixed questionnaires on the encounter mount: the widget mounts a real input (no degradation notice), exactly one question header renders (the duplicated-label regression), and the form canvas never scrolls horizontally. The per-type CRUD flows stay in the sibling specs; this pins the rendering/layout contract those never covered — six of the eleven types had no fill-page coverage at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: f0e4bdff8402
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
Deploying care-preview with
|
| Latest commit: |
839870b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c23a9cca.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://bodhi-qv2-structured-alignme.care-preview-a7w.pages.dev |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Grudgingly, this is a clean PR. Dead QuestionLabel deleted, 11 widgets trimmed of a duplicated header, README kept honest, and a parametric spec now covers all 11 types. The flex direction swap from justify-between to justify-end is the correct mechanical consequence of dropping the left-side label — no complaints there.
One inline gripe: the overflow helper's assertion message is attached to the wrong assertion (it says "must not scroll horizontally" but is actually guarding a null-canvas condition). That will produce confusing failure output. Fix the message, or restructure the two assertions so each one is honest about what it's checking.
Everything else: fine. Ship it after fixing the test wording.
Generated by Grumpy PR Reviewer for issue #16631 · 37.1 AIC · ⌖ 5.32 AIC · ⊞ 6.3K
Comments that could not be inline-anchored
tests/facility/patient/encounter/structuredQuestions/structuredRendering.spec.ts:430
The error message on .not.toBeNull() says "form canvas must not scroll horizontally" — but this assertion is actually checking that the canvas element was found at all. If the selector section[aria-label="Form canvas"] never matches, the failure message is going to be completely misleading to whoever debugs it at 2am. Separate it into two distinct expectations with distinct messages, or just throw a descriptive error when overflow === null.
There was a problem hiding this comment.
The core fix is right — removing the internal QuestionLabel from all 11 structured widgets and letting the renderer's single numbered header own the display. The justify-end alignment change is a logical consequence. The new parametric spec is a good idea for regression coverage.
Two concerns left inline:
-
Test correctness — the "exactly one question header" assertion may be a false positive. If the renderer doesn't emit a
<label>element,toHaveCount(1)on a zero-match locator will fail, but if the regex misses the actual text (e.g. "Symptom *" vs "Symptom"), the count is 0 and the test silently proves nothing. -
SLUG_MAX_LENGTH25→50 — unexplained change in a PR about visual alignment. Needs justification or its own PR.
Generated by Grumpy PR Reviewer for issue #16631 · 45.5 AIC · ⌖ 7.96 AIC · ⊞ 6.3K
| */ | ||
| export const SLUG_MIN_LENGTH = 5; | ||
| export const SLUG_MAX_LENGTH = 25; | ||
| export const SLUG_MAX_LENGTH = 50; |
There was a problem hiding this comment.
SLUG_MAX_LENGTH jumped from 25 to 50 with zero mention in the PR description. This is a stack fixing a visual label duplication bug — why is the slug validation limit changing here? If this is load-bearing for a fixed questionnaire slug, say so. Otherwise it's a drive-by change that belongs in its own PR with its own explanation.
|
|
||
| await test.step("layout holds", async () => { | ||
| await expectNoHorizontalOverflow(page); | ||
| }); |
There was a problem hiding this comment.
The regex ^${text}$ is anchored to the exact text content of the <label> element. But required questions render an asterisk — the label's full text is "Symptom *", not "Symptom". If ^Symptom$ matches nothing, toHaveCount(1) fails. If the renderer doesn't use a <label> at all and the count is already 0, this passes as a false positive — i.e., you deleted the old duplicate label and now you're asserting nothing duplicates it, which is vacuously true and tests nothing. Verify what element the renderer actually emits and match against that, or use a less brittle locator.
2 participants
Stack 6/6 — chain: #16618 ← #16627 ← #16628 ← #16629 ← #16630 ← this.
Alignment fix
On the v2 fill page every structured question rendered two headers: the renderer's block header ("1. Symptom *") and the widget's own internal
QuestionLabelright under it — differently styled, ~30px off the block's left edge, with the label's indigo ornament bar floating between them.The legacy fill stack that needed the internal label is deleted and the v2 adapters are these widgets' only consumers, so the widgets drop it: all 11
QuestionTypes/*components now render content only, the renderer's numbered header is the single header, and history/template affordances keep a right-aligned row.QuestionLabelitself became dead code and is deleted (v2 README allowlist updated).1. Symptom *⟶ ornament bar ⟶Symptom *(misaligned) ⟶ table1. Symptom *⟶ table (history button right-aligned)Structured rendering coverage for all 11 types
Existing structured specs cover CRUD for 5 types (symptom, diagnosis, allergy, medication request, charge item) — six types had no fill-page coverage at all. New
structuredRendering.spec.tsparametrizes over every core type via its fixed questionnaire (symptom, diagnosis, allergy_intolerance, medication_request, medication_statement, service_request, encounter, files, time_of_death, charge_item, appointment) and pins per type:Verification
diagnosis.spec.tsflake that fails on a different test each run, tracked separately).tsc, ESLint and Prettier clean.🤖 Generated with Claude Code