From 8c291b81cd7fb10479a55fb47e9a9cebcfc1b9b8 Mon Sep 17 00:00:00 2001
From: Craig Jennings <c@cjennings.net>
Date: Fri, 22 May 2026 15:48:48 -0500
Subject: refactor(skills): locator-first playwright guidance, drop emoji
 markers

Two cleanups to the playwright skills, landed together since they overlap the same files.

The skills taught networkidle as the readiness check and leaned on raw page.click/fill/waitForSelector. Playwright discourages networkidle for readiness, so the guidance in both SKILL.md files now waits for a visible app landmark via a web assertion or locator, the login and form examples use getByLabel/getByRole plus expect, the API reference leads with that pattern, and lib/helpers.js defaults waitForPageReady to load (preferring a caller-supplied landmark) and races the success indicator in authenticate instead of waiting on networkidle.

The second cleanup strips emoji console markers across run.js, helpers.js, both SKILL.md files, and the py examples, replacing each with a plain ASCII tag like [ok], [error], or [scan]. node --check and py_compile pass, and an emoji grep comes back clean.
---
 playwright-js/lib/helpers.js | 54 ++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 19 deletions(-)

(limited to 'playwright-js/lib')

diff --git a/playwright-js/lib/helpers.js b/playwright-js/lib/helpers.js
index 0920d68..42aeedd 100644
--- a/playwright-js/lib/helpers.js
+++ b/playwright-js/lib/helpers.js
@@ -82,30 +82,45 @@ async function createPage(context, options = {}) {
 }
 
 /**
- * Smart wait for page to be ready
+ * Smart wait for page to be ready.
+ *
+ * Readiness defaults to the 'load' event. Pass `options.waitForSelector` with an
+ * app-specific landmark (a heading, a role=main region, known text) to wait on
+ * something the app actually rendered — that's the most reliable readiness signal
+ * and the one to prefer. `networkidle` is intentionally NOT the default: Playwright
+ * discourages it for readiness, since a page can be interactive long before the
+ * network goes quiet (and may never go quiet with polling or analytics). Callers
+ * can still pass `waitUntil: 'networkidle'` explicitly if they really need it.
  * @param {Object} page - Playwright page
- * @param {Object} options - Wait options
+ * @param {Object} options - Wait options (waitUntil, timeout, waitForSelector)
  */
 async function waitForPageReady(page, options = {}) {
   const waitOptions = {
-    waitUntil: options.waitUntil || 'networkidle',
+    waitUntil: options.waitUntil || 'load',
     timeout: options.timeout || 30000
   };
-  
+
+  // Prefer waiting on a caller-supplied landmark selector when given; it's a
+  // far stronger readiness signal than any load-state event.
+  if (options.waitForSelector) {
+    try {
+      await page.waitForSelector(options.waitForSelector, {
+        state: 'visible',
+        timeout: waitOptions.timeout
+      });
+      return;
+    } catch (e) {
+      console.warn(`Landmark "${options.waitForSelector}" not visible in time, falling back to load state...`);
+    }
+  }
+
   try {
-    await page.waitForLoadState(waitOptions.waitUntil, { 
-      timeout: waitOptions.timeout 
+    await page.waitForLoadState(waitOptions.waitUntil, {
+      timeout: waitOptions.timeout
     });
   } catch (e) {
     console.warn('Page load timeout, continuing...');
   }
-  
-  // Additional wait for dynamic content if selector provided
-  if (options.waitForSelector) {
-    await page.waitForSelector(options.waitForSelector, { 
-      timeout: options.timeout 
-    });
-  }
 }
 
 /**
@@ -215,10 +230,11 @@ async function authenticate(page, credentials, selectors = {}) {
   await safeType(page, finalSelectors.password, credentials.password);
   await safeClick(page, finalSelectors.submit);
   
-  // Wait for navigation or success indicator
+  // Wait for a post-login landmark to become visible (preferred), or for a
+  // navigation to settle. networkidle is avoided here as a readiness signal.
   await Promise.race([
-    page.waitForNavigation({ waitUntil: 'networkidle' }),
-    page.waitForSelector(selectors.successIndicator || '.dashboard, .user-menu, .logout', { timeout: 10000 })
+    page.waitForSelector(selectors.successIndicator || '.dashboard, .user-menu, .logout', { state: 'visible', timeout: 10000 }),
+    page.waitForNavigation({ waitUntil: 'load' })
   ]).catch(() => {
     console.log('Login might have completed without navigation');
   });
@@ -383,7 +399,7 @@ async function detectDevServers(customPorts = []) {
 
   const detectedServers = [];
 
-  console.log('🔍 Checking for running dev servers...');
+  console.log('[scan] Checking for running dev servers...');
 
   for (const port of allPorts) {
     try {
@@ -397,7 +413,7 @@ async function detectDevServers(customPorts = []) {
         }, (res) => {
           if (res.statusCode < 500) {
             detectedServers.push(`http://localhost:${port}`);
-            console.log(`  ✅ Found server on port ${port}`);
+            console.log(`  [ok] Found server on port ${port}`);
           }
           resolve();
         });
@@ -416,7 +432,7 @@ async function detectDevServers(customPorts = []) {
   }
 
   if (detectedServers.length === 0) {
-    console.log('  ❌ No dev servers detected');
+    console.log('  [none] No dev servers detected');
   }
 
   return detectedServers;
-- 
cgit v1.2.3