From 0d36d2b85455fb6a16095bd4438c4ee986059bc1 Mon Sep 17 00:00:00 2001 From: bymyself Date: Tue, 3 Feb 2026 17:58:40 -0800 Subject: [PATCH] fix: address PR review comments - Remove duplicate 'After Making Changes' section in browser_tests/AGENTS.md - Remove unnecessary nextFrame() after loadWorkflow() in template - Update gotchas table to clarify nextFrame() not needed after loadWorkflow() - Update asset creation guidance for agents (manual JSON editing) - Remove '(non-obvious)' from Vue Node CSS classes note - Add local test command to CI debugging section Amp-Thread-ID: https://ampcode.com/threads/T-019c265d-ca00-7024-8d9b-b1eb804e5390 --- .claude/skills/writing-playwright-tests/SKILL.md | 8 ++++---- browser_tests/AGENTS.md | 6 ------ 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/.claude/skills/writing-playwright-tests/SKILL.md b/.claude/skills/writing-playwright-tests/SKILL.md index 30723a12d..2b0dfc056 100644 --- a/.claude/skills/writing-playwright-tests/SKILL.md +++ b/.claude/skills/writing-playwright-tests/SKILL.md @@ -14,7 +14,7 @@ description: 'Writes Playwright e2e tests for ComfyUI_frontend. Use when creatin 3. **Use premade JSON workflow assets** instead of building workflows programmatically. - Assets live in `browser_tests/assets/` - Load with `await comfyPage.workflow.loadWorkflow('feature/my_workflow')` - - Create new assets by exporting from ComfyUI UI + - Create new assets by starting with `browser_tests/assets/default.json` and manually editing the JSON to match your desired graph state ## Vue Nodes vs LiteGraph: Decision Guide @@ -32,7 +32,7 @@ await comfyPage.settings.setSetting('Comfy.VueNodes.Enabled', true) await comfyPage.vueNodes.waitForNodes() ``` -**Vue Node state uses CSS classes** (non-obvious): +**Vue Node state uses CSS classes:** ```typescript const BYPASS_CLASS = /before:bg-bypass\/60/ await expect(node).toHaveClass(BYPASS_CLASS) @@ -42,7 +42,7 @@ await expect(node).toHaveClass(BYPASS_CLASS) | Issue | Symptom | Fix | | --------------------------- | ------------------------------------ | -------------------------------------------- | -| **Missing nextFrame()** | Test passes locally, fails in CI | Add `await comfyPage.nextFrame()` after canvas ops | +| **Missing nextFrame()** | Test passes locally, fails in CI | Add `await comfyPage.nextFrame()` after canvas ops (not needed after `loadWorkflow()`) | | **Missing focus** | Keyboard shortcuts don't work | Add `await comfyPage.canvas.click()` first | | **Double-click timing** | Double-click doesn't trigger | Add `{ delay: 5 }` option | | **Drag animation** | Elements end up in wrong position | Use `{ steps: 10 }` not `{ steps: 1 }` | @@ -107,6 +107,7 @@ await expect(async () => { 2. Extract and view trace: `npx playwright show-trace trace.zip` 3. CI deploys HTML report to Cloudflare Pages (link in PR comment) 4. Reproduce CI: `CI=true pnpm test:browser` +5. Local runs: `pnpm test:browser:local` ## Anti-Patterns @@ -140,7 +141,6 @@ test.describe('FeatureName', { tag: ['@canvas'] }, () => { test('should do something', async ({ comfyPage }) => { await comfyPage.workflow.loadWorkflow('myWorkflow') - await comfyPage.nextFrame() const node = (await comfyPage.nodeOps.getNodeRefsByTitle('KSampler'))[0] // ... test logic diff --git a/browser_tests/AGENTS.md b/browser_tests/AGENTS.md index 178d71cb4..dd1f455d0 100644 --- a/browser_tests/AGENTS.md +++ b/browser_tests/AGENTS.md @@ -36,12 +36,6 @@ browser_tests/ - Run `pnpm exec eslint browser_tests/path/to/file.ts` to lint specific files - Run `pnpm exec oxlint browser_tests/path/to/file.ts` to check with oxlint -## After Making Changes - -- Run `pnpm typecheck:browser` after modifying TypeScript files in this directory -- Run `pnpm exec eslint browser_tests/path/to/file.ts` to lint specific files -- Run `pnpm exec oxlint browser_tests/path/to/file.ts` to check with oxlint - ## Skill Documentation A Playwright test-writing skill exists at `.claude/skills/writing-playwright-tests/SKILL.md`.