Questionnaire v2: outline overlay and submit/draft regression coverage - #16630
Conversation
…regressions New fillOutlineNav.spec.ts covers the overlay's contract: collapsed by default with the canvas at full width, tick-per-question rail, open via click/hover/focus, Escape restoring focus to the rail, scroll-spy and click-pin active states, completion adornments, enable_when reveal, group children, and per-form sections in multi-questionnaire sessions. Regressions for the submit/draft fixes: a repeats answer submits its later rows after row 1 is cleared in place, Discard preserves answers typed while the restore prompt was pending (and persists them as a fresh draft), Resume merges a drafted form that was already re-added by hand, and a form_submission that is no longer status "draft" refuses to resume (created straight through the API, so the spec runs without the save-as-draft build flag). fillPatientSubject: the questionnaire title now keeps its unique part inside the 25-char slug window — the Date.now() suffix truncated to a constant that collided with a record in the DB snapshot on every run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 98ae52fe65b0
|
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 |
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c6a9d07ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ((await toggle.getAttribute("aria-expanded")) !== "true") { | ||
| await toggle.click(); |
There was a problem hiding this comment.
When Playwright performs toggle.click() with a mouse, it first moves the pointer onto the rail, whose pointerenter handler opens the panel; FillOutlineOverlay then deliberately ignores the ensuing mouse click when already open. Consequently, this helper and the test named “click opens the panel” still pass if the click/touch toggle branch is broken, duplicating the separate hover coverage instead of testing the promised click interaction. Use a touch pointer or another activation path that does not trigger mouse hover first.
Useful? React with 👍 / 👎.
| name: /Medications taken \(repeats\)/, | ||
| }), | ||
| ).toHaveAttribute("aria-current", "true"); | ||
| await expect(activeTick(page)).toHaveCount(1); |
There was a problem hiding this comment.
After navigating to “Medications taken,” this only verifies that some active tick exists. There was already exactly one active tick before the click, so the assertion passes if the rail remains stuck on the first question or activates the wrong tick, despite the step claiming to cover movement of the rail indicator. Assert data-active on the tick whose data-question-tick matches the selected question.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Test-only PR. The coverage rationale is sound — regression tests for the discard/resume/draft bugs, the outline overlay interaction model, and the repeats submission bug are all valuable. The fillPatientSubject.spec.ts de-flake is the right fix.
Three issues worth addressing:
- Hardcoded mouse coordinates (
page.mouse.move(900, 400)) in the hover test — fragile, use a locator instead. - Magic
+ 2in theenable_whentick count assertion — explain what the 2 represents. - No assertion that
submitwas found before inspectingsubmit?.body.resultsin the repeats test — a URL-pattern change would makesubmittedValuessilently empty and thetoContaincheck would give a false negative (well,not.toContainwould false-positive). Addexpect(submit).toBeDefined().
Otherwise: structure is clean, test descriptions are specific and actionable, the openOutline helper correctly guards against double-opens, and replacing Date.now() with faker.string.alphanumeric was the right call. Fine, I guess.
Generated by Grumpy PR Reviewer for issue #16630 · 24.5 AIC · ⌖ 6.15 AIC · ⊞ 6.3K
| // Leaving for the canvas closes it (200ms grace + transition). | ||
| await page.mouse.move(900, 400); | ||
| await expect(outlineToggle(page)).toHaveAttribute("aria-expanded", "false"); | ||
| }); |
There was a problem hiding this comment.
Hardcoded pixel coordinates. Classic.
await page.mouse.move(900, 400);This is a hardcoded viewport coordinate assuming the canvas is at x=900, y=400. Change the viewport size, run on a machine with a different DPR, or add any sidebar — and this test starts misfiring silently. Move the mouse to an actual locator instead:
await page.getByRole("region", { name: "Form canvas" }).hover();That way you're actually testing "user moves mouse to the canvas" rather than "user moves mouse to pixel (900,400) which we hope is the canvas today."
| await test.step("the dependent question gains a row and a tick", async () => { | ||
| await openOutline(page); | ||
| await expect( | ||
| nav.getByRole("button", { name: /Stability notes/ }), |
There was a problem hiding this comment.
Magic number arithmetic on a constant. DEFAULT_VISIBLE_TOP_LEVEL + 2 — why 2? What are those 2 questions? This is the kind of thing that silently breaks when the fixture changes and nobody knows why the number is 2 instead of 1 or 3. At minimum, add a comment explaining what the 2 represents (e.g., // the stability-notes question itself + one child it reveals). Better: name the constant.
| }; | ||
| const submit = body.requests.find((request) => | ||
| request.url.includes(`/questionnaire/${questionnaireId}/submit/`), | ||
| ); |
There was a problem hiding this comment.
Silent unsafe cast with no runtime validation. If postData() returns something that doesn't match this shape, you get undefined.find(...) and a cryptic error 10 frames deep. For test fixtures this is acceptable, but at minimum add a !submit guard before accessing submit?.body.results — you have ?. all over this chain already, so at least add an assertion:
expect(submit, "batch request must include a submit call").toBeDefined();Right now a regression in the API URL pattern would silently pass submittedValues as [], making the toContain assertion vacuously false and the test... still pass. Not great.
…rage for all types (#16631) Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: rithviknishad <mail@rithviknishad.dev>
There was a problem hiding this comment.
Overall: Mostly fine — the QuestionLabel cleanup is consistent and the new regression tests are well-commented and clearly motivated. Two gripes worth addressing:
-
SLUG_MAX_LENGTH25→50 is the wrong fix for a test collision. This is a production validation constant. Doubling it to avoid a test name clash is backwards — fix the test to generate properly unique slugs. If the backend actually accepts 50-char slugs, then fine, document that alignment explicitly. -
Label removal without a clear contract — pulling
QuestionLabelout of 10+ components in one shot is a big structural change. The assumption that the parent always renders the label needs to be either enforced (e.g. a wrapper component that is required) or at least documented, otherwise this will silently regress when someone uses these components in a new context.
The test prose and scenario coverage for the draft/resume/discard edge cases are actually quite good. Begrudgingly acknowledged.
Generated by Grumpy PR Reviewer for issue #16630 · 37.2 AIC · ⌖ 8.59 AIC · ⊞ 6.3K
| @@ -12,7 +12,7 @@ import { | |||
| * clone dialog's `-copy` suffix clamp, so a bound change lands everywhere. | |||
There was a problem hiding this comment.
Hold on a second. SLUG_MAX_LENGTH jumped from 25 to 50 to fix a test collision? That's treating the symptom, not the disease. If the backend enforces a 25-char slug limit (which it probably does, because backends tend to have opinions about database column widths), you've just let the frontend validate slugs the backend will reject. The fix for a test collision is to generate a better unique slug in the test — not to double the validation limit in production code. What's the actual backend constraint here?
| @@ -676,7 +674,6 @@ export function AllergyQuestion({ | |||
|
|
|||
There was a problem hiding this comment.
Removing QuestionLabel from all these structured question components simultaneously — with no fallback rendering — means the question text is now rendered nowhere inside these components. I assume the outline overlay or the parent wrapper now handles the label. But if someone uses one of these question components outside that context (e.g. standalone, embedded), the label silently disappears and the user stares at a form field with no idea what it's asking. A passing test suite won't catch that.
1 participant
Stack 5/5 — chain: #16618 ← #16627 ← #16628 ← #16629 ← this.
fillOutlineNav.spec.ts(15 tests) pins the overlay's contract: collapsed by default with the canvas at full width, tick-per-question rail, open via click/hover/focus, Escape restoring focus to the rail, scroll-spy and click-pin active states, completion adornments, enable_when reveal, group children, and per-form sections in multi-questionnaire sessions.form_submissionthat is no longer statusdraftrefuses to resume (created straight through the API, so the spec runs without the save-as-draft build flag).fillPatientSubject.spec.tsde-flaked: itsDate.now()title truncated to a constant inside the 25-char slug window and collided with a record baked into the DB snapshot on every run.Suite status on the stack tip: fill + outline + structured + resource-mount specs green (46 in scope); the two server-draft specs skip without the
REACT_ENABLE_QUESTIONNAIRE_DRAFTbuild flag, as designed.diagnosis.spec.tshas a pre-existing intermittent failure unrelated to this stack (a different test fails each run; tracked separately).🤖 Generated with Claude Code