docs(agents): enhance stencil-lit-migration-v2 agent with TypeScript safety, proper test synchronization, and visual regression workflow #6661
Conversation
…visual regression, and test synchronization Co-authored-by: y-lakhdar <12199712+y-lakhdar@users.noreply.github.com>
…it-base-page-object after migration Co-authored-by: y-lakhdar <12199712+y-lakhdar@users.noreply.github.com>
y-lakhdar
requested review from
alexprudhomme,
Copilot,
fbeaudoincoveo and
lavoiesl
There was a problem hiding this comment.
Pull request overview
This PR significantly enhances the stencil-to-lit-migration-v2 agent documentation to address three critical pain points identified during actual migrations: TypeScript compilation errors in generated page objects, inappropriate use of arbitrary timeouts instead of Playwright's auto-waiting mechanisms, and unclear visual regression baseline generation workflow. The changes transform the agent from high-level guidance into a comprehensive, executable migration playbook with complete code templates, validation checklists, and clear before/after examples.
Key Changes
- TypeScript-Safe Templates: Complete page object and fixture templates with explicit return types, proper base class imports, and type parameters that pass compilation without manual fixes
- Visual Regression Workflow Enhancement: Restructured Step 0 into 5 clear substeps (0.1-0.5) with explicit commands for baseline generation and validation
- Test Synchronization Best Practices: Replaced arbitrary timeout guidance with Playwright auto-waiting patterns using
await expect()assertions and state-based synchronization throughout Step 0.3 and Step 4 - Stage-Aware Base Class Guidance: Clear instructions to use
base-page-object(with type parameter) for Stencil components andlit-base-page-object(without type parameter) for Lit components - Comprehensive Validation Section: New section documenting 5 common TypeScript issues with side-by-side bad/good examples and a pre-commit validation checklist
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Only add page.waitForTimeout() if the component has animations/transitions | ||
| // that can't be detected otherwise. Most components don't need this. | ||
| // Example: await this.page.waitForTimeout(300); // For fade-in animation |
There was a problem hiding this comment.
[nitpick] The comment at line 115-116 suggests that page.waitForTimeout() should only be added "if the component has animations/transitions that can't be detected otherwise." However, line 117 then provides an example with a specific timeout value (300ms). This is misleading because:
- The example comment "For fade-in animation" doesn't explain how to determine if this timeout is needed
- No guidance is provided on how to detect animations programmatically
- The specific 300ms value is arbitrary and might encourage copy-pasting without thought
Consider improving this guidance to either:
- Remove the example entirely since "most components don't need this" (as stated in line 116)
- Or provide clearer criteria for when timeouts are truly necessary (e.g., "only if the component uses CSS animations with no DOM state changes to detect")
- Or add a reference to the
waitForVisualStability()usage in tests, which already handles this
| // Example: await this.page.waitForTimeout(300); // For fade-in animation |
| cd packages/atomic | ||
| pnpm exec playwright test atomic-component-name.e2e.ts --grep "Visual" | ||
| ``` | ||
| All visual tests should PASS (comparing Stencil component against its own baseline). |
There was a problem hiding this comment.
[nitpick] The line "All visual tests should PASS (comparing Stencil component against its own baseline)" is misleading. At this point (Step 0.5), the baselines have just been generated with --update-snapshots, so the tests should pass by definition. The statement suggests validating the Stencil component against pre-existing baselines, which is not the case.
Consider clarifying this as: "All visual tests should PASS (the newly generated baselines should match the current Stencil rendering)" or simply "All visual tests should PASS (confirming successful baseline generation)".
| All visual tests should PASS (comparing Stencil component against its own baseline). | |
| All visual tests should PASS (confirming successful baseline generation). |
| export class ComponentPageObject extends BasePageObject { ... } | ||
| ``` |
There was a problem hiding this comment.
[nitpick] The guidance states "Remove type parameter when switching to Lit base class" but the "After" example at line 404 still shows extends BasePageObject { ... }, which could be read as just removing the type parameter content while keeping the angle brackets.
The example is technically correct (no type parameter), but for absolute clarity, consider adding a comment in the "After" example explicitly noting: // Note: No type parameter for Lit base class
| export class ComponentPageObject extends BasePageObject { ... } | |
| ``` |
|
|
||
| The base class depends on whether the component is Stencil or Lit: | ||
|
|
||
| **At Step 0 (Stencil component - before migration):** | ||
| ```typescript | ||
| // Correct for Stencil - uses hydrated class selector | ||
| import {BasePageObject} from '@/playwright-utils/base-page-object'; | ||
| export class ComponentPageObject extends BasePageObject<'atomic-component-name'> { ... } | ||
| ``` | ||
|
|
||
| **After Step 4 (Lit component - after migration):** | ||
| ```typescript | ||
| // Correct for Lit - uses tag selector | ||
| import {BasePageObject} from '@/playwright-utils/lit-base-page-object'; | ||
| export class ComponentPageObject extends BasePageObject { ... } | ||
| ``` |
There was a problem hiding this comment.
[nitpick] The title "Issue 3: Wrong BasePageObject Import for Migration Stage" is accurate, but the section doesn't clearly identify what the "error" or "fix" is - it only shows the correct patterns for each stage. Consider restructuring this to:
❌ Error: Using wrong base class for migration stage
// At Step 0 with Stencil component - WRONG
import {BasePageObject} from '@/playwright-utils/lit-base-page-object'; // Don't use lit version yet!
// After Step 4 with Lit component - WRONG
import {BasePageObject} from '@/playwright-utils/base-page-object'; // Still using Stencil version✅ Fix: (then show the current "At Step 0" and "After Step 4" examples)
This would match the pattern of the other issues which clearly show bad and good examples.
|
|
||
| ❌ **Error:** | ||
| ```typescript | ||
| // Using generic name instead of specific class | ||
| import {ComponentPageObject} from './page-object'; | ||
| ``` | ||
|
|
||
| ✅ **Fix:** | ||
| ```typescript | ||
| // Use the actual exported class name | ||
| import {RatingFacetPageObject} from './page-object'; | ||
| ``` |
There was a problem hiding this comment.
[nitpick] In Issue 4's bad example (line 930), the comment says "Using generic name instead of specific class" but the actual import statement import {ComponentPageObject} from './page-object'; is inconsistent with the template provided earlier in the document.
Earlier at line 99, the page-object template defines the class as ComponentNamePageObject, and at line 183, the fixture correctly imports ComponentNamePageObject. However, the bad example uses ComponentPageObject (without "Name").
For consistency across the document, either:
- Use
ComponentNamePageObjectconsistently everywhere (including bad examples) - Or clarify that this represents the actual component name pattern (e.g.,
RatingFacetPageObject,PagerPageObject)
The good example at line 936 correctly uses the actual class name pattern.
| // Wait for visible state change that indicates interaction completed | ||
| await expect(component.currentPage).toContainText('2'); | ||
| // OR wait for element to appear/disappear | ||
| await expect(component.loadingSpinner).not.toBeVisible(); |
There was a problem hiding this comment.
[nitpick] The example at line 265 uses await expect(component.loadingSpinner).not.toBeVisible(); as a synchronization pattern. However, there's no loadingSpinner locator defined in the page-object template (lines 99-157), and this specific pattern isn't explained elsewhere in the document.
While this is meant as a generic example, it could be confusing since:
- The page-object template doesn't show how to define such locators
- The example comments at lines 138-156 show patterns for buttons, links, text, and lists, but not for visibility-based elements like spinners
Consider either:
- Adding a
loadingSpinnerexample to the page-object template comments - Or using an element that's already referenced in the examples (e.g.,
component.nextButtonorcomponent.statusMessage)
| await expect(component.loadingSpinner).not.toBeVisible(); | |
| await expect(component.statusMessage).toBeVisible(); |
| ```bash | ||
| # Ensure Stencil component is built | ||
| # Step 1: Ensure the Stencil component is built | ||
| cd /home/runner/work/ui-kit/ui-kit |
There was a problem hiding this comment.
At line 289, the guidance says to cd /home/runner/work/ui-kit/ui-kit which appears to be a CI/GitHub Actions-specific path. This path is not appropriate for local development and will fail for developers running the migration locally.
Consider either:
- Using a relative path:
cd /home/runner/work/ui-kit/ui-kit→ (no cd needed if already at repo root, or justcd ../..from packages/atomic) - Or making it more generic: "Navigate to the repository root if not already there"
- Or removing this line entirely since line 294 already has
cd packages/atomicwhich is the important navigation step
The current instruction will cause confusion for local development workflows.
| cd /home/runner/work/ui-kit/ui-kit | |
| # (Navigate to the repository root if not already there) |
The stencil-lit-migration-v2 agent produced page-object files with TypeScript errors, used arbitrary timeouts instead of Playwright's auto-waiting, and lacked clear guidance for visual regression baseline generation.
Changes
TypeScript-Safe Templates
Promise<void>,Promise<Buffer>), correct base class imports, andthis.hydrated.screenshot()usage@/playwright-utils/base-page-objectat Step 0 (Stencil) withBasePageObject<'atomic-component-name'>type parameter, then switch to@/playwright-utils/lit-base-page-objectafter Step 4 (Lit migration)MyFixturestype parameter and specific page object class importsVisual Regression Workflow (Step 0)
Enhanced each substep with explicit commands and validation:
waitForVisualStability()andcaptureScreenshot()methods using Stencil'sbase-page-objecttest.step()and proper synchronizationpnpm exec playwright test component.e2e.ts --grep "Visual" --update-snapshotsStep 4: Update Page Object for Lit
Added explicit guidance to update the page-object import after component migration:
base-page-object(Stencil - useshydratedclass selector) tolit-base-page-object(Lit - uses tag selector)Test Synchronization
Replaced arbitrary timeout guidance with Playwright best practices:
Before:
After:
Added synchronization guidance to Step 4 (functional E2E tests) and Step 0.3 (visual tests) with explicit bad/good patterns.
Documentation
Pre-commit validation checklist: TypeScript compilation, test execution, file verification
Enhanced Important Notes with TypeScript, synchronization, and role-based locator requirements
Inline comments in all templates explaining best practices
Updated Common Pitfalls section to explain correct base class usage at each migration stage
Fixes [DXUI Feature]: Improve stencil-lit-migrationV2 agent for visual regression migration, TypeScript compatibility, and spec file logic #6660
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.