Open
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR restructures Playwright testing documentation from a topic-based to hierarchical directory structure with eight top-level categories (core/, debugging/, testing-patterns/, advanced/, browser-apis/, architecture/, frameworks/, infrastructure-ci-cd/). It adds 40+ new comprehensive guides covering authentication flows, configuration, testing patterns (drag-drop, forms, GraphQL, visual regression), framework-specific guidance (Angular, Next.js, React, Vue), and CI/CD strategies. Internal cross-references are normalized throughout. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
debugging/debugging.md (1)
186-186:⚠️ Potential issue | 🟡 MinorInconsistent link path for assertions-waiting.md
Line 186 uses a flat reference
assertions-waiting.md#waiting-strategies, while other references to the same file (lines 441, 502) correctly use the relative path../core/assertions-waiting.md. This inconsistency will likely cause a broken link.🔗 Proposed fix to align with other references
-> **For comprehensive waiting patterns** (navigation, element state, network, polling), see [assertions-waiting.md](assertions-waiting.md#waiting-strategies). +> **For comprehensive waiting patterns** (navigation, element state, network, polling), see [assertions-waiting.md](../core/assertions-waiting.md#waiting-strategies).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@debugging/debugging.md` at line 186, Update the inconsistent link in debugging.md by replacing the flat reference "assertions-waiting.md#waiting-strategies" with the relative path used elsewhere "../core/assertions-waiting.md#waiting-strategies" so the link resolves correctly (search for the exact string "assertions-waiting.md#waiting-strategies" in the file and update it to "../core/assertions-waiting.md#waiting-strategies").
🟡 Minor comments (17)
testing-patterns/visual-regression.md-117-126 (1)
117-126:⚠️ Potential issue | 🟡 MinorReplace
waitForTimeoutwith deterministic waits.Using
waitForTimeout(500)is a Playwright anti-pattern that introduces non-deterministic behavior and potential flakiness. For JavaScript animations, wait for a specific condition instead of an arbitrary timeout.⏱️ Proposed fix using deterministic waits
test('page with JS animations', async ({ page }) => { await page.goto('/animated-hero'); - await page.getByTestId('hero-banner').waitFor({ state: 'visible' }); - await page.waitForTimeout(500); + // Wait for animation completion by checking a stable condition + const heroBanner = page.getByTestId('hero-banner'); + await heroBanner.waitFor({ state: 'visible' }); + + // Option 1: Wait for animation CSS class to be removed + await expect(heroBanner).not.toHaveClass(/animating/); + + // Option 2: Wait for a data attribute indicating completion + // await expect(heroBanner).toHaveAttribute('data-animation-complete', 'true'); await expect(page).toHaveScreenshot('hero.png', { animations: 'disabled',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing-patterns/visual-regression.md` around lines 117 - 126, Replace the non-deterministic page.waitForTimeout(500) in the test('page with JS animations') with a deterministic wait: remove the waitForTimeout call and wait for a specific condition that indicates the animation finished (for example use page.getByTestId('hero-banner').waitFor({ state: 'stable' }) if available, or await page.waitForFunction to assert computed style/transform/opacity has reached the final value, or wait for a CSS class/attribute that marks animation completion) before calling expect(page).toHaveScreenshot; keep the rest of the test (page.goto, locator waitFor visible, and toHaveScreenshot with animations:'disabled') intact.architecture/pom-vs-fixtures.md-32-32 (1)
32-32:⚠️ Potential issue | 🟡 MinorAdd language tags to fenced code blocks (MD040)
Line 32 and Line 260 use unlabeled fenced blocks. Add a language (e.g.,
text) to satisfy markdownlint and improve editor rendering.Suggested fix
-``` +```text What kind of reusable code? ... -``` +``` -``` +```text tests/ ... -``` +```Also applies to: 260-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@architecture/pom-vs-fixtures.md` at line 32, Add language tags to the unlabeled fenced code blocks that start with the text "What kind of reusable code?" and the block showing "tests/" so markdownlint MD040 is satisfied and editors render correctly; replace the opening triple-backticks of each block with a labeled fence such as ```text (i.e., change ``` to ```text for the two fenced blocks containing those snippets).testing-patterns/accessibility.md-359-359 (1)
359-359:⚠️ Potential issue | 🟡 MinorLink label is stale vs destination filename
On Line 359, the link text says
test-organization.mdbut the target istest-suite-structure.md. This is confusing in docs navigation.Suggested fix
-- **Visual testing**: See [test-organization.md](../core/test-suite-structure.md) for screenshot comparison +- **Visual testing**: See [test-suite-structure.md](../core/test-suite-structure.md) for screenshot comparison🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing-patterns/accessibility.md` at line 359, The link label is stale: update the link text "test-organization.md" to match the actual target filename "test-suite-structure.md" (or alternatively change the target to match the label) in the sentence under "Visual testing" so the link text and destination are consistent; locate the anchor containing the link to ../core/test-suite-structure.md and edit the visible label accordingly..agents/skills/skill-creator/references/output-patterns.md-56-80 (1)
56-80:⚠️ Potential issue | 🟡 MinorNested code fences are malformed and trigger MD040
The example block in this range nests triple backticks inside a triple-backtick fence, which breaks Markdown structure and causes lint warnings (including Line 68 and Line 77). Use a higher-level outer fence and label inner fences.
Suggested fix
-```markdown +````markdown ## Commit message format ... **Example 1:** Input: Added user authentication with JWT tokens Output: -``` +```text feat(auth): implement JWT-based authentication Add login endpoint and token validation middleware...
Example 2:
Input: Fixed bug where dates displayed incorrectly in reports
Output:
-+text
fix(reports): correct date formatting in timezone conversionUse UTC timestamps consistently across report generation
Follow this style: type(scope): brief description, then detailed explanation. -``` +````🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/skill-creator/references/output-patterns.md around lines 56 - 80, The nested triple-backtick fences in the "## Commit message format" block are malformed and trigger MD040; fix by replacing the outer triple-backtick fence with a higher-level fence (e.g., four backticks) and change the inner example fences to labeled code fences such as ```text so the inner examples are not nested inside identical fences; update the block containing "Example 1" and "Example 2" (the commit message examples) accordingly so the outer fence uses four backticks and each example's Output uses a labeled ```text fence.advanced/mobile-testing.md-406-406 (1)
406-406:⚠️ Potential issue | 🟡 MinorDisplayed link name does not match target file
On Line 406, link text says
test-organization.mdbut target istest-suite-structure.md. Please align the visible label to the actual file.Suggested fix
-- **Visual Testing**: See [test-organization.md](../core/test-suite-structure.md) for screenshot testing +- **Visual Testing**: See [test-suite-structure.md](../core/test-suite-structure.md) for screenshot testing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@advanced/mobile-testing.md` at line 406, The link label and target mismatch: the displayed link text "test-organization.md" should match the actual target "test-suite-structure.md"; edit the markdown line under the "**Visual Testing**" section in advanced/mobile-testing.md and replace the visible link text "test-organization.md" with "test-suite-structure.md" so the label and target are aligned (or alternatively change the href to "test-organization.md" if the intent was to point there).frameworks/angular.md-63-74 (1)
63-74:⚠️ Potential issue | 🟡 MinorAdd a language hint to the project-structure code fence.
This fence is untyped; markdownlint MD040 will keep flagging it.
🛠️ Minimal fix
-``` +```text my-angular-app/ src/ e2e/ ... angular.json</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@frameworks/angular.mdaround lines 63 - 74, The code fence showing the
Angular project structure (the block starting with "my-angular-app/" and
containing src/, e2e/, playwright.config.ts, angular.json) is missing a language
hint; update the opening triple-backtick to include a language tag (e.g., change
totext) so the fence is typed and stops markdownlint MD040 from flagging
it.</details> </blockquote></details> <details> <summary>infrastructure-ci-cd/reporting.md-262-279 (1)</summary><blockquote> `262-279`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language tag to the fenced code block.** The artifact tree block should specify a language (e.g., `text`) to satisfy markdown linting and keep docs CI clean. <details> <summary>Suggested patch</summary> ```diff -``` +```text test-results/ ├── checkout-test-chromium/ ... blob-report/ └── report-1.zip ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@infrastructure-ci-cd/reporting.mdaround lines 262 - 279, The fenced
artifact tree code block is missing a language tag which fails markdown linting;
update the triple-backtick fence that opens the block (the block showing
test-results/, playwright-report/, blob-report/) to include a language tag such
as text (i.e., changetotext) so the code fence is properly recognized
and the docs CI linter passes.</details> </blockquote></details> <details> <summary>.agents/skills/skill-creator/SKILL.md-113-113 (1)</summary><blockquote> `113-113`: _⚠️ Potential issue_ | _🟡 Minor_ **Typo: "auxilary" should be "auxiliary".** <details> <summary>Proposed fix</summary> ```diff -The skill should only contain the information needed for an AI agent to do the job at hand. It should not contain auxilary context about the process that went into creating it, setup and testing procedures, user-facing documentation, etc. Creating additional documentation files just adds clutter and confusion. +The skill should only contain the information needed for an AI agent to do the job at hand. It should not contain auxiliary context about the process that went into creating it, setup and testing procedures, user-facing documentation, etc. Creating additional documentation files just adds clutter and confusion. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/skill-creator/SKILL.md at line 113, Replace the misspelled word "auxilary" with the correct spelling "auxiliary" in the SKILL.md sentence that reads "It should not contain auxilary context..." so the document now reads "It should not contain auxiliary context..."; update only that word in the paragraph to avoid altering surrounding text. ``` </details> </blockquote></details> <details> <summary>SKILL.md-120-120 (1)</summary><blockquote> `120-120`: _⚠️ Potential issue_ | _🟡 Minor_ **Duplicate reference in table row.** The same file `test-suite-structure.md` is listed twice in the reference column for "Test suite structure". <details> <summary>Proposed fix</summary> ```diff -| **Test suite structure** | [test-suite-structure.md](core/test-suite-structure.md), [test-suite-structure.md](core/test-suite-structure.md) | +| **Test suite structure** | [test-suite-structure.md](core/test-suite-structure.md) | ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@SKILL.md` at line 120, The table row for "Test suite structure" contains a duplicated link to test-suite-structure.md; edit the SKILL.md table row that has the label "Test suite structure" and remove the redundant reference so the reference column lists test-suite-structure.md only once (leave a single link [test-suite-structure.md](core/test-suite-structure.md) and delete the duplicate). ``` </details> </blockquote></details> <details> <summary>.agents/skills/skill-creator/scripts/quick_validate.py-73-84 (1)</summary><blockquote> `73-84`: _⚠️ Potential issue_ | _🟡 Minor_ **Empty `description` passes validation after stripping.** Same issue as `name` — a whitespace-only description will pass validation. Since `description` is a required field, it should be validated as non-empty. <details> <summary>Proposed fix</summary> ```diff description = frontmatter.get('description', '') if not isinstance(description, str): return False, f"Description must be a string, got {type(description).__name__}" description = description.strip() + if not description: + return False, "Description cannot be empty" if description: # Check for angle brackets ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/skill-creator/scripts/quick_validate.py around lines 73 - 84, The description field is treated like name and allows whitespace-only values because you only strip but don't reject empty strings; update the validation in quick_validate.py where the variable description is handled so that after description = description.strip() you check if description == '' (or not truthy) and return False with a clear message like "Description is required and cannot be empty or whitespace" before further checks (angle brackets and length) so whitespace-only descriptions fail validation. ``` </details> </blockquote></details> <details> <summary>.agents/skills/skill-creator/scripts/quick_validate.py-52-72 (1)</summary><blockquote> `52-72`: _⚠️ Potential issue_ | _🟡 Minor_ **Empty `name` passes validation after stripping.** If `name` is whitespace-only (e.g., `" "`), `name.strip()` produces an empty string, and the `if name:` block is skipped entirely. This allows an effectively empty name to pass validation. <details> <summary>Proposed fix</summary> ```diff name = frontmatter.get('name', '') if not isinstance(name, str): return False, f"Name must be a string, got {type(name).__name__}" name = name.strip() + if not name: + return False, "Name cannot be empty" if name: # Check naming convention (kebab-case: lowercase with hyphens) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/skill-creator/scripts/quick_validate.py around lines 52 - 72, The validation currently strips whitespace from frontmatter.get('name') into the variable name but then allows empty (whitespace-only) names to pass because the subsequent if name: block is skipped; update quick_validate.py to explicitly reject empty names after stripping by checking if name == '' (or not name) and returning False with an appropriate message (e.g., "Name cannot be empty or whitespace-only"), keeping the existing type check for name and the subsequent kebab-case/length checks intact; reference the frontmatter variable and the name local variable to locate the change. ``` </details> </blockquote></details> <details> <summary>core/configuration.md-446-452 (1)</summary><blockquote> `446-452`: _⚠️ Potential issue_ | _🟡 Minor_ **Inconsistent link format in Related section.** Line 450 uses `test-organization.md` as the link text but links to `test-suite-structure.md`. This appears to be a naming mismatch. <details> <summary>Proposed fix</summary> ```diff -- [test-organization.md](test-suite-structure.md) - file structure and naming +- [test-suite-structure.md](./test-suite-structure.md) - file structure and naming ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@core/configuration.md` around lines 446 - 452, The link text and target are inconsistent in the "Related" list: the item shows "test-organization.md" but links to "test-suite-structure.md"; update this entry so the link text and target match (either change the link text to "test-suite-structure.md" or change the target to "./test-organization.md") and ensure the link uses the same naming pattern as the other entries (e.g., `- [test-suite-structure.md](./test-suite-structure.md)`), so readers see the correct name and destination. ``` </details> </blockquote></details> <details> <summary>testing-patterns/graphql-testing.md-175-204 (1)</summary><blockquote> `175-204`: _⚠️ Potential issue_ | _🟡 Minor_ **Missing error handling in `adminGqlClient` fixture login flow.** If the login mutation fails (e.g., invalid credentials), `data.login` will be undefined, causing `data.login.token` to throw. The fixture should validate the login response before using the token. <details> <summary>Proposed fix</summary> ```diff const { data } = await loginResp.json(); + if (!data?.login?.token) { + throw new Error('Admin login failed: no token received'); + } await loginCtx.dispose(); const ctx = await playwright.request.newContext({ ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@testing-patterns/graphql-testing.md` around lines 175 - 204, The adminGqlClient fixture's login flow currently assumes a successful mutation and directly accesses data.login.token, which will throw if the login failed; update the logic around the playwright.request.newContext/loginResp/json handling to validate the response (check loginResp.ok or status), ensure the parsed JSON contains data and data.login and data.login.token (e.g., data?.login?.token), and if any of those are missing throw or fail the fixture with a clear error message that includes the loginResp status and response body; keep using the existing adminGqlClient fixture and loginCtx/loginResp names so the check is applied before building the authenticated ctx and before disposing loginCtx. ``` </details> </blockquote></details> <details> <summary>testing-patterns/forms-validation.md-60-74 (1)</summary><blockquote> `60-74`: _⚠️ Potential issue_ | _🟡 Minor_ **Race condition: register `waitForResponse` before the triggering action.** On lines 66-67, `waitForResponse` is created after `pressSequentially` has already started typing. If the API responds quickly, the promise might miss the response. The listener should be registered before the action that triggers the request. <details> <summary>Proposed fix</summary> ```diff const streetField = page.getByLabel("Street"); + const responsePromise = page.waitForResponse("**/api/address-lookup*"); await streetField.pressSequentially("456 Elm", { delay: 50 }); - const responsePromise = page.waitForResponse("**/api/address-lookup*"); await responsePromise; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@testing-patterns/forms-validation.md` around lines 60 - 74, The test "typeahead with API-driven suggestions" has a race where page.waitForResponse("**/api/address-lookup*") is awaited after streetField.pressSequentially("456 Elm") begins, so the response may be missed; fix by registering the waitForResponse before triggering the request (move the responsePromise = page.waitForResponse(...) to before calling streetField.pressSequentially) or use Promise.all to run both concurrently (e.g., const responsePromise = page.waitForResponse(...); await Promise.all([responsePromise, streetField.pressSequentially("456 Elm", { delay: 50 })])); ensure the rest of the test still awaits the responsePromise before interacting with options. ``` </details> </blockquote></details> <details> <summary>advanced/authentication.md-732-741 (1)</summary><blockquote> `732-741`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language hint to the fenced decision-tree block.** The code fence starting at Line 732 has no language and will trigger markdown linting (MD040). <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@advanced/authentication.mdaround lines 732 - 741, The fenced decision-tree
block (the triple-backtick block containing the login decision tree) is missing
a language hint and triggers MD040; update the opening fence fromto a language-specified fence such astext (or ```dot if you prefer graphviz) so
the block has a language hint and the markdown linter stops flagging it—leave
the decision-tree content unchanged and only modify the opening fence.</details> </blockquote></details> <details> <summary>advanced/authentication.md-25-29 (1)</summary><blockquote> `25-29`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix invalid object literal syntax in the config snippet.** Line 27 uses a semicolon inside an object literal (`storageState: ".auth/session.json";`), which is invalid JS/TS syntax. Object properties must be separated by commas. <details> <summary>Suggested fix</summary> ```diff { use: { - storageState: ".auth/session.json"; + storageState: ".auth/session.json", } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@advanced/authentication.md` around lines 25 - 29, The snippet contains invalid object literal syntax in the "use" object: the property "storageState" ends with a semicolon instead of a comma or nothing; update the "use" object (the use block and its storageState property) to use valid JS/TS punctuation by replacing the semicolon after storageState: ".auth/session.json" with the correct separator (or remove it if it's the last property) so the object literal is syntactically correct. ``` </details> </blockquote></details> <details> <summary>core/assertions-waiting.md-361-361 (1)</summary><blockquote> `361-361`: _⚠️ Potential issue_ | _🟡 Minor_ **Link text/target mismatch in Related References.** The visible label says `test-organization.md`, but the link points to `test-suite-structure.md`. Consider renaming the label for consistency and to reduce reader confusion. <details> <summary>✏️ Suggested doc-only tweak</summary> ```diff -- **Network interception**: See [test-organization.md](test-suite-structure.md) for API mocking +- **Network interception**: See [test-suite-structure.md](test-suite-structure.md) for API mocking ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@core/assertions-waiting.md` at line 361, The link label/target mismatch on the "Network interception" reference should be fixed: either change the visible label "test-organization.md" to "test-suite-structure.md" to match the link target, or update the link target from "test-suite-structure.md" to "test-organization.md" so both label and target refer to the same document; update the markdown line containing the link text "test-organization.md" / link target "test-suite-structure.md" accordingly to keep label and destination consistent. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (6)</summary><blockquote> <details> <summary>testing-patterns/visual-regression.md (1)</summary><blockquote> `228-231`: **Document cross-platform Docker command alternatives.** The Docker commands using `$(pwd)` work on macOS/Linux but fail on Windows PowerShell and CMD. Consider documenting platform-specific alternatives for Windows users. <details> <summary>📝 Suggested cross-platform guidance</summary> Add a note after line 231: ```markdown **Windows users**: Replace `$(pwd)` with: - PowerShell: `${PWD}` - CMD: `%cd%` Or use the NPM script which can be made cross-platform with a package like `cross-var`. ``` For the package.json script (line 239), consider using a cross-platform approach: ```json { "scripts": { "test:visual": "npx playwright test --project=visual", "test:visual:update": "docker run --rm -v ${PWD}:/work -w /work mcr.microsoft.com/playwright:latest npx playwright test --update-snapshots --project=visual" } } ``` Or recommend tools like `cross-var` for full cross-platform support. </details> Also applies to: 239-239 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@testing-patterns/visual-regression.md` around lines 228 - 231, Add cross-platform guidance after the docker run example in visual-regression.md: explain that $(pwd) is Unix-only and provide Windows alternatives (PowerShell: ${PWD}, CMD: %cd%) and recommend using an npm script (e.g., "test:visual" / "test:visual:update") or tooling like cross-var/cross-env to make the Docker mount path portable; also update the suggested package.json "test:visual:update" example to use the cross-platform variable form or note using cross-var so Windows users can run the same command. ``` </details> </blockquote></details> <details> <summary>infrastructure-ci-cd/parallel-sharding.md (1)</summary><blockquote> `126-130`: **Pin GitHub Actions to specific versions instead of using `@latest`.** The following lines use floating version refs, which weakens reproducibility and supply-chain security in CI/CD documentation: - Lines 126, 242, 272: `actions/checkout@latest` → use `actions/checkout@v4` - Lines 129, 341: `actions/download-artifact@latest` → use `actions/download-artifact@v4` - Lines 137, 336: `actions/upload-artifact@latest` → use `actions/upload-artifact@v4` Also applies to lines in other files: infrastructure-ci-cd/reporting.md (286, 293), infrastructure-ci-cd/github-actions.md (52, 58, 74, 82, 122, 128, 145, 158, 163, 173, 214, 223, 258, 264, 280, 312, 322, 331, 376, 382), infrastructure-ci-cd/docker.md (138). <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@infrastructure-ci-cd/parallel-sharding.md` around lines 126 - 130, Replace floating GitHub Action refs with pinned versions: change "actions/checkout@latest" to "actions/checkout@v4", "actions/download-artifact@latest" to "actions/download-artifact@v4", and "actions/upload-artifact@latest" to "actions/upload-artifact@v4". Update each occurrence shown in this PR (e.g., the "actions/checkout@latest", "actions/download-artifact@latest", and "actions/upload-artifact@latest" strings in parallel-sharding.md and the other mentioned docs) to the corresponding `@v4` tag, and run a quick repo-wide search for any remaining "@latest" action refs to pin them similarly. ``` </details> </blockquote></details> <details> <summary>.agents/skills/skill-creator/scripts/quick_validate.py (1)</summary><blockquote> `6-10`: **Unused import: `os` is imported but never used.** <details> <summary>Proposed fix</summary> ```diff import sys -import os import re import yaml from pathlib import Path ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/skill-creator/scripts/quick_validate.py around lines 6 - 10, Remove the unused import by deleting "import os" from the top of quick_validate.py; ensure the remaining imports (sys, re, yaml, Path from pathlib) are kept intact and run tests/lint to confirm no other references to "os" remain in the file (look for any usage in functions or helpers within quick_validate.py). ``` </details> </blockquote></details> <details> <summary>.agents/skills/skill-creator/scripts/package_skill.py (1)</summary><blockquote> `67-82`: **Catch specific exceptions instead of bare `Exception`.** Catching `Exception` masks unexpected errors (e.g., `KeyboardInterrupt` is not caught, but other programming errors are). For file operations, catch the specific exceptions that can occur. <details> <summary>Proposed fix</summary> ```diff try: with zipfile.ZipFile(skill_filename, 'w', zipfile.ZIP_DEFLATED) as zipf: # Walk through the skill directory for file_path in skill_path.rglob('*'): if file_path.is_file(): # Calculate the relative path within the zip arcname = file_path.relative_to(skill_path.parent) zipf.write(file_path, arcname) print(f" Added: {arcname}") print(f"\n✅ Successfully packaged skill to: {skill_filename}") return skill_filename - except Exception as e: + except (OSError, zipfile.BadZipFile) as e: print(f"❌ Error creating .skill file: {e}") return None ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/skill-creator/scripts/package_skill.py around lines 67 - 82, Replace the broad "except Exception as e" in the packaging block with specific filesystem/zip related exceptions (e.g., OSError, FileNotFoundError, PermissionError and zipfile.BadZipFile) so only expected I/O/zip failures are caught; reference the ZipFile context where skill_filename and skill_path are used and the zipf.write/arcname logic, and ensure each except branch logs the exception details and returns None (or re-raises unexpected exceptions) rather than silently catching all Exception types. ``` </details> </blockquote></details> <details> <summary>.agents/skills/skill-creator/scripts/init_skill.py (1)</summary><blockquote> `217-219`: **Use narrower filesystem exceptions instead of catching `Exception` broadly.** Replace `except Exception as e:` with `except OSError as e:` (or `except (OSError, PermissionError) as e:` for finer-grained handling) at: - Line 217: `mkdir()` operation can raise `FileExistsError`, `PermissionError`, etc. - Line 232: `write_text()` can raise `PermissionError`, `FileNotFoundError`, etc. - Line 259: Multiple filesystem operations (`mkdir()`, `write_text()`, `chmod()`) raise `OSError` and subclasses Catching `Exception` masks unexpected errors; using `OSError` lets legitimate filesystem errors be handled while unexpected bugs propagate. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/skill-creator/scripts/init_skill.py around lines 217 - 219, Replace the broad "except Exception as e:" handlers around filesystem operations with narrower OS-related exceptions: use "except OSError as e:" (or "except (OSError, PermissionError) as e:" if you want PermissionError handled separately) for the blocks that wrap mkdir(), write_text(), and the combined mkdir/write_text/chmod sequence in init_skill.py so filesystem errors are handled but unrelated exceptions are not swallowed; keep the existing error logging/print and the "return None" behavior inside those narrower except blocks and let any other unexpected exceptions propagate. ``` </details> </blockquote></details> <details> <summary>infrastructure-ci-cd/performance.md (1)</summary><blockquote> `452-452`: **Update stale link label to match renamed document.** The target points to `test-suite-structure.md`, but the visible label still says `test-organization.md`. Please align the label with the new canonical name to avoid confusion. <details> <summary>Proposed doc fix</summary> ```diff -- **Test organization**: See [test-organization.md](../core/test-suite-structure.md) for structuring tests +- **Test organization**: See [test-suite-structure.md](../core/test-suite-structure.md) for structuring tests ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@infrastructure-ci-cd/performance.md` at line 452, Update the stale Markdown link label so the visible text matches the renamed target: replace the link text "[test-organization.md]" in the line beginning "**Test organization**: See [test-organization.md](../core/test-suite-structure.md)" with a label matching the target (for example "[test-suite-structure.md]" or "Test suite structure") so the link reads "**Test organization**: See [test-suite-structure.md](../core/test-suite-structure.md)" (or similar consistent label). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/skill-creator/scripts/init_skill.py:
- Around line 229-234: The write to SKILL.md (skill_md_path.write_text) and
subsequent resource creation can fail and currently return None while leaving a
partially-created skill scaffold; wrap the SKILL.md write and the other resource
creation steps (the blocks around skill_md_path.write_text and the code in the
237-261 region) in try/except/finally so that on any exception you remove any
files/directories created for this scaffold (e.g., call Path.unlink() for
created files and shutil.rmtree() for the skill directory) before returning
None; ensure you identify created artifacts (using variables like skill_md_path
and the skill directory Path variable) so the cleanup removes only this partial
scaffold.- Around line 274-288: The script currently accepts any skill name from
sys.argv[1] without enforcing the documented constraints; add validation after
assigning skill_name and path to reject invalid names: check skill_name matches
the kebab-case pattern (only lowercase letters, digits and hyphens, max 64
chars) using a regex like ^[a-z0-9-]{1,64}$ and also verify that skill_name ==
os.path.basename(path) (use os.path.normpath on path) to ensure it matches the
target directory name exactly; if either check fails, print a clear usage/error
message and sys.exit(1). Ensure you reference the existing variables skill_name
and path in init_skill.py when implementing these checks.In @.agents/skills/skill-creator/scripts/package_skill.py:
- Around line 13-16: The import of validate_skill from quick_validate is fragile
because it assumes quick_validate is on sys.path; update package_skill.py to
load quick_validate relative to the script file instead of relying on CWD:
compute the script directory via Path(file).resolve().parent and insert that
directory (or its parent if quick_validate sits alongside) into sys.path before
importing validate_skill, or use importlib.util.spec_from_file_location to
import the module by file path; target the top-level import statement that
references quick_validate and ensure the module is loaded reliably regardless of
the current working directory.In
@core/test-data.md:
- Line 492: Replace all stale references to the removed file name
"test-organization.md" with the new "test-suite-structure.md" across the
repository; specifically update the occurrences in the listed markdown files
(e.g., the line in core/test-data.md that currently reads "API Testing: See
[test-organization.md]..." and similar links in
testing-patterns/canvas-webgl.md, testing-patterns/accessibility.md,
infrastructure-ci-cd/performance.md, core/page-object-model.md,
core/fixtures-hooks.md, core/configuration.md, core/assertions-waiting.md,
advanced/network-advanced.md, advanced/mobile-testing.md, and
architecture/test-architecture.md) so all links point to
test-suite-structure.md.In
@infrastructure-ci-cd/docker.md:
- Line 138: Replace the floating ref "uses: actions/checkout@latest" with a
pinned ref to ensure reproducible CI; update the uses entry (the
actions/checkout reference) to either a full commit SHA (e.g.,
@de0fac2e4500dabe0009e67214ff5f5447ce83dd) or a stable major tag like@v6, and
commit that change so the workflow no longer depends on the unsupported@latest
ref.In
@infrastructure-ci-cd/github-actions.md:
- Around line 353-356: The workflow declares an unused input node-version and
references strategy.job-index in a job without a matrix; fix by adding an
actions/setup-node step that uses the node-version input (use
inputs.node-version in the setup-node step) so the input is actually applied,
and remove or replace the invalid strategy.job-index reference (either convert
the job to use a strategy.matrix so job-index exists or change uses of
strategy.job-index to a valid context such as github.run_id or the job’s own
index variable); update the step or job that currently reads strategy.job-index
to use the chosen valid context.- Around line 52-53: The workflow currently references actions with floating
tags like "actions/checkout@latest"; update every occurrence (e.g., the
"actions/checkout@latest" entry and the other floating tags noted at lines 58,
74, 82, 122–123, 145–146, 163, 173, 211, 214, 223, 331, 376, 382, 398) to pin
each action to its full-length commit SHA instead oflatestor version tags so
runs are reproducible and supply-chain safe; locate each uses: entry in the
workflow file and replace the tag with the corresponding action repository’s
full commit SHA (you can copy the SHA from the action repo’s commit history or
use Dependabot to automate updates).In
@infrastructure-ci-cd/gitlab.md:
- Line 30: Replace all occurrences of the floating Playwright Docker image
"mcr.microsoft.com/playwright:latest" used in examples and troubleshooting
snippets with a pinned tag that matches the Playwright version (use the pattern
"mcr.microsoft.com/playwright:v-noble"); update every
snippet that references "mcr.microsoft.com/playwright:latest" so the docs
consistently instruct pinning the image to a specific version and include the
pinned format in the troubleshooting "Fix" example as well.In
@infrastructure-ci-cd/other-providers.md:
- Around line 36-37: Replace floating Playwright Docker tags and orb pins with
explicit, pinned versions: change occurrences of image
'mcr.microsoft.com/playwright:latest' to a specific version tag or digest (e.g.,
match the vX.Y.Z used by@playwright/testin package.json) and update any
CircleCI orb references that use@latestto pinned@major.minor(or
@major.minor.patch); update examples in the Jenkins and CircleCI snippets
referenced (lines noted in the comment) accordingly. Also modify the
troubleshooting section (currently recommending:latest) to instead instruct
users to pin the Playwright Docker image version to match the@playwright/test
dependency and to update both in the same PR. Ensure the docs mention pinning by
digest as the most reproducible option.In
@infrastructure-ci-cd/reporting.md:
- Around line 154-161: The webhook POST in the reporter's onEnd currently calls
fetch(webhookUrl, ...) without timeout or error handling; update the onEnd
implementation to race the fetch against a short timeout (use AbortController or
Promise.race) so the request is aborted/stopped after the timeout, catch and log
any errors, and ensure onEnd does not await indefinitely—use the
NOTIFICATION_WEBHOOK env var, the webhookUrl variable, and the
message.join('\n') payload but wrap the network call with a bounded timeout and
error handling to prevent CI stalls.In
@testing-patterns/visual-regression.md:
- Around line 568-581: The docs currently suggest using "latest" for both the
Playwright npm package and Docker image which undermines version pinning; update
the guidance to instruct readers to pin a matching, specific Playwright version
instead of "latest" by replacing the "@playwright/test": "latest" recommendation
in package.json and the container image mcr.microsoft.com/playwright:latest in
CI with a concrete version (e.g., "1.40.0" and
mcr.microsoft.com/playwright:v1.40.0-jammy) and call out that both should be
updated together when upgrading Playwright to ensure reproducible rendering
across environments.
Outside diff comments:
In@debugging/debugging.md:
- Line 186: Update the inconsistent link in debugging.md by replacing the flat
reference "assertions-waiting.md#waiting-strategies" with the relative path used
elsewhere "../core/assertions-waiting.md#waiting-strategies" so the link
resolves correctly (search for the exact string
"assertions-waiting.md#waiting-strategies" in the file and update it to
"../core/assertions-waiting.md#waiting-strategies").
Minor comments:
In @.agents/skills/skill-creator/references/output-patterns.md:
- Around line 56-80: The nested triple-backtick fences in the "## Commit message
format" block are malformed and trigger MD040; fix by replacing the outer
triple-backtick fence with a higher-level fence (e.g., four backticks) and
change the inner example fences to labeled code fences such as ```text so the
inner examples are not nested inside identical fences; update the block
containing "Example 1" and "Example 2" (the commit message examples) accordingly
so the outer fence uses four backticks and each example's Output uses a labeledIn @.agents/skills/skill-creator/scripts/quick_validate.py: - Around line 73-84: The description field is treated like name and allows whitespace-only values because you only strip but don't reject empty strings; update the validation in quick_validate.py where the variable description is handled so that after description = description.strip() you check if description == '' (or not truthy) and return False with a clear message like "Description is required and cannot be empty or whitespace" before further checks (angle brackets and length) so whitespace-only descriptions fail validation. - Around line 52-72: The validation currently strips whitespace from frontmatter.get('name') into the variable name but then allows empty (whitespace-only) names to pass because the subsequent if name: block is skipped; update quick_validate.py to explicitly reject empty names after stripping by checking if name == '' (or not name) and returning False with an appropriate message (e.g., "Name cannot be empty or whitespace-only"), keeping the existing type check for name and the subsequent kebab-case/length checks intact; reference the frontmatter variable and the name local variable to locate the change. In @.agents/skills/skill-creator/SKILL.md: - Line 113: Replace the misspelled word "auxilary" with the correct spelling "auxiliary" in the SKILL.md sentence that reads "It should not contain auxilary context..." so the document now reads "It should not contain auxiliary context..."; update only that word in the paragraph to avoid altering surrounding text. In `@advanced/authentication.md`: - Around line 732-741: The fenced decision-tree block (the triple-backtick block containing the login decision tree) is missing a language hint and triggers MD040; update the opening fence from ``` to a language-specified fence such as ```text (or ```dot if you prefer graphviz) so the block has a language hint and the markdown linter stops flagging it—leave the decision-tree content unchanged and only modify the opening fence. - Around line 25-29: The snippet contains invalid object literal syntax in the "use" object: the property "storageState" ends with a semicolon instead of a comma or nothing; update the "use" object (the use block and its storageState property) to use valid JS/TS punctuation by replacing the semicolon after storageState: ".auth/session.json" with the correct separator (or remove it if it's the last property) so the object literal is syntactically correct. In `@advanced/mobile-testing.md`: - Line 406: The link label and target mismatch: the displayed link text "test-organization.md" should match the actual target "test-suite-structure.md"; edit the markdown line under the "**Visual Testing**" section in advanced/mobile-testing.md and replace the visible link text "test-organization.md" with "test-suite-structure.md" so the label and target are aligned (or alternatively change the href to "test-organization.md" if the intent was to point there). In `@architecture/pom-vs-fixtures.md`: - Line 32: Add language tags to the unlabeled fenced code blocks that start with the text "What kind of reusable code?" and the block showing "tests/" so markdownlint MD040 is satisfied and editors render correctly; replace the opening triple-backticks of each block with a labeled fence such as ```text (i.e., change ``` to ```text for the two fenced blocks containing those snippets). In `@core/assertions-waiting.md`: - Line 361: The link label/target mismatch on the "Network interception" reference should be fixed: either change the visible label "test-organization.md" to "test-suite-structure.md" to match the link target, or update the link target from "test-suite-structure.md" to "test-organization.md" so both label and target refer to the same document; update the markdown line containing the link text "test-organization.md" / link target "test-suite-structure.md" accordingly to keep label and destination consistent. In `@core/configuration.md`: - Around line 446-452: The link text and target are inconsistent in the "Related" list: the item shows "test-organization.md" but links to "test-suite-structure.md"; update this entry so the link text and target match (either change the link text to "test-suite-structure.md" or change the target to "./test-organization.md") and ensure the link uses the same naming pattern as the other entries (e.g., `- [test-suite-structure.md](./test-suite-structure.md)`), so readers see the correct name and destination. In `@frameworks/angular.md`: - Around line 63-74: The code fence showing the Angular project structure (the block starting with "my-angular-app/" and containing src/, e2e/, playwright.config.ts, angular.json) is missing a language hint; update the opening triple-backtick to include a language tag (e.g., change ``` to ```text) so the fence is typed and stops markdownlint MD040 from flagging it. In `@infrastructure-ci-cd/reporting.md`: - Around line 262-279: The fenced artifact tree code block is missing a language tag which fails markdown linting; update the triple-backtick fence that opens the block (the block showing test-results/, playwright-report/, blob-report/) to include a language tag such as text (i.e., change ``` to ```text) so the code fence is properly recognized and the docs CI linter passes. In `@SKILL.md`: - Line 120: The table row for "Test suite structure" contains a duplicated link to test-suite-structure.md; edit the SKILL.md table row that has the label "Test suite structure" and remove the redundant reference so the reference column lists test-suite-structure.md only once (leave a single link [test-suite-structure.md](core/test-suite-structure.md) and delete the duplicate). In `@testing-patterns/accessibility.md`: - Line 359: The link label is stale: update the link text "test-organization.md" to match the actual target filename "test-suite-structure.md" (or alternatively change the target to match the label) in the sentence under "Visual testing" so the link text and destination are consistent; locate the anchor containing the link to ../core/test-suite-structure.md and edit the visible label accordingly. In `@testing-patterns/forms-validation.md`: - Around line 60-74: The test "typeahead with API-driven suggestions" has a race where page.waitForResponse("**/api/address-lookup*") is awaited after streetField.pressSequentially("456 Elm") begins, so the response may be missed; fix by registering the waitForResponse before triggering the request (move the responsePromise = page.waitForResponse(...) to before calling streetField.pressSequentially) or use Promise.all to run both concurrently (e.g., const responsePromise = page.waitForResponse(...); await Promise.all([responsePromise, streetField.pressSequentially("456 Elm", { delay: 50 })])); ensure the rest of the test still awaits the responsePromise before interacting with options. In `@testing-patterns/graphql-testing.md`: - Around line 175-204: The adminGqlClient fixture's login flow currently assumes a successful mutation and directly accesses data.login.token, which will throw if the login failed; update the logic around the playwright.request.newContext/loginResp/json handling to validate the response (check loginResp.ok or status), ensure the parsed JSON contains data and data.login and data.login.token (e.g., data?.login?.token), and if any of those are missing throw or fail the fixture with a clear error message that includes the loginResp status and response body; keep using the existing adminGqlClient fixture and loginCtx/loginResp names so the check is applied before building the authenticated ctx and before disposing loginCtx. In `@testing-patterns/visual-regression.md`: - Around line 117-126: Replace the non-deterministic page.waitForTimeout(500) in the test('page with JS animations') with a deterministic wait: remove the waitForTimeout call and wait for a specific condition that indicates the animation finished (for example use page.getByTestId('hero-banner').waitFor({ state: 'stable' }) if available, or await page.waitForFunction to assert computed style/transform/opacity has reached the final value, or wait for a CSS class/attribute that marks animation completion) before calling expect(page).toHaveScreenshot; keep the rest of the test (page.goto, locator waitFor visible, and toHaveScreenshot with animations:'disabled') intact. --- Nitpick comments: In @.agents/skills/skill-creator/scripts/init_skill.py: - Around line 217-219: Replace the broad "except Exception as e:" handlers around filesystem operations with narrower OS-related exceptions: use "except OSError as e:" (or "except (OSError, PermissionError) as e:" if you want PermissionError handled separately) for the blocks that wrap mkdir(), write_text(), and the combined mkdir/write_text/chmod sequence in init_skill.py so filesystem errors are handled but unrelated exceptions are not swallowed; keep the existing error logging/print and the "return None" behavior inside those narrower except blocks and let any other unexpected exceptions propagate. In @.agents/skills/skill-creator/scripts/package_skill.py: - Around line 67-82: Replace the broad "except Exception as e" in the packaging block with specific filesystem/zip related exceptions (e.g., OSError, FileNotFoundError, PermissionError and zipfile.BadZipFile) so only expected I/O/zip failures are caught; reference the ZipFile context where skill_filename and skill_path are used and the zipf.write/arcname logic, and ensure each except branch logs the exception details and returns None (or re-raises unexpected exceptions) rather than silently catching all Exception types. In @.agents/skills/skill-creator/scripts/quick_validate.py: - Around line 6-10: Remove the unused import by deleting "import os" from the top of quick_validate.py; ensure the remaining imports (sys, re, yaml, Path from pathlib) are kept intact and run tests/lint to confirm no other references to "os" remain in the file (look for any usage in functions or helpers within quick_validate.py). In `@infrastructure-ci-cd/parallel-sharding.md`: - Around line 126-130: Replace floating GitHub Action refs with pinned versions: change "actions/checkout@latest" to "actions/checkout@v4", "actions/download-artifact@latest" to "actions/download-artifact@v4", and "actions/upload-artifact@latest" to "actions/upload-artifact@v4". Update each occurrence shown in this PR (e.g., the "actions/checkout@latest", "actions/download-artifact@latest", and "actions/upload-artifact@latest" strings in parallel-sharding.md and the other mentioned docs) to the corresponding `@v4` tag, and run a quick repo-wide search for any remaining "@latest" action refs to pin them similarly. In `@infrastructure-ci-cd/performance.md`: - Line 452: Update the stale Markdown link label so the visible text matches the renamed target: replace the link text "[test-organization.md]" in the line beginning "**Test organization**: See [test-organization.md](../core/test-suite-structure.md)" with a label matching the target (for example "[test-suite-structure.md]" or "Test suite structure") so the link reads "**Test organization**: See [test-suite-structure.md](../core/test-suite-structure.md)" (or similar consistent label). In `@testing-patterns/visual-regression.md`: - Around line 228-231: Add cross-platform guidance after the docker run example in visual-regression.md: explain that $(pwd) is Unix-only and provide Windows alternatives (PowerShell: ${PWD}, CMD: %cd%) and recommend using an npm script (e.g., "test:visual" / "test:visual:update") or tooling like cross-var/cross-env to make the Docker mount path portable; also update the suggested package.json "test:visual:update" example to use the cross-platform variable form or note using cross-var so Windows users can run the same command.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (68)
.agents/skills/skill-creator/LICENSE.txt.agents/skills/skill-creator/SKILL.md.agents/skills/skill-creator/references/output-patterns.md.agents/skills/skill-creator/references/workflows.md.agents/skills/skill-creator/scripts/init_skill.py.agents/skills/skill-creator/scripts/package_skill.py.agents/skills/skill-creator/scripts/quick_validate.py.cursor/skills/skill-creatorREADME.mdSKILL.mdadvanced/authentication-flows.mdadvanced/authentication.mdadvanced/clock-mocking.mdadvanced/mobile-testing.mdadvanced/multi-context.mdadvanced/multi-user.mdadvanced/network-advanced.mdadvanced/third-party.mdarchitecture/pom-vs-fixtures.mdarchitecture/test-architecture.mdarchitecture/when-to-mock.mdbrowser-apis/browser-apis.mdbrowser-apis/iframes.mdbrowser-apis/service-workers.mdbrowser-apis/websockets.mdcore/annotations.mdcore/assertions-waiting.mdcore/configuration.mdcore/fixtures-hooks.mdcore/global-setup.mdcore/locators.mdcore/page-object-model.mdcore/projects-dependencies.mdcore/test-data.mdcore/test-suite-structure.mdcore/test-tags.mddebugging/console-errors.mddebugging/debugging.mddebugging/error-testing.mddebugging/flaky-tests.mdframeworks/angular.mdframeworks/nextjs.mdframeworks/react.mdframeworks/vue.mdinfrastructure-ci-cd/ci-cd.mdinfrastructure-ci-cd/docker.mdinfrastructure-ci-cd/github-actions.mdinfrastructure-ci-cd/gitlab.mdinfrastructure-ci-cd/other-providers.mdinfrastructure-ci-cd/parallel-sharding.mdinfrastructure-ci-cd/performance.mdinfrastructure-ci-cd/reporting.mdinfrastructure-ci-cd/test-coverage.mdtesting-patterns/accessibility.mdtesting-patterns/api-testing.mdtesting-patterns/browser-extensions.mdtesting-patterns/canvas-webgl.mdtesting-patterns/component-testing.mdtesting-patterns/drag-drop.mdtesting-patterns/electron.mdtesting-patterns/file-operations.mdtesting-patterns/file-upload-download.mdtesting-patterns/forms-validation.mdtesting-patterns/graphql-testing.mdtesting-patterns/i18n.mdtesting-patterns/performance-testing.mdtesting-patterns/security-testing.mdtesting-patterns/visual-regression.md
Comment on lines
+229
to
+234
| try: | ||
| skill_md_path.write_text(skill_content) | ||
| print("✅ Created SKILL.md") | ||
| except Exception as e: | ||
| print(f"❌ Error creating SKILL.md: {e}") | ||
| return None |
There was a problem hiding this comment.
Rollback partially created scaffolds on write failures.
If SKILL.md or resource creation fails, the function returns None but leaves a partially created directory. The next run then fails at Line 209 (“already exists”).
Suggested fix pattern
+import shutil
@@
try:
skill_md_path.write_text(skill_content)
print("✅ Created SKILL.md")
except Exception as e:
print(f"❌ Error creating SKILL.md: {e}")
+ shutil.rmtree(skill_dir, ignore_errors=True)
return None
@@
except Exception as e:
print(f"❌ Error creating resource directories: {e}")
+ shutil.rmtree(skill_dir, ignore_errors=True)
return NoneAlso applies to: 237-261
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 232-232: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/skill-creator/scripts/init_skill.py around lines 229 - 234,
The write to SKILL.md (skill_md_path.write_text) and subsequent resource
creation can fail and currently return None while leaving a partially-created
skill scaffold; wrap the SKILL.md write and the other resource creation steps
(the blocks around skill_md_path.write_text and the code in the 237-261 region)
in try/except/finally so that on any exception you remove any files/directories
created for this scaffold (e.g., call Path.unlink() for created files and
shutil.rmtree() for the skill directory) before returning None; ensure you
identify created artifacts (using variables like skill_md_path and the skill
directory Path variable) so the cleanup removes only this partial scaffold.
Comment on lines
+274
to
+288
| if len(sys.argv) < 4 or sys.argv[2] != '--path': | ||
| print("Usage: init_skill.py <skill-name> --path <path>") | ||
| print("\nSkill name requirements:") | ||
| print(" - Kebab-case identifier (e.g., 'my-data-analyzer')") | ||
| print(" - Lowercase letters, digits, and hyphens only") | ||
| print(" - Max 64 characters") | ||
| print(" - Must match directory name exactly") | ||
| print("\nExamples:") | ||
| print(" init_skill.py my-new-skill --path skills/public") | ||
| print(" init_skill.py my-api-helper --path skills/private") | ||
| print(" init_skill.py custom-skill --path /custom/location") | ||
| sys.exit(1) | ||
|
|
||
| skill_name = sys.argv[1] | ||
| path = sys.argv[3] |
There was a problem hiding this comment.
Enforce the documented skill-name constraints before initialization.
Lines 276–280 document strict rules, but Lines 287–288 accept any string. Invalid names can slip through and break downstream tooling assumptions.
Suggested fix
import sys
+import re
from pathlib import Path
+SKILL_NAME_RE = re.compile(r"^[a-z0-9](?:[a-z0-9-]{0,62}[a-z0-9])?$")
+
def main():
@@
skill_name = sys.argv[1]
path = sys.argv[3]
+
+ if not SKILL_NAME_RE.fullmatch(skill_name):
+ print(f"❌ Invalid skill name: {skill_name}")
+ print(" Use kebab-case, lowercase letters/digits/hyphens, max 64 chars.")
+ sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/skill-creator/scripts/init_skill.py around lines 274 - 288,
The script currently accepts any skill name from sys.argv[1] without enforcing
the documented constraints; add validation after assigning skill_name and path
to reject invalid names: check skill_name matches the kebab-case pattern (only
lowercase letters, digits and hyphens, max 64 chars) using a regex like
^[a-z0-9-]{1,64}$ and also verify that skill_name == os.path.basename(path) (use
os.path.normpath on path) to ensure it matches the target directory name
exactly; if either check fails, print a clear usage/error message and
sys.exit(1). Ensure you reference the existing variables skill_name and path in
init_skill.py when implementing these checks.
Comment on lines
+13
to
+16
| import sys | ||
| import zipfile | ||
| from pathlib import Path | ||
| from quick_validate import validate_skill |
There was a problem hiding this comment.
Relative import may fail when script is invoked from a different directory.
from quick_validate import validate_skill assumes quick_validate.py is in sys.path. When running from outside the scripts/ directory, this import will fail with ModuleNotFoundError.
Proposed fix using path-relative import
import sys
import zipfile
from pathlib import Path
-from quick_validate import validate_skill
+
+# Add script directory to path for sibling imports
+SCRIPT_DIR = Path(__file__).resolve().parent
+sys.path.insert(0, str(SCRIPT_DIR))
+from quick_validate import validate_skill🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/skill-creator/scripts/package_skill.py around lines 13 - 16,
The import of validate_skill from quick_validate is fragile because it assumes
quick_validate is on sys.path; update package_skill.py to load quick_validate
relative to the script file instead of relying on CWD: compute the script
directory via Path(__file__).resolve().parent and insert that directory (or its
parent if quick_validate sits alongside) into sys.path before importing
validate_skill, or use importlib.util.spec_from_file_location to import the
module by file path; target the top-level import statement that references
quick_validate and ensure the module is loaded reliably regardless of the
current working directory.
core/test-data.md
Outdated
|
|
||
| - **Fixtures**: See [fixtures-hooks.md](fixtures-hooks.md) for fixture patterns | ||
| - **API Testing**: See [test-organization.md](test-organization.md) for API mocking | ||
| - **API Testing**: See [test-organization.md](test-suite-structure.md) for API mocking |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify test-organization.md was renamed to test-suite-structure.md
echo "=== Checking for new file ==="
fd --type f --full-path 'test-suite-structure.md$'
echo -e "\n=== Checking for old file (should not exist) ==="
fd --type f --full-path 'test-organization.md$'
echo -e "\n=== Checking for any remaining references to old filename ==="
rg -l 'test-organization\.md' --type=mdRepository: currents-dev/playwright-best-practices-skill
Length of output: 568
Update all stale references to test-organization.md in the codebase.
The file rename from test-organization.md to test-suite-structure.md is incomplete. While the new file exists and the old file has been removed, 11 files still reference the old filename:
- testing-patterns/canvas-webgl.md
- testing-patterns/accessibility.md
- infrastructure-ci-cd/performance.md
- core/test-data.md
- core/page-object-model.md
- core/fixtures-hooks.md
- core/configuration.md
- core/assertions-waiting.md
- advanced/network-advanced.md
- advanced/mobile-testing.md
- architecture/test-architecture.md
Update all references from test-organization.md to test-suite-structure.md in these files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/test-data.md` at line 492, Replace all stale references to the removed
file name "test-organization.md" with the new "test-suite-structure.md" across
the repository; specifically update the occurrences in the listed markdown files
(e.g., the line in core/test-data.md that currently reads "**API Testing**: See
[test-organization.md]..." and similar links in
testing-patterns/canvas-webgl.md, testing-patterns/accessibility.md,
infrastructure-ci-cd/performance.md, core/page-object-model.md,
core/fixtures-hooks.md, core/configuration.md, core/assertions-waiting.md,
advanced/network-advanced.md, advanced/mobile-testing.md, and
architecture/test-architecture.md) so all links point to
test-suite-structure.md.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
waltergalvao
requested changes
Feb 27, 2026
Contributor
waltergalvao
left a comment
waltergalvao
left a comment
There was a problem hiding this comment.
Seems some referenced files needs to be updated after their name changed.
Also, I think we should remove the .agents / .cursor folders here - otherwise people trying to install our skill will be prompted to choose between playwright-best-practices-skill or skill-creator (the CLI command reads all SKILL.md from the repository)
Author
|
@cursor address the comments |
SummaryI've successfully addressed all the CodeRabbit review comments. Here's what was fixed: Documentation Links & References:
Version Pinning:
Validation Scripts:
Markdown Linting:
Code Quality:
Repository Structure:
All changes have been committed and pushed to the |
- Fix stale test-organization.md references across 11 markdown files - Pin GitHub Actions versions from @latest to @v4 - Pin Playwright Docker images from :latest to v1.48.0-noble - Add validation for empty name/description in quick_validate.py - Add skill name validation and cleanup on failure in init_skill.py - Fix fragile import in package_skill.py - Replace broad Exception handlers with specific OSError/BadZipFile - Add language tags to unlabeled code fences (MD040) - Fix nested code fences in output-patterns.md - Fix typo: auxilary -> auxiliary - Remove duplicate link in SKILL.md - Fix race condition in forms-validation.md (waitForResponse) - Add error handling in graphql-testing.md login flow - Add timeout/error handling to webhook call in reporting.md - Replace waitForTimeout with deterministic wait in visual-regression.md - Fix invalid object literal syntax in authentication.md - Remove .agents and .cursor folders per maintainer feedback Co-authored-by: Andrew Goldis <agoldis@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
infrastructure-ci-cd/other-providers.md (1)
251-252:⚠️ Potential issue | 🟡 MinorPin CircleCI orb to specific version instead of
@latest.Line 252 uses
circleci/node@latest, which creates non-reproducible builds and can introduce breaking changes. CircleCI best practice is to pin orbs to@major.minor(e.g.,@5.2) or@major.minor.patchfor stability.♻️ Suggested fix
orbs: - node: circleci/node@latest + node: circleci/node@6.1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure-ci-cd/other-providers.md` around lines 251 - 252, The CircleCI orb declaration currently uses an unpinned tag "circleci/node@latest" (under the orbs -> node key); replace it with a specific semver-pinned orb like "circleci/node@<major.minor>" or "circleci/node@<major.minor.patch>" (e.g., `@5.2` or `@5.2.1`) to ensure reproducible builds and stability, and update any other occurrences of "circleci/node@latest" in this file to the chosen pinned version.testing-patterns/visual-regression.md (1)
571-584:⚠️ Potential issue | 🟡 MinorPin specific matching versions instead of recommending
"latest".The troubleshooting advice recommends
"latest"for both the npm package and Docker image, which undermines reproducibility. Instead, advise pinning both to the same specific version (e.g.,"1.48.0"andv1.48.0-noble) and updating them together.🔧 Suggested fix
{ "devDependencies": { - "@playwright/test": "latest" + "@playwright/test": "1.48.0" } }container: image: mcr.microsoft.com/playwright:v1.48.0-nobleAdd guidance:
**Best practice**: Pin both the npm package and Docker image to matching specific versions. Update both together when upgrading Playwright to ensure consistent rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing-patterns/visual-regression.md` around lines 571 - 584, Update the guidance to recommend pinning exact matching Playwright versions instead of using "latest": change the note around package.json/@playwright/test to instruct pinning the npm dependency to a specific version (e.g., "1.48.0") and the container image tag to the matching Docker tag (e.g., v1.48.0-noble), and add a short "Best practice" sentence advising to update both package.json and the container image together to maintain reproducible renders.
🧹 Nitpick comments (5)
testing-patterns/graphql-testing.md (1)
217-240: Consider enhancing the helper to check GraphQL errors or document the pattern.The helper function checks HTTP-level success (
resp.ok()) but doesn't check for GraphQL-level errors, which is somewhat inconsistent with the document's repeated emphasis that "GraphQL returns 200 even on errors — always check both."While the usage example correctly shows callers checking the
errorsarray, the helper could be more defensive by either:
- Optionally checking for and throwing on GraphQL errors
- Adding a JSDoc comment clarifying that callers must check the
errorsarray in the returned objectThis would make the pattern clearer and reduce the risk of developers forgetting to check GraphQL errors.
💡 Proposed enhancement with optional error checking
// utils/graphql.ts import { APIRequestContext, expect } from "@playwright/test"; +/** + * Execute a GraphQL query. Checks HTTP-level success but does NOT validate GraphQL errors. + * Callers MUST check the returned `errors` array for GraphQL-level failures. + */ export async function gqlQuery<T = any>( request: APIRequestContext, query: string, variables?: Record<string, any> ): Promise<{ data: T; errors?: any[] }> { const resp = await request.post("/graphql", { data: { query, variables }, }); expect(resp.ok()).toBeTruthy(); return resp.json(); }Or, for a more defensive approach that validates GraphQL errors:
export async function gqlQuery<T = any>( request: APIRequestContext, query: string, variables?: Record<string, any>, options?: { allowErrors?: boolean } ): Promise<{ data: T; errors?: any[] }> { const resp = await request.post("/graphql", { data: { query, variables }, }); expect(resp.ok()).toBeTruthy(); const result = await resp.json(); if (!options?.allowErrors && result.errors) { throw new Error(`GraphQL errors: ${JSON.stringify(result.errors)}`); } return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing-patterns/graphql-testing.md` around lines 217 - 240, Update the helper to detect GraphQL-level errors and fail tests by default: modify gqlQuery to accept an optional options parameter (e.g., { allowErrors?: boolean }) and after awaiting resp.json() check result.errors; if errors exist and options?.allowErrors is not true, throw a descriptive Error including the serialized errors; also update gqlMutation to accept and forward the same options to gqlQuery so mutations benefit from the same behavior. This keeps the existing HTTP-level expect(resp.ok()) and makes callers opt-in to allow GraphQL errors.advanced/authentication.md (1)
101-108: Add explicitbaseURLguard in setup examplesThese examples assume
baseURLis always set. A missing value produces broken URLs likeundefined/loginand confusing setup failures.Suggested fix
async function globalSetup(config: FullConfig) { const { baseURL } = config.projects[0].use; + if (!baseURL) { + throw new Error("Missing `use.baseURL` in Playwright config"); + } const browser = await chromium.launch();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@advanced/authentication.md` around lines 101 - 108, The setup example in globalSetup reads baseURL from config.projects[0].use and then uses it in page.goto, which fails when baseURL is undefined; add an explicit guard after extracting const { baseURL } = config.projects[0].use to check for a truthy baseURL and throw or log a clear error (e.g., throw new Error("baseURL is not set in Playwright config; please set config.projects[0].use.baseURL")) so page.goto(`${baseURL}/login`) never receives "undefined"; ensure the guard references globalSetup, baseURL, and the page.goto call so maintainers can locate the fix.architecture/test-architecture.md (3)
332-337: Add language identifier to code fence for clarity.The code fence displays a file structure listing but lacks a language identifier.
📝 Suggested improvement
-``` +```text tests/e2e/inventory.spec.ts - manager creates item and sees it in listAs per coding guidelines (markdownlint MD040: fenced-code-language).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@architecture/test-architecture.md` around lines 332 - 337, The fenced code block showing the test list (starting with the line "tests/e2e/inventory.spec.ts") is missing a language identifier; update the opening fence to include a language tag (e.g., change ``` to ```text) so the block becomes "```text" and satisfies markdownlint MD040 while preserving the listed lines.
293-306: Add language identifier to code fence for clarity.The code fence displays a file structure listing but lacks a language identifier. Adding
textorplaintextimproves syntax highlighting and readability.📝 Suggested improvement
-``` +```text tests/api/inventory.spec.ts - creates item with valid data (201)As per coding guidelines (markdownlint MD040: fenced-code-language).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@architecture/test-architecture.md` around lines 293 - 306, The fenced code block that lists "tests/api/inventory.spec.ts" lacks a language identifier; update the opening fence to include a language (e.g., change ``` to ```text or ```plaintext) so markdownlint MD040 is satisfied and the snippet renders with proper highlighting/readability; locate the fence containing the lines starting with "tests/api/inventory.spec.ts" and modify the opening backticks accordingly.
312-326: Add language identifier to code fence for clarity.The code fence displays a file structure listing but lacks a language identifier.
📝 Suggested improvement
-``` +```text tests/components/InventoryForm.spec.tsx - shows validation errors on empty submitAs per coding guidelines (markdownlint MD040: fenced-code-language).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@architecture/test-architecture.md` around lines 312 - 326, The fenced code block showing the test file listings (containing lines like tests/components/InventoryForm.spec.tsx and tests/components/InventoryTable.spec.tsx) needs a language identifier to satisfy markdownlint MD040; update the opening fence from ``` to ```text (or another appropriate language) so the block becomes a labeled code fence and re-run linting to confirm the warning is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@advanced/authentication.md`:
- Around line 377-392: The test "bypass OAuth entirely via API session
injection" uses the isolated request fixture so cookies won't reach the browser;
update the test to call page.request.post(...) instead of request.post(...) and
remove the unused request parameter from the test signature so the API call uses
the page's shared APIRequestContext and the session cookies are available to
page.context().storageState and subsequent page.goto("/home").
- Around line 150-158: The worker-scoped fixture callback is incorrectly calling
test.info(); instead accept and destructure the workerInfo parameter in the
async fixture signature and replace test.info().parallelIndex with
workerInfo.parallelIndex where the username is filled (the block that calls
page.goto("/login") and page.getByLabel("Username").fill(...)); update the
fixture parameter list for the async ({ browser }, use) => to include workerInfo
and use workerInfo.parallelIndex to construct the worker-{index}@example.com
username.
In `@testing-patterns/forms-validation.md`:
- Around line 249-262: The test "form shows loading state during submission" has
a race because it asserts synchronous state changes; instead capture the submit
button once (e.g., const submit = page.getByRole("button", { name: /Submit
feedback|Submitting/ })) then after click await
expect(submit).toHaveText(/Submitting/); await expect(submit).toBeDisabled();
and only after that await expect(submit).toHaveText("Submit feedback"); await
expect(submit).toBeEnabled(); this uses Playwright's built-in waiting
(toHaveText/toBeDisabled/toBeEnabled) to reliably wait for the loading
transition rather than asserting immediately on two different locators.
---
Duplicate comments:
In `@infrastructure-ci-cd/other-providers.md`:
- Around line 251-252: The CircleCI orb declaration currently uses an unpinned
tag "circleci/node@latest" (under the orbs -> node key); replace it with a
specific semver-pinned orb like "circleci/node@<major.minor>" or
"circleci/node@<major.minor.patch>" (e.g., `@5.2` or `@5.2.1`) to ensure
reproducible builds and stability, and update any other occurrences of
"circleci/node@latest" in this file to the chosen pinned version.
In `@testing-patterns/visual-regression.md`:
- Around line 571-584: Update the guidance to recommend pinning exact matching
Playwright versions instead of using "latest": change the note around
package.json/@playwright/test to instruct pinning the npm dependency to a
specific version (e.g., "1.48.0") and the container image tag to the matching
Docker tag (e.g., v1.48.0-noble), and add a short "Best practice" sentence
advising to update both package.json and the container image together to
maintain reproducible renders.
---
Nitpick comments:
In `@advanced/authentication.md`:
- Around line 101-108: The setup example in globalSetup reads baseURL from
config.projects[0].use and then uses it in page.goto, which fails when baseURL
is undefined; add an explicit guard after extracting const { baseURL } =
config.projects[0].use to check for a truthy baseURL and throw or log a clear
error (e.g., throw new Error("baseURL is not set in Playwright config; please
set config.projects[0].use.baseURL")) so page.goto(`${baseURL}/login`) never
receives "undefined"; ensure the guard references globalSetup, baseURL, and the
page.goto call so maintainers can locate the fix.
In `@architecture/test-architecture.md`:
- Around line 332-337: The fenced code block showing the test list (starting
with the line "tests/e2e/inventory.spec.ts") is missing a language identifier;
update the opening fence to include a language tag (e.g., change ``` to ```text)
so the block becomes "```text" and satisfies markdownlint MD040 while preserving
the listed lines.
- Around line 293-306: The fenced code block that lists
"tests/api/inventory.spec.ts" lacks a language identifier; update the opening
fence to include a language (e.g., change ``` to ```text or ```plaintext) so
markdownlint MD040 is satisfied and the snippet renders with proper
highlighting/readability; locate the fence containing the lines starting with
"tests/api/inventory.spec.ts" and modify the opening backticks accordingly.
- Around line 312-326: The fenced code block showing the test file listings
(containing lines like tests/components/InventoryForm.spec.tsx and
tests/components/InventoryTable.spec.tsx) needs a language identifier to satisfy
markdownlint MD040; update the opening fence from ``` to ```text (or another
appropriate language) so the block becomes a labeled code fence and re-run
linting to confirm the warning is resolved.
In `@testing-patterns/graphql-testing.md`:
- Around line 217-240: Update the helper to detect GraphQL-level errors and fail
tests by default: modify gqlQuery to accept an optional options parameter (e.g.,
{ allowErrors?: boolean }) and after awaiting resp.json() check result.errors;
if errors exist and options?.allowErrors is not true, throw a descriptive Error
including the serialized errors; also update gqlMutation to accept and forward
the same options to gqlQuery so mutations benefit from the same behavior. This
keeps the existing HTTP-level expect(resp.ok()) and makes callers opt-in to
allow GraphQL errors.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
SKILL.mdadvanced/authentication.mdadvanced/mobile-testing.mdadvanced/network-advanced.mdarchitecture/pom-vs-fixtures.mdarchitecture/test-architecture.mdcore/assertions-waiting.mdcore/configuration.mdcore/fixtures-hooks.mdcore/page-object-model.mdcore/test-data.mddebugging/debugging.mdframeworks/angular.mdinfrastructure-ci-cd/docker.mdinfrastructure-ci-cd/github-actions.mdinfrastructure-ci-cd/gitlab.mdinfrastructure-ci-cd/other-providers.mdinfrastructure-ci-cd/parallel-sharding.mdinfrastructure-ci-cd/performance.mdinfrastructure-ci-cd/reporting.mdtesting-patterns/accessibility.mdtesting-patterns/canvas-webgl.mdtesting-patterns/forms-validation.mdtesting-patterns/graphql-testing.mdtesting-patterns/visual-regression.md
✅ Files skipped from review due to trivial changes (2)
- advanced/network-advanced.md
- infrastructure-ci-cd/parallel-sharding.md
🚧 Files skipped from review as they are similar to previous changes (10)
- infrastructure-ci-cd/performance.md
- testing-patterns/canvas-webgl.md
- testing-patterns/accessibility.md
- core/configuration.md
- core/page-object-model.md
- infrastructure-ci-cd/gitlab.md
- advanced/mobile-testing.md
- core/assertions-waiting.md
- frameworks/angular.md
- architecture/pom-vs-fixtures.md
Comment on lines
+150
to
+158
| async ({ browser }, use) => { | ||
| const context = await browser.newContext(); | ||
| const page = await context.newPage(); | ||
|
|
||
| await page.goto("/login"); | ||
| await page | ||
| .getByLabel("Username") | ||
| .fill(`worker-${test.info().parallelIndex}@example.com`); | ||
| await page.getByLabel("Password").fill("secretPass123"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Playwright worker-scoped fixture workerInfo parallelIndex API documentation
💡 Result:
In Playwright Test, worker-scoped fixtures receive a WorkerInfo object (not TestInfo). The API doc for workerInfo.parallelIndex is here: WorkerInfo → parallelIndex. (playwright.dev)
Key points from the docs:
workerInfo.parallelIndex(added in v1.10): the worker’s index from0toworkers - 1; workers running concurrently have differentparallelIndex. On worker restart, the new process keeps the sameparallelIndex. (playwright.dev)- Also available as
process.env.TEST_PARALLEL_INDEX. (playwright.dev) - Related:
workerInfo.workerIndexis a unique process id that changes on restart; also available asprocess.env.TEST_WORKER_INDEX. (playwright.dev) - Parallelism doc section explaining workerIndex vs parallelIndex: (playwright.dev)
- Worker-scoped fixtures overview (shows
workerInfousage in fixtures): (playwright.dev)
Citations:
- 1: https://playwright.dev/docs/api/class-workerinfo?utm_source=openai
- 2: https://playwright.dev/docs/api/class-workerinfo?utm_source=openai
- 3: https://playwright.dev/docs/api/class-workerinfo?utm_source=openai
- 4: https://playwright.dev/docs/api/class-workerinfo?utm_source=openai
- 5: https://playwright.dev/docs/next/test-parallel?utm_source=openai
- 6: https://playwright.dev/docs/test-fixtures?utm_source=openai
🏁 Script executed:
cat -n advanced/authentication.md | sed -n '140,165p'Repository: currents-dev/playwright-best-practices-skill
Length of output: 1053
Use workerInfo in worker-scoped fixture instead of test.info()
The fixture is worker-scoped (indicated by the square bracket on line 149), so it should receive workerInfo as a parameter. Accessing worker metadata via test.info().parallelIndex is incorrect—test.info() is for test-scoped context only. The correct approach is to destructure workerInfo from the fixture callback and use workerInfo.parallelIndex.
Suggested fix
- authenticatedContext: [
- async ({ browser }, use) => {
+ authenticatedContext: [
+ async ({ browser }, use, workerInfo) => {
const context = await browser.newContext();
const page = await context.newPage();
await page.goto("/login");
await page
.getByLabel("Username")
- .fill(`worker-${test.info().parallelIndex}@example.com`);
+ .fill(`worker-${workerInfo.parallelIndex}@example.com`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async ({ browser }, use) => { | |
| const context = await browser.newContext(); | |
| const page = await context.newPage(); | |
| await page.goto("/login"); | |
| await page | |
| .getByLabel("Username") | |
| .fill(`worker-${test.info().parallelIndex}@example.com`); | |
| await page.getByLabel("Password").fill("secretPass123"); | |
| async ({ browser }, use, workerInfo) => { | |
| const context = await browser.newContext(); | |
| const page = await context.newPage(); | |
| await page.goto("/login"); | |
| await page | |
| .getByLabel("Username") | |
| .fill(`worker-${workerInfo.parallelIndex}@example.com`); | |
| await page.getByLabel("Password").fill("secretPass123"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@advanced/authentication.md` around lines 150 - 158, The worker-scoped fixture
callback is incorrectly calling test.info(); instead accept and destructure the
workerInfo parameter in the async fixture signature and replace
test.info().parallelIndex with workerInfo.parallelIndex where the username is
filled (the block that calls page.goto("/login") and
page.getByLabel("Username").fill(...)); update the fixture parameter list for
the async ({ browser }, use) => to include workerInfo and use
workerInfo.parallelIndex to construct the worker-{index}@example.com username.
Comment on lines
+377
to
+392
| test("bypass OAuth entirely via API session injection", async ({ | ||
| page, | ||
| request, | ||
| }) => { | ||
| // Call a test-only endpoint that creates a session without OAuth | ||
| const response = await request.post("/api/test/create-session", { | ||
| data: { | ||
| email: "oauth-user@example.com", | ||
| provider: "provider", | ||
| role: "member", | ||
| }, | ||
| }); | ||
| expect(response.ok()).toBeTruthy(); | ||
|
|
||
| await page.context().storageState({ path: ".auth/oauth-user.json" }); | ||
| await page.goto("/home"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Playwright Test request fixture vs page.request cookie session state sharing
💡 Result:
In Playwright Test, these are different APIRequestContexts with different cookie jars:
-
requestfixture (test('…', async ({ request }) => …)):- It is a global/isolated
APIRequestContextcreated viaplaywright.request.newContext()(and disposed by the fixture). - Its cookies/session do not automatically share with your test’s
page/contextcookies. If you need sharing, you must pass/transferstorageStateyourself. [2]
- It is a global/isolated
-
page.request/context.request:- This
APIRequestContextis associated with the BrowserContext and shares cookie storage with it. - Requests sent via
page.requestautomatically include the browser context’s cookies, andSet-Cookieresponses will update the browser context cookies—so logging in via API makes the page logged in (and vice versa). [1][2]
- This
Practical guidance
- Want API calls to act as the same logged-in user as the UI (shared cookie session)? Use
page.request(orcontext.request). [1][2] - Want API calls that are isolated from the UI session (no cookie bleed between API and browser)? Use the
requestfixture (orplaywright.request.newContext()). [1][2]
Sources: Playwright APIRequestContext docs on cookie sharing and isolation, and Playwright “API testing” docs explaining request fixture vs context-associated requests. [1][2]
References:
[1] Playwright docs — APIRequestContext (cookie sharing for page.request / context.request, isolation via newContext) (playwright.dev)
[2] Playwright docs — API testing (“request fixture” uses newContext; “Context request vs global request”; storageState interoperability) (playwright.dev)
Citations:
- 1: https://playwright.dev/docs/api/class-apirequestcontext?utm_source=openai
- 2: https://playwright.dev/docs/next/api-testing?utm_source=openai
request fixture and page context have isolated cookies—use page.request instead
The request fixture creates an isolated APIRequestContext with its own cookie jar that doesn't automatically share with the page's browser context. Logging in via the test request fixture won't authenticate the subsequent page navigation, causing the test to silently fail.
Change request.post() to page.request.post() and remove the unused request parameter. This ensures API session cookies are stored in the shared browser context and available when the page navigates.
Suggested fix
-test("bypass OAuth entirely via API session injection", async ({
- page,
- request,
-}) => {
+test("bypass OAuth entirely via API session injection", async ({ page }) => {
// Call a test-only endpoint that creates a session without OAuth
- const response = await request.post("/api/test/create-session", {
+ const response = await page.request.post("/api/test/create-session", {
data: {
email: "oauth-user@example.com",
provider: "provider",
role: "member",
},
});
expect(response.ok()).toBeTruthy();
await page.context().storageState({ path: ".auth/oauth-user.json" });
await page.goto("/home");
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("bypass OAuth entirely via API session injection", async ({ | |
| page, | |
| request, | |
| }) => { | |
| // Call a test-only endpoint that creates a session without OAuth | |
| const response = await request.post("/api/test/create-session", { | |
| data: { | |
| email: "oauth-user@example.com", | |
| provider: "provider", | |
| role: "member", | |
| }, | |
| }); | |
| expect(response.ok()).toBeTruthy(); | |
| await page.context().storageState({ path: ".auth/oauth-user.json" }); | |
| await page.goto("/home"); | |
| test("bypass OAuth entirely via API session injection", async ({ page }) => { | |
| // Call a test-only endpoint that creates a session without OAuth | |
| const response = await page.request.post("/api/test/create-session", { | |
| data: { | |
| email: "oauth-user@example.com", | |
| provider: "provider", | |
| role: "member", | |
| }, | |
| }); | |
| expect(response.ok()).toBeTruthy(); | |
| await page.context().storageState({ path: ".auth/oauth-user.json" }); | |
| await page.goto("/home"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@advanced/authentication.md` around lines 377 - 392, The test "bypass OAuth
entirely via API session injection" uses the isolated request fixture so cookies
won't reach the browser; update the test to call page.request.post(...) instead
of request.post(...) and remove the unused request parameter from the test
signature so the API call uses the page's shared APIRequestContext and the
session cookies are available to page.context().storageState and subsequent
page.goto("/home").
Comment on lines
+249
to
+262
| test("form shows loading state during submission", async ({ page }) => { | ||
| await page.goto("/feedback"); | ||
|
|
||
| await page.getByLabel("Subject").fill("Bug report"); | ||
| await page.getByLabel("Email").fill("user@test.com"); | ||
| await page.getByLabel("Details").fill("Found an issue"); | ||
|
|
||
| await page.getByRole("button", { name: "Submit feedback" }).click(); | ||
|
|
||
| await expect(page.getByRole("button", { name: /Submitting/ })).toBeDisabled(); | ||
| await expect( | ||
| page.getByRole("button", { name: "Submit feedback" }) | ||
| ).toBeEnabled(); | ||
| }); |
There was a problem hiding this comment.
Race condition in loading state test.
The test expects the button to show "Submitting" text and be disabled, then immediately expects "Submit feedback" and enabled state, with no wait for the transition to complete. The loading state may finish before the first assertion runs, or the second may fail if loading is still in progress.
🔄 Proposed fix to properly wait for state transitions
test("form shows loading state during submission", async ({ page }) => {
await page.goto("/feedback");
await page.getByLabel("Subject").fill("Bug report");
await page.getByLabel("Email").fill("user@test.com");
await page.getByLabel("Details").fill("Found an issue");
+ const submitButton = page.getByRole("button", { name: /Submit/ });
+ await submitButton.click();
- await page.getByRole("button", { name: "Submit feedback" }).click();
- await expect(page.getByRole("button", { name: /Submitting/ })).toBeDisabled();
+ await expect(submitButton).toBeDisabled();
+ await expect(submitButton).toHaveText(/Submitting/);
+
+ // Wait for submission to complete
+ await page.waitForResponse("**/api/feedback");
+
- await expect(
- page.getByRole("button", { name: "Submit feedback" })
- ).toBeEnabled();
+ await expect(submitButton).toBeEnabled();
+ await expect(submitButton).toHaveText("Submit feedback");
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("form shows loading state during submission", async ({ page }) => { | |
| await page.goto("/feedback"); | |
| await page.getByLabel("Subject").fill("Bug report"); | |
| await page.getByLabel("Email").fill("user@test.com"); | |
| await page.getByLabel("Details").fill("Found an issue"); | |
| await page.getByRole("button", { name: "Submit feedback" }).click(); | |
| await expect(page.getByRole("button", { name: /Submitting/ })).toBeDisabled(); | |
| await expect( | |
| page.getByRole("button", { name: "Submit feedback" }) | |
| ).toBeEnabled(); | |
| }); | |
| test("form shows loading state during submission", async ({ page }) => { | |
| await page.goto("/feedback"); | |
| await page.getByLabel("Subject").fill("Bug report"); | |
| await page.getByLabel("Email").fill("user@test.com"); | |
| await page.getByLabel("Details").fill("Found an issue"); | |
| const submitButton = page.getByRole("button", { name: /Submit/ }); | |
| await submitButton.click(); | |
| await expect(submitButton).toBeDisabled(); | |
| await expect(submitButton).toHaveText(/Submitting/); | |
| // Wait for submission to complete | |
| await page.waitForResponse("**/api/feedback"); | |
| await expect(submitButton).toBeEnabled(); | |
| await expect(submitButton).toHaveText("Submit feedback"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testing-patterns/forms-validation.md` around lines 249 - 262, The test "form
shows loading state during submission" has a race because it asserts synchronous
state changes; instead capture the submit button once (e.g., const submit =
page.getByRole("button", { name: /Submit feedback|Submitting/ })) then after
click await expect(submit).toHaveText(/Submitting/); await
expect(submit).toBeDisabled(); and only after that await
expect(submit).toHaveText("Submit feedback"); await
expect(submit).toBeEnabled(); this uses Playwright's built-in waiting
(toHaveText/toBeDisabled/toBeEnabled) to reliably wait for the loading
transition rather than asserting immediately on two different locators.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit