Fix docs search regression by testing against preview build and adding responsive search-modal coverage#43338
Conversation
…on test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a docs search regression in automated checks by aligning both the docs Playwright harness and the multi-device docs workflow to run against an Astro production preview (build + preview) instead of dev mode, and adds responsive E2E coverage to ensure the Search dialog remains usable on larger viewports.
Changes:
- Switch
docsPlaywrightwebServertonpm run build && npm run previewto exercise production-only search behavior. - Add a new Playwright spec that validates the search modal’s open/close/reopen behavior and that the homepage CTA remains clickable after dismissing the modal (desktop + tablet).
- Update the
daily-multi-device-docs-testerworkflow to build docs and startastro preview, plus allow the needednpm run build*/npm run preview*commands.
Show a summary per file
| File | Description |
|---|---|
| docs/tests/search-dialog.spec.ts | Adds desktop/tablet E2E regression coverage for the Search dialog usability and dismissal behavior. |
| docs/playwright.config.ts | Runs docs tests against a built preview server so search matches production behavior. |
| .github/workflows/daily-multi-device-docs-tester.md | Updates the workflow to build docs and run preview, and expands allowed commands accordingly. |
| .github/workflows/daily-multi-device-docs-tester.lock.yml | Regenerates the compiled workflow lock output to reflect the markdown workflow changes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Low
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (2 tests)
🔍 Test Detail
Covers the full open → inspect → Escape-dismiss cycle, then a reopen cycle, then link navigation after dialog interactions. The reopen pass is meaningful regression coverage (this PR title says it is fixing a dismiss-and-reopen bug). Selectors are role-based ( Minor improvement opportunities (informational, not violations):
Verdict
The References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with targeted improvements; no blocking issues found.
📋 Key Themes & Highlights
Key Themes
- Flakiness risk re-introduced:
waitForLoadState('networkidle')in the new spec file is exactly the kind of environment-dependent wait that caused the original false-critical failure. Removing it keeps the tests fast and stable. - Bundled assertions: The single parametrised test packs four distinct behaviours — opening, dismissing, reopening, and CTA navigation — into one body. Splitting them yields clearer failure signal and better spec readability.
- Incomplete responsive coverage: The PR title promises responsive coverage but omits a mobile viewport, where Starlight's search trigger may differ.
webServertimeout not updated:playwright.config.tsstill uses 180 s for a startup command that now includes a full production build.
Positive Highlights
- ✅ Root-cause fix is well-reasoned: switching from
npm run devtonpm run build && npm run previewis the correct and minimal change to align test and production environments. - ✅ Both the E2E config and the agentic workflow are updated consistently — no half-measures.
- ✅ The new spec uses Playwright's recommended role-based selectors throughout.
- ✅ Lock file updated correctly — no manual edits needed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 43.1 AIC · ⌖ 8.11 AIC · ⊞ 6.6K
Comment /matt to run again
| await page.setViewportSize({ width: viewport.width, height: viewport.height }); | ||
| await page.goto('/gh-aw/'); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
There was a problem hiding this comment.
[/diagnosing-bugs] waitForLoadState('networkidle') is prone to flakiness and could reintroduce intermittent failures — exactly the problem this PR fixes.
💡 Suggestion
Drop the explicit networkidle wait and rely on Playwright's auto-waiting:
await page.goto('/gh-aw/');
// Auto-wait: Playwright retries until the element is visible
await expect(page.getByRole('button', { name: 'Search' })).toBeVisible();The workflow guidance already recommends domcontentloaded over heavier load states for the same reason (avoiding timeout-induced false failures).
@copilot please address this.
| @@ -0,0 +1,35 @@ | |||
| import { test, expect } from '@playwright/test'; | |||
|
|
|||
| const VIEWPORTS = [ | |||
There was a problem hiding this comment.
[/tdd] The PR title mentions "responsive" coverage, but only desktop (1366px) and tablet (834px) are tested — no mobile viewport. The original false-critical failure was a desktop issue; a mobile breakpoint would round out the regression suite.
💡 Suggested addition
const VIEWPORTS = [
{ name: 'desktop', width: 1366, height: 768 },
{ name: 'tablet', width: 834, height: 1194 },
{ name: 'mobile', width: 390, height: 844 },
];On narrow viewports, Starlight typically hides or replaces the desktop search button with a mobile-specific trigger — worth asserting the modal is still reachable.
@copilot please address this.
| ]; | ||
|
|
||
| for (const viewport of VIEWPORTS) { | ||
| test(`search dialog is usable and dismissible on ${viewport.name}`, async ({ page }) => { |
There was a problem hiding this comment.
[/tdd] The for loop generates a single test case that packs open → dismiss → reopen → CTA navigation into one scenario. If the reopen assertion fails, the CTA check is silently skipped. Splitting these into separate test blocks would give clearer signal on which behaviour regressed.
💡 Suggested structure
test(`search dialog opens with visible input on ${viewport.name}`, async ({ page }) => { /* open only */ });
test(`search dialog dismisses on Escape on ${viewport.name}`, async ({ page }) => { /* open + Escape */ });
test(`search dialog can be reopened on ${viewport.name}`, async ({ page }) => { /* open, close, reopen */ });
test(`CTA is actionable after closing dialog on ${viewport.name}`, async ({ page }) => { /* open, close, click CTA */ });Each test name reads as a spec statement and the failure message points directly to the broken behaviour.
@copilot please address this.
| // Keep startup lighter than a full prebuild, but ensure the homepage slide | ||
| // preview asset exists before the dev server starts. | ||
| command: 'npm run build:slides && npm run dev --silent -- --host 127.0.0.1 --port 4321', | ||
| // Search is only available in production builds, so run tests against a |
There was a problem hiding this comment.
[/diagnosing-bugs] The timeout for webServer is still 180 * 1000 ms but the startup command now includes a full npm run build before preview. If the build takes longer than ~3 min on a cold CI runner, Playwright will kill the server before tests start and produce confusing timeout errors.
💡 Suggestion
Consider raising the timeout to accommodate the build step, or separating it:
timeout: 300 * 1000, // 5 min: build (≈2 min) + preview startupAlternatively, document the expected build time in a comment so future editors know the limit isn't arbitrary.
@copilot please address this.
There was a problem hiding this comment.
Review: Fix docs search regression — preview build + search-modal coverage
Overall: The root-cause fix (switching from dev to npm run build && preview) is correct and well-motivated. The new spec validates real production search behavior. Three non-blocking issues worth addressing before merge.
Change summary
| File | Change |
|---|---|
daily-multi-device-docs-tester.md + .lock.yml |
Switch workflow pre-agent server startup to npm run build && preview, update allowed bash commands |
docs/playwright.config.ts |
Switch webServer command to production build; keeps reuseExistingServer for local dev |
docs/tests/search-dialog.spec.ts |
New spec: search open → visible input → Escape dismiss → reopen → CTA nav |
Issues found (non-blocking, suggestions):
-
waitForLoadState('networkidle')— flakiness risk (line 14, spec file): The workflow guidance updated in this same PR explicitly recommendsdomcontentloadedovernetworkidleto avoid CI timeouts. The new spec contradicts that guidance. -
Missing mobile viewport (line 6, spec file): Bug title mentions multi-device; only desktop + tablet are tested. A breakpoint regression on mobile would go undetected.
-
webServer.timeoutunchanged at 180 s (playwright.config.ts): The startup command is now a full production build (npm run buildtriggers codegen + asset pipeline), which is materially heavier than the previousbuild:slides && dev. 180 s may be too tight in CI.
References: §28699957975
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 47.7 AIC · ⌖ 6.16 AIC · ⊞ 4.9K
| await page.goto('/gh-aw/'); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| await page.getByRole('button', { name: 'Search' }).click(); |
There was a problem hiding this comment.
Using waitForLoadState('networkidle') can be flaky in CI — the workflow guidance in this same PR recommends domcontentloaded for consistent checks.
Consider replacing with:
await page.goto('/gh-aw/', { waitUntil: 'domcontentloaded' });
// remove the separate waitForLoadState callnetworkidle waits for 500 ms of zero network activity, which can time out if Astro's prefetch or service-worker traffic is ongoing.
@copilot please address this.
| const VIEWPORTS = [ | ||
| { name: 'desktop', width: 1366, height: 768 }, | ||
| { name: 'tablet', width: 834, height: 1194 }, | ||
| ]; |
There was a problem hiding this comment.
The spec loops over only desktop (1366×768) and tablet (834×1194) viewports, but the original bug report was a multi-device regression. A mobile viewport (e.g., 390×844 — iPhone 14) would complete the coverage story.
const VIEWPORTS = [
{ name: 'desktop', width: 1366, height: 768 },
{ name: 'tablet', width: 834, height: 1194 },
{ name: 'mobile', width: 390, height: 844 },
];Without a mobile entry, a breakpoint-specific regression on small screens would go undetected by this suite.
@copilot please address this.
| command: 'npm run build && npm run preview -- --host 127.0.0.1 --port 4321', | ||
| url: 'http://localhost:4321/gh-aw/', | ||
| reuseExistingServer: !process.env.CI, | ||
| timeout: 180 * 1000, |
There was a problem hiding this comment.
The webServer.timeout is still 180 s, but the startup command changed from npm run build:slides && npm run dev to a full npm run build (which runs codegen + asset compilation) followed by npm run preview. A full Astro build is considerably heavier and may exceed 180 s in CI, causing spurious test failures.
Consider increasing the timeout or adding a step in CI to cache the build output:
timeout: 300 * 1000, // full production build can take 3–5 min in CI@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — two blocking issues and two medium issues found
The direction is right — switching to a preview build does fix the root cause. But the implementation has issues that will cause the new tests to fail or misfire before they ever guard against real regressions.
Blocking issues (must fix before merge)
1. waitForLoadState('networkidle') in search-dialog.spec.ts:12
A full Astro production build leaves open network requests (deferred hydration, analytics) well past Playwright's 500 ms idle window. The workflow's own guidance already says to use domcontentloaded. Replace with waitForLoadState('domcontentloaded').
2. Missing mobile viewport in search-dialog.spec.ts:3
The PR title claims "responsive search-modal coverage" but the VIEWPORTS array covers only desktop and tablet. Mobile (≤640 px) is the dimension where layout regressions are most likely on a Starlight site. Add at least one mobile viewport and verify the search button is reachable at that breakpoint.
Medium issues (strong recommendation to fix)
3. webServer timeout: 180 * 1000 in playwright.config.ts:50
The new build command runs the full prebuild chain (generate-agent-factory, generate-model-tables, build:slides, then full Astro SSG). On a cold runner this exceeds 3 minutes. The timeout is exactly 3 minutes and will reliably fire before the preview server is ready. Increase to at least 360 s.
4. Ambiguous getByRole('button', { name: 'Search' }) selector
Without exact: true this can match multiple elements on a Starlight page (top-bar button + ⌘K shortcut hint), causing a strict-mode violation that surfaces as a confusing test error unrelated to search behavior.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 107.3 AIC · ⌖ 7.1 AIC · ⊞ 5.4K
Comment /review to run again
| test(`search dialog is usable and dismissible on ${viewport.name}`, async ({ page }) => { | ||
| await page.setViewportSize({ width: viewport.width, height: viewport.height }); | ||
| await page.goto('/gh-aw/'); | ||
| await page.waitForLoadState('networkidle'); |
There was a problem hiding this comment.
Flakiness risk: waitForLoadState('networkidle') will cause sporadic CI timeouts against a production preview build.
💡 Explanation and fix
A full Astro production build registers deferred hydration callbacks, analytics pings, and background fetches that keep open network requests well past Playwright's 500 ms idle window. On a cold CI runner this reliably causes networkidle to time out. The workflow's own agent guidance already tells agents to use waitUntil: 'domcontentloaded' for exactly this reason.
Replace:
await page.waitForLoadState('networkidle');With:
await page.waitForLoadState('domcontentloaded');For a static Astro site, domcontentloaded is sufficient — all critical HTML is rendered synchronously.
| @@ -0,0 +1,35 @@ | |||
| import { test, expect } from '@playwright/test'; | |||
|
|
|||
| const VIEWPORTS = [ | |||
There was a problem hiding this comment.
Missing mobile viewport coverage directly contradicts the PR's claim of 'responsive search-modal coverage'.
💡 Explanation and fix
VIEWPORTS covers only desktop (1366×768) and tablet (834×1194). On Starlight-based sites, mobile viewports (≤640 px) often hide the top-bar search button and expose it via a different control (e.g., inside a hamburger menu or a bottom-sheet overlay). Any mobile-specific regression in the search UI goes completely undetected by this test suite.
Add at least one mobile breakpoint:
const VIEWPORTS = [
{ name: 'mobile', width: 390, height: 844 },
{ name: 'tablet', width: 834, height: 1194 },
{ name: 'desktop', width: 1366, height: 768 },
];On mobile, the search button may need a different locator (e.g., a menu must be opened first). Test that path explicitly rather than assuming desktop and tablet selectors will work.
| command: 'npm run build:slides && npm run dev --silent -- --host 127.0.0.1 --port 4321', | ||
| // Search is only available in production builds, so run tests against a | ||
| // built preview server. | ||
| command: 'npm run build && npm run preview -- --host 127.0.0.1 --port 4321', |
There was a problem hiding this comment.
webServer timeout of 180 s will be breached by a full production build — tests will never start.
💡 Explanation and fix
The old command was npm run build:slides && npm run dev — a fast partial build. The new command is npm run build && npm run preview. Looking at package.json, npm run build triggers its prebuild hook, which runs generate-agent-factory, generate-model-tables, and build:slides before the full Astro SSG pass. On a cold CI runner without Node module cache, this pipeline routinely exceeds 3 minutes. The current timeout: 180 * 1000 (exactly 3 minutes) will fire before the build finishes, terminating the test run with a misleading Playwright startup error.
Increase the timeout:
timeout: 360 * 1000, // 6 min — covers full Astro prebuild + preview startupIf build time is a concern, a PLAYWRIGHT_REUSE_SERVER=1 convention (or a custom env var checked via reuseExistingServer) lets local developers reuse an already-built site across test runs.
| await page.goto('/gh-aw/'); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| await page.getByRole('button', { name: 'Search' }).click(); |
There was a problem hiding this comment.
getByRole('button', { name: 'Search' }) may throw a strict-mode violation if multiple elements match.
💡 Explanation and fix
Starlight search implementations commonly render multiple elements whose accessible name includes the word "Search" — for example, the visible icon button in the top bar, a keyboard-shortcut hint button (⌘K), and potentially an aria-label on the search container itself. Playwright's getByRole in strict mode (the default) throws if more than one element matches, so on real Starlight pages this selector is fragile.
Use exact: true to match only the primary button and reduce the chance of multiple matches:
await page.getByRole('button', { name: 'Search', exact: true }).click();Or, if there are still multiple matches, scope the query to the header:
await page.locator('header').getByRole('button', { name: 'Search', exact: true }).click();|
🎉 This pull request is included in a new release. Release: |
The multi-device docs report identified a false-critical desktop failure where the Search dialog opened without a usable input and blocked primary homepage actions. Root cause was environment mismatch: tests/workflow exercised Astro dev mode, where search is intentionally unavailable, instead of production-like preview behavior.
Workflow runtime alignment (docs tester)
daily-multi-device-docs-testerto build docs and runastro previewbefore device checks.npm run build*andnpm run preview*.Docs E2E harness alignment
webServerfrom dev server tonpm run build && npm run previewso search behavior matches deployed docs.Responsive regression coverage for search interaction
docs/tests/search-dialog.spec.tswith desktop + tablet cases that assert: