[squad] Implement #53519: Add docs integration, test coverage, and regression checks - #53750
Conversation
…aywright flow Closes #53519 - Link the AW wizard page from the docs sidebar (Setup > AW Wizard) - Extract wizard data model validation into validation.ts (no JSON import) so it can be unit tested directly with plain node - Add docs/src/lib/wizard/model.test.js covering valid data plus malformed cases: non-object input, missing/non-semver version, empty top-level arrays, and goal/trigger/destination options missing required fields - Wire the new test into make test-docs-wizard-model - Add a second Playwright flow in wizard.spec.ts covering the issue-automation goal branch to exercise a distinct trigger/destination inference path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Non-blocking review: this refactor cleanly extracts the wizard model validator and the added regression coverage hits the right failure modes.
Highlights
- The new pure
validation.tssplit removes the JSON import constraint from unit testing without changing runtime validation behavior. - The direct Node test covers malformed top-level structure and missing required fields, which is the gap this PR set out to close.
- The extra Playwright scenario exercises a second goal branch and checks the inferred destination text, which is the highest-value UI regression to add here.
I didn't find a changed-line correctness, security, or performance issue strong enough to block merge.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.92 AIC · ⌖ 6.75 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Ponytail review (over-engineering only): one finding — the test file reinvents assert helpers that Node ships built-in.
net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #53750 · auto · 22.4 AIC · ⌖ 7.49 AIC · ⊞ 7.3K
Comment /ponytail to run again
| { | ||
| assertDoesNotThrow(() => validateModel(clone(validModel)), 'checked-in wizard-data-model.json passes validation'); | ||
| } | ||
|
|
||
| // ------------------------------------------------------------------- | ||
| // Test: non-object input is rejected | ||
| // ------------------------------------------------------------------- | ||
| console.log('\nnon-object input:'); | ||
| { | ||
| assertThrows(() => validateModel(null), 'null is rejected'); | ||
| assertThrows(() => validateModel('not an object'), 'string is rejected'); | ||
| assertThrows(() => validateModel([]), 'array is rejected'); | ||
| } | ||
|
|
||
| // ------------------------------------------------------------------- | ||
| // Test: missing/invalid version field | ||
| // ------------------------------------------------------------------- | ||
| console.log('\nversion field:'); | ||
| { | ||
| const missingVersion = clone(validModel); | ||
| delete missingVersion.version; | ||
| assertThrows(() => validateModel(missingVersion), 'missing version is rejected'); | ||
|
|
||
| const nonSemverVersion = clone(validModel); | ||
| nonSemverVersion.version = '1.0'; |
There was a problem hiding this comment.
L55-79: stdlib: hand-rolled assertThrows/assertDoesNotThrow reimplement assert.throws/assert.doesNotThrow from Node's built-in assert module. Use require('node:assert') (strict variant) instead.
There was a problem hiding this comment.
Review: LGTM ✅
The PR cleanly delivers all three stated goals:
-
Docs sidebar wiring —
/wizard/is correctly placed under Setup inastro.config.mjs, consistent with the existing sidebar link convention. -
Validation extraction —
model.tsis refactored to delegate tovalidation.ts; the validation logic is unchanged and all public types/exports are re-exported, so downstream consumers see no breaking change. -
Test coverage —
model.test.jscovers 14 cases (valid model, null/string/array inputs, missing version, empty arrays, missing required fields) and all pass. The Playwright regression inwizard.spec.tsexercises the issue-automation goal → comment destination inference path end-to-end.
Minor observation (non-blocking): test-docs-wizard-model is not wired into any aggregate CI target, but this matches the pre-existing behaviour of test-docs-remark. No action required on this PR.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 49.9 AIC · ⌖ 8.83 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Pull request overview
Adds documentation navigation and regression coverage for the AW wizard.
Changes:
- Adds the wizard to the docs sidebar.
- Extracts model validation and adds malformed-data tests.
- Adds another Playwright wizard flow.
Show a summary per file
| File | Description |
|---|---|
Makefile |
Adds a wizard-model test target. |
docs/astro.config.mjs |
Adds the wizard sidebar link. |
docs/src/lib/wizard/model.ts |
Imports and re-exports extracted validation. |
docs/src/lib/wizard/validation.ts |
Defines model types and validation. |
docs/src/lib/wizard/model.test.js |
Tests malformed model data. |
docs/tests/wizard.spec.ts |
Tests the issue-automation flow. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 5
- Review effort level: Balanced
| if (!Array.isArray(goal.triggerOptionIds) || goal.triggerOptionIds.length === 0) { | ||
| throw new Error(`Goal category "${goal.id}" must define triggerOptionIds.`); | ||
| } | ||
| if (!Array.isArray(goal.destinationOptionIds) || goal.destinationOptionIds.length === 0) { | ||
| throw new Error(`Goal category "${goal.id}" must define destinationOptionIds.`); |
| await expect(page.getByLabel('When an issue is opened')).toBeVisible(); | ||
| await expect(page.getByLabel('When someone comments on an issue')).toBeVisible(); | ||
|
|
||
| await page.getByRole('button', { name: 'Next' }).click(); |
| @echo "✓ Docs remark plugin unit tests passed" | ||
|
|
||
| .PHONY: test-docs-wizard-model | ||
| test-docs-wizard-model: |
| await page.goto('/gh-aw/wizard/'); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| await expect(page.getByText('Step 1 of 3')).toBeVisible(); |
| * Run with: node docs/src/lib/wizard/model.test.js | ||
| */ | ||
|
|
||
| import { validateModel } from './validation.ts'; |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on a few targeted issues before merge.
📋 Key Themes & Highlights
Issues Found
- Implicit Node version dependency (
model.test.js:15): importingvalidation.tsdirectly requires Node ≥22 (strip-types), which isn't guarded or documented in the Makefile target. The existinginlineMarkdownInHtml.test.jsavoids this by importing only.js. - Fragile Playwright assertion (
wizard.spec.ts:44): the issue-automation test skips trigger selection, so the destination inference asserts whatever the wizard pre-selects by default. Explicitly checking a trigger makes the test a specification rather than a snapshot. - Misleading test label (
model.test.js:101): the mutation deletes the wholetextobject but the label saystext.label, making the coverage intent unclear. - Redundant import (
model.ts:3):WizardDataModelis imported but not used locally; all types reach consumers via the re-export block.
Positive Highlights
- ✅ Clean extraction of
validation.ts— pure function with no JSON import makes it testable outside Astro's module system. - ✅ Good use of assertion helpers (
assertThrows/assertDoesNotThrow) that mirror the spirit of the/tddred-green style. - ✅ Sidebar placement of the Wizard page under Setup is sensible and minimal.
- ✅ 14 targeted assertions cover the meaningful structural invariants of the model.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.9 AIC · ⌖ 10.3 AIC · ⊞ 7.8K
Comment /matt to run again
| await expect(page.getByRole('heading', { name: 'Review the inferred output destination' })).toBeVisible(); | ||
| await expect(page.locator('[data-wizard-summary]')).toContainText('Automate issue triage or replies'); | ||
| await expect(page.locator('[data-wizard-preview]')).toContainText('Destination: Post a comment'); | ||
| await expect(page.getByRole('button', { name: 'Review selections' })).toBeVisible(); |
There was a problem hiding this comment.
[/tdd] The test advances past step 2 without asserting which trigger option is selected, making the destination-inference assertion fragile.
💡 Suggestion
The destination "Post a comment" is inferred from the selected trigger type. If the wizard's default trigger changes, this test will silently start testing the wrong path. Explicitly select a trigger before clicking Next so the test reads as a specification:
await page.getByLabel('When an issue is opened').check();
await page.getByRole('button', { name: 'Next' }).click();This makes the intent clear and decouples the assertion from whatever the wizard happens to pre-select.
@copilot please address this.
| * Run with: node docs/src/lib/wizard/model.test.js | ||
| */ | ||
|
|
||
| import { validateModel } from './validation.ts'; |
There was a problem hiding this comment.
[/tdd] Importing validation.ts with a .ts extension relies on Node 22's --experimental-strip-types, which is absent from the existing test infrastructure and undocumented in the Makefile target.
💡 Suggestion
The existing inlineMarkdownInHtml.test.js imports only .js files. The new test imports a .ts file directly, which works on Node 22+ (strip-types is on by default) but will silently fail on older Node versions with a cryptic module-not-found error.
Add a minimum Node version guard in the Makefile target:
test-docs-wizard-model:
`@node` --version | grep -qE '^v(2[2-9]|[3-9][0-9])' || (echo "Node 22+ required for TypeScript strip-types"; exit 1)
`@node` docs/src/lib/wizard/model.test.jsAlternatively, document the Node >=22 requirement in the target comment so future CI upgrades know why.
@copilot please address this.
| assertThrows(() => validateModel(emptyDestinations), 'empty destinationOptions is rejected'); | ||
| } | ||
|
|
||
| // ------------------------------------------------------------------- |
There was a problem hiding this comment.
[/tdd] Test label says 'goal category missing text.label is rejected' but the mutation deletes the entire text object — a broader failure than the label implies.
💡 Suggestion
Rename the label to match the actual mutation, or tighten the test to delete only text.label to precisely exercise the hasText helper's typeof value.label === 'string' branch:
const missingGoalTextLabel = clone(validModel);
delete missingGoalTextLabel.goalCategories[0].text.label;
assertThrows(() => validateModel(missingGoalTextLabel), 'goal category missing text.label is rejected');@copilot please address this.
| } | ||
| } | ||
| import { validateModel, type WizardDataModel } from './validation'; | ||
|
|
There was a problem hiding this comment.
[/codebase-design] type WizardDataModel is imported on line 3 but never used locally — it is re-exported via the separate export type { ... } from './validation' block on lines 4–10.
💡 Suggestion
Remove the redundant named import to keep the module header minimal:
import { validateModel } from './validation';All types flow to consumers exclusively through the re-export block below.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
test body> Generated by PR Description Updater for #53750 · auto · 37.4 AIC · ⌖ 5.63 AIC · ⊞ 7.6K · ◷