feat: move skills into domain subdirectories#4
Merged
waltergalvao merged 8 commits intomainfrom Mar 13, 2026
Merged
Conversation
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR reorganizes documentation into directory-based categories, adds many new in-depth guides (auth, config, frameworks, CI/CD, testing patterns), renames/renumbers several topics, and normalizes internal cross-reference links to the new folder structure. 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)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
waltergalvao
previously 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)
Contributor
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>
Contributor
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
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
1-2:⚠️ Potential issue | 🟡 MinorRemove redundant code fence.
Lines 1-2 contain an empty code fence that appears redundant, as the ASCII art banner already has its own code fence starting at line 3.
📝 Proposed fix
-``` - ░█▀█░█░░░█▀█░█░█░█░█░█▀▄░▀█▀░█▀▀░█░█░▀█▀░░░█▀▄░█▀▀░█▀▀░▀█▀░░░█▀█░█▀▄░█▀█░█▀▀░▀█▀░▀█▀░█▀▀░█▀▀░█▀▀░🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 1 - 2, Remove the redundant empty code fence at the start of the README (the standalone triple backticks immediately preceding the ASCII art banner) so the banner's own code fence is the only one present; simply delete the extra opening and closing backticks that sit before the ASCII art block to avoid an empty fenced code block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@README.md`:
- Around line 1-2: Remove the redundant empty code fence at the start of the
README (the standalone triple backticks immediately preceding the ASCII art
banner) so the banner's own code fence is the only one present; simply delete
the extra opening and closing backticks that sit before the ASCII art block to
avoid an empty fenced code block.
- Clarified usage scenarios for per-worker authentication in `authentication.md`. - Enhanced OAuth testing instructions with detailed flow and session injection methods. - Updated Node.js version from 20 to 22 in multiple CI/CD configurations. - Minor formatting adjustments in test coverage documentation for consistency. - Improved form validation tests to handle loading states more effectively.
Contributor
Author
|
@waltergalvao address all the comments and fixed links |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SKILL.md`:
- Around line 192-291: Add a language identifier to the fenced code block that
begins with the line "What are you doing?" (the triple-backtick block in
SKILL.md) to satisfy markdownlint MD040; change the opening fence from ``` to
```text so the decision-tree is explicitly marked as plain text and the linter
stops flagging the block.
In `@testing-patterns/forms-validation.md`:
- Around line 64-67: The test currently awaits the first network response via
page.waitForResponse (responsePromise) after calling
streetField.pressSequentially, which is flaky because pressSequentially triggers
multiple requests; instead, after calling streetField.pressSequentially("456
Elm", { delay: 50 }) wait for the UI suggestion to appear using a DOM/web-first
assertion (e.g., wait for the specific suggestion element or role/text to be
visible) rather than awaiting responsePromise, and remove or stop relying on
page.waitForResponse("**/api/address-lookup*"); reference the symbols
page.waitForResponse, pressSequentially, responsePromise and streetField to
locate and update the code.
- Around line 515-519: The current code calls evaluate and reads
getAttribute("contenteditable"), which returns literal attribute strings or null
and misses inherited editability; change the evaluate callback used on
page.getByTestId("editor") to return the element's isContentEditable property
(el.isContentEditable) so isContentEditable is a proper boolean reflecting
effective editability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14a481a9-499f-4b77-900d-6c0a0279b2c6
📒 Files selected for processing (5)
SKILL.mdadvanced/authentication.mdinfrastructure-ci-cd/ci-cd.mdinfrastructure-ci-cd/test-coverage.mdtesting-patterns/forms-validation.md
✅ Files skipped from review due to trivial changes (1)
- advanced/authentication.md
Comment on lines
192
to
291
| ``` | ||
| What are you doing? | ||
| │ | ||
| ├─ Writing a new test? | ||
| │ ├─ E2E test → test-organization.md, locators.md, assertions-waiting.md | ||
| │ ├─ Component test → component-testing.md | ||
| │ ├─ API test → test-organization.md, assertions-waiting.md | ||
| │ ├─ Visual/canvas test → canvas-webgl.md, test-organization.md | ||
| │ ├─ Accessibility test → accessibility.md | ||
| │ ├─ Mobile/responsive test → mobile-testing.md | ||
| │ ├─ i18n/locale test → i18n.md | ||
| │ ├─ Electron app test → electron.md | ||
| │ ├─ Browser extension test → browser-extensions.md | ||
| │ └─ Multi-user test → multi-user.md | ||
| │ ├─ E2E test → core/test-suite-structure.md, core/locators.md, core/assertions-waiting.md | ||
| │ ├─ Component test → testing-patterns/component-testing.md | ||
| │ ├─ API test → testing-patterns/api-testing.md, core/test-suite-structure.md | ||
| │ ├─ GraphQL test → testing-patterns/graphql-testing.md | ||
| │ ├─ Visual regression → testing-patterns/visual-regression.md | ||
| │ ├─ Visual/canvas test → testing-patterns/canvas-webgl.md, core/test-suite-structure.md | ||
| │ ├─ Accessibility test → testing-patterns/accessibility.md | ||
| │ ├─ Mobile/responsive test → advanced/mobile-testing.md | ||
| │ ├─ i18n/locale test → testing-patterns/i18n.md | ||
| │ ├─ Electron app test → testing-patterns/electron.md | ||
| │ ├─ Browser extension test → testing-patterns/browser-extensions.md | ||
| │ ├─ Multi-user test → advanced/multi-user.md | ||
| │ ├─ Form validation test → testing-patterns/forms-validation.md | ||
| │ └─ Drag and drop test → testing-patterns/drag-drop.md | ||
| │ | ||
| ├─ Testing specific features? | ||
| │ ├─ File upload/download → file-operations.md | ||
| │ ├─ Date/time dependent → clock-mocking.md | ||
| │ ├─ WebSocket/real-time → websockets.md | ||
| │ ├─ Geolocation/permissions → browser-apis.md | ||
| │ ├─ OAuth/SSO mocking → third-party.md, multi-context.md | ||
| │ ├─ Payments/email/SMS → third-party.md | ||
| │ ├─ iFrames → iframes.md | ||
| │ ├─ Canvas/WebGL/charts → canvas-webgl.md | ||
| │ ├─ Service workers/PWA → service-workers.md | ||
| │ ├─ i18n/localization → i18n.md | ||
| │ ├─ Security (XSS, CSRF) → security-testing.md | ||
| │ └─ Performance/Web Vitals → performance-testing.md | ||
| │ ├─ File upload/download → testing-patterns/file-operations.md, testing-patterns/file-upload-download.md | ||
| │ ├─ Date/time dependent → advanced/clock-mocking.md | ||
| │ ├─ WebSocket/real-time → browser-apis/websockets.md | ||
| │ ├─ Geolocation/permissions → browser-apis/browser-apis.md | ||
| │ ├─ OAuth/SSO mocking → advanced/third-party.md, advanced/multi-context.md | ||
| │ ├─ Payments/email/SMS → advanced/third-party.md | ||
| │ ├─ iFrames → browser-apis/iframes.md | ||
| │ ├─ Canvas/WebGL/charts → testing-patterns/canvas-webgl.md | ||
| │ ├─ Service workers/PWA → browser-apis/service-workers.md | ||
| │ ├─ i18n/localization → testing-patterns/i18n.md | ||
| │ ├─ Security (XSS, CSRF) → testing-patterns/security-testing.md | ||
| │ └─ Performance/Web Vitals → testing-patterns/performance-testing.md | ||
| │ | ||
| ├─ Architecture decisions? | ||
| │ ├─ POM vs fixtures → architecture/pom-vs-fixtures.md | ||
| │ ├─ Test type selection → architecture/test-architecture.md | ||
| │ ├─ Mock vs real services → architecture/when-to-mock.md | ||
| │ └─ Test suite structure → core/test-suite-structure.md | ||
| │ | ||
| ├─ Framework-specific testing? | ||
| │ ├─ React app → frameworks/react.md | ||
| │ ├─ Angular app → frameworks/angular.md | ||
| │ ├─ Vue/Nuxt app → frameworks/vue.md | ||
| │ └─ Next.js app → frameworks/nextjs.md | ||
| │ | ||
| ├─ Authentication testing? | ||
| │ ├─ Basic auth patterns → advanced/authentication.md | ||
| │ └─ Complex flows (MFA, reset) → advanced/authentication-flows.md | ||
| │ | ||
| ├─ Test is failing/flaky? | ||
| │ ├─ Flaky test investigation → flaky-tests.md | ||
| │ ├─ Element not found → locators.md, debugging.md | ||
| │ ├─ Timeout issues → assertions-waiting.md, debugging.md | ||
| │ ├─ Race conditions → flaky-tests.md, debugging.md | ||
| │ ├─ Flaky only with multiple workers → flaky-tests.md, performance.md | ||
| │ ├─ State leak / isolation → flaky-tests.md, fixtures-hooks.md | ||
| │ ├─ Console/JS errors → console-errors.md, debugging.md | ||
| │ └─ General debugging → debugging.md | ||
| │ ├─ Flaky test investigation → debugging/flaky-tests.md | ||
| │ ├─ Element not found → core/locators.md, debugging/debugging.md | ||
| │ ├─ Timeout issues → core/assertions-waiting.md, debugging/debugging.md | ||
| │ ├─ Race conditions → debugging/flaky-tests.md, debugging/debugging.md | ||
| │ ├─ Flaky only with multiple workers → debugging/flaky-tests.md, infrastructure-ci-cd/performance.md | ||
| │ ├─ State leak / isolation → debugging/flaky-tests.md, core/fixtures-hooks.md | ||
| │ ├─ Console/JS errors → debugging/console-errors.md, debugging/debugging.md | ||
| │ └─ General debugging → debugging/debugging.md | ||
| │ | ||
| ├─ Testing error scenarios? | ||
| │ ├─ Network failures → error-testing.md, network-advanced.md | ||
| │ ├─ Offline (unexpected) → error-testing.md | ||
| │ ├─ Offline-first/PWA → service-workers.md | ||
| │ ├─ Error boundaries → error-testing.md | ||
| │ └─ Form validation → error-testing.md | ||
| │ ├─ Network failures → debugging/error-testing.md, advanced/network-advanced.md | ||
| │ ├─ Offline (unexpected) → debugging/error-testing.md | ||
| │ ├─ Offline-first/PWA → browser-apis/service-workers.md | ||
| │ ├─ Error boundaries → debugging/error-testing.md | ||
| │ └─ Form validation → testing-patterns/forms-validation.md, debugging/error-testing.md | ||
| │ | ||
| ├─ Refactoring existing code? | ||
| │ ├─ Implementing POM → page-object-model.md | ||
| │ ├─ Improving selectors → locators.md | ||
| │ ├─ Extracting fixtures → fixtures-hooks.md | ||
| │ └─ Creating data factories → test-data.md | ||
| │ ├─ Implementing POM → core/page-object-model.md | ||
| │ ├─ Improving selectors → core/locators.md | ||
| │ ├─ Extracting fixtures → core/fixtures-hooks.md | ||
| │ ├─ Creating data factories → core/test-data.md | ||
| │ └─ Configuration setup → core/configuration.md | ||
| │ | ||
| ├─ Setting up infrastructure? | ||
| │ ├─ CI/CD → ci-cd.md | ||
| │ ├─ Global setup/teardown → global-setup.md | ||
| │ ├─ Project dependencies → projects-dependencies.md | ||
| │ ├─ Test performance → performance.md | ||
| │ ├─ Test coverage → test-coverage.md | ||
| │ └─ Project config → test-organization.md, projects-dependencies.md | ||
| │ ├─ CI/CD → infrastructure-ci-cd/ci-cd.md | ||
| │ ├─ GitHub Actions → infrastructure-ci-cd/github-actions.md | ||
| │ ├─ GitLab CI → infrastructure-ci-cd/gitlab.md | ||
| │ ├─ Other CI providers → infrastructure-ci-cd/other-providers.md | ||
| │ ├─ Docker/containers → infrastructure-ci-cd/docker.md | ||
| │ ├─ Sharding/parallel → infrastructure-ci-cd/parallel-sharding.md | ||
| │ ├─ Reporting/artifacts → infrastructure-ci-cd/reporting.md | ||
| │ ├─ Global setup/teardown → core/global-setup.md | ||
| │ ├─ Project dependencies → core/projects-dependencies.md | ||
| │ ├─ Test performance → infrastructure-ci-cd/performance.md | ||
| │ ├─ Test coverage → infrastructure-ci-cd/test-coverage.md | ||
| │ └─ Project config → core/configuration.md, core/projects-dependencies.md | ||
| │ | ||
| ├─ Organizing tests? | ||
| │ ├─ Skip/fixme/slow tests → core/annotations.md | ||
| │ ├─ Test tags (@smoke, @fast) → core/test-tags.md | ||
| │ ├─ Filtering tests (--grep) → core/test-tags.md | ||
| │ ├─ Test steps → core/annotations.md | ||
| │ └─ Conditional execution → core/annotations.md | ||
| │ | ||
| └─ Organizing tests? | ||
| ├─ Skip/fixme/slow tests → annotations.md | ||
| ├─ Test steps → annotations.md | ||
| └─ Conditional execution → annotations.md | ||
| └─ Running subset of tests? | ||
| ├─ By tag (@smoke, @critical) → core/test-tags.md | ||
| ├─ Exclude slow/flaky tests → core/test-tags.md | ||
| ├─ PR vs nightly tests → core/test-tags.md, infrastructure-ci-cd/ci-cd.md | ||
| └─ Project-specific filtering → core/test-tags.md, core/configuration.md | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the fenced decision-tree block.
Line 192 opens a fenced code block without a language, which matches the reported markdownlint MD040 warning and can keep docs linting red. text fits this block well.
Suggested fix
-```
+```text
What are you doing?
│
├─ Writing a new test?
│ ├─ E2E test → core/test-suite-structure.md, core/locators.md, core/assertions-waiting.md
│ ├─ Component test → testing-patterns/component-testing.md
│ ├─ API test → testing-patterns/api-testing.md, core/test-suite-structure.md
│ ├─ GraphQL test → testing-patterns/graphql-testing.md
│ ├─ Visual regression → testing-patterns/visual-regression.md
│ ├─ Visual/canvas test → testing-patterns/canvas-webgl.md, core/test-suite-structure.md
│ ├─ Accessibility test → testing-patterns/accessibility.md
│ ├─ Mobile/responsive test → advanced/mobile-testing.md
│ ├─ i18n/locale test → testing-patterns/i18n.md
│ ├─ Electron app test → testing-patterns/electron.md
│ ├─ Browser extension test → testing-patterns/browser-extensions.md
│ ├─ Multi-user test → advanced/multi-user.md
│ ├─ Form validation test → testing-patterns/forms-validation.md
│ └─ Drag and drop test → testing-patterns/drag-drop.md
│
├─ Testing specific features?
│ ├─ File upload/download → testing-patterns/file-operations.md, testing-patterns/file-upload-download.md
│ ├─ Date/time dependent → advanced/clock-mocking.md
│ ├─ WebSocket/real-time → browser-apis/websockets.md
│ ├─ Geolocation/permissions → browser-apis/browser-apis.md
│ ├─ OAuth/SSO mocking → advanced/third-party.md, advanced/multi-context.md
│ ├─ Payments/email/SMS → advanced/third-party.md
│ ├─ iFrames → browser-apis/iframes.md
│ ├─ Canvas/WebGL/charts → testing-patterns/canvas-webgl.md
│ ├─ Service workers/PWA → browser-apis/service-workers.md
│ ├─ i18n/localization → testing-patterns/i18n.md
│ ├─ Security (XSS, CSRF) → testing-patterns/security-testing.md
│ └─ Performance/Web Vitals → testing-patterns/performance-testing.md
│
├─ Architecture decisions?
│ ├─ POM vs fixtures → architecture/pom-vs-fixtures.md
│ ├─ Test type selection → architecture/test-architecture.md
│ ├─ Mock vs real services → architecture/when-to-mock.md
│ └─ Test suite structure → core/test-suite-structure.md
│
├─ Framework-specific testing?
│ ├─ React app → frameworks/react.md
│ ├─ Angular app → frameworks/angular.md
│ ├─ Vue/Nuxt app → frameworks/vue.md
│ └─ Next.js app → frameworks/nextjs.md
│
├─ Authentication testing?
│ ├─ Basic auth patterns → advanced/authentication.md
│ └─ Complex flows (MFA, reset) → advanced/authentication-flows.md
│
├─ Test is failing/flaky?
│ ├─ Flaky test investigation → debugging/flaky-tests.md
│ ├─ Element not found → core/locators.md, debugging/debugging.md
│ ├─ Timeout issues → core/assertions-waiting.md, debugging/debugging.md
│ ├─ Race conditions → debugging/flaky-tests.md, debugging/debugging.md
│ ├─ Flaky only with multiple workers → debugging/flaky-tests.md, infrastructure-ci-cd/performance.md
│ ├─ State leak / isolation → debugging/flaky-tests.md, core/fixtures-hooks.md
│ ├─ Console/JS errors → debugging/console-errors.md, debugging/debugging.md
│ └─ General debugging → debugging/debugging.md
│
├─ Testing error scenarios?
│ ├─ Network failures → debugging/error-testing.md, advanced/network-advanced.md
│ ├─ Offline (unexpected) → debugging/error-testing.md
│ ├─ Offline-first/PWA → browser-apis/service-workers.md
│ ├─ Error boundaries → debugging/error-testing.md
│ └─ Form validation → testing-patterns/forms-validation.md, debugging/error-testing.md
│
├─ Refactoring existing code?
│ ├─ Implementing POM → core/page-object-model.md
│ ├─ Improving selectors → core/locators.md
│ ├─ Extracting fixtures → core/fixtures-hooks.md
│ ├─ Creating data factories → core/test-data.md
│ └─ Configuration setup → core/configuration.md
│
├─ Setting up infrastructure?
│ ├─ CI/CD → infrastructure-ci-cd/ci-cd.md
│ ├─ GitHub Actions → infrastructure-ci-cd/github-actions.md
│ ├─ GitLab CI → infrastructure-ci-cd/gitlab.md
│ ├─ Other CI providers → infrastructure-ci-cd/other-providers.md
│ ├─ Docker/containers → infrastructure-ci-cd/docker.md
│ ├─ Sharding/parallel → infrastructure-ci-cd/parallel-sharding.md
│ ├─ Reporting/artifacts → infrastructure-ci-cd/reporting.md
│ ├─ Global setup/teardown → core/global-setup.md
│ ├─ Project dependencies → core/projects-dependencies.md
│ ├─ Test performance → infrastructure-ci-cd/performance.md
│ ├─ Test coverage → infrastructure-ci-cd/test-coverage.md
│ └─ Project config → core/configuration.md, core/projects-dependencies.md
│
├─ Organizing tests?
│ ├─ Skip/fixme/slow tests → core/annotations.md
│ ├─ Test tags (`@smoke`, `@fast`) → core/test-tags.md
│ ├─ Filtering tests (--grep) → core/test-tags.md
│ ├─ Test steps → core/annotations.md
│ └─ Conditional execution → core/annotations.md
│
└─ Running subset of tests?
├─ By tag (`@smoke`, `@critical`) → core/test-tags.md
├─ Exclude slow/flaky tests → core/test-tags.md
├─ PR vs nightly tests → core/test-tags.md, infrastructure-ci-cd/ci-cd.md
└─ Project-specific filtering → core/test-tags.md, core/configuration.md</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 192-192: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @SKILL.md around lines 192 - 291, Add a language identifier to the fenced
code block that begins with the line "What are you doing?" (the triple-backtick
block in SKILL.md) to satisfy markdownlint MD040; change the opening fence from
totext so the decision-tree is explicitly marked as plain text and the
linter stops flagging the block.
</details>
<!-- fingerprinting:phantom:triton:grasshopper -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+64
to
+67
| const responsePromise = page.waitForResponse("**/api/address-lookup*"); | ||
| await streetField.pressSequentially("456 Elm", { delay: 50 }); | ||
|
|
||
| await responsePromise; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Playwright's page.waitForResponse("**/api/address-lookup*") resolve on the first matching response, and what do the Playwright docs recommend when typing triggers multiple network requests?
💡 Result:
-
Yes.
page.waitForResponse('**/api/address-lookup*')resolves with the first Response event whose URL matches that glob pattern (i.e., the first “matched response” that occurs after you start waiting). Playwright describeswaitForResponseas “returns the matched response” and points to the general “waiting for event” behavior, which resolves when an event fires (optionally when a predicate first becomes truthy). [1], [2] -
When typing triggers multiple requests, Playwright’s docs recommend narrowing what you wait for and/or waiting on the UI instead of “all network”:
- Use a predicate to disambiguate (e.g., match on
url(),status(), and/orrequest().method()), not just a broad URL glob. [1] - Start the wait before the action that triggers the requests (create the promise, then type/click), to avoid races. [1], [3]
- Prefer web-first assertions (wait for the UI result of the typing, e.g., suggestion list updated/visible) where possible. [4]
- Use a predicate to disambiguate (e.g., match on
Sources:
[1] Playwright API: page.waitForResponse (class Page)
[2] Playwright Docs: Events / “Waiting for event” (predicate resolves when it becomes truthy)
[3] Playwright Docs: Network guide (waitForResponse pattern: start waiting, then trigger)
[4] Playwright Docs: Best Practices (web-first assertions)
Wait for the UI result, not the first matching response.
page.waitForResponse("**/api/address-lookup*") resolves on the first API response. When pressSequentially() types multiple characters, it can trigger several requests; the code will advance before suggestions for the final value are rendered, making the example flaky. Use web-first assertions instead—wait for the option to appear in the DOM.
Suggested doc fix
- const responsePromise = page.waitForResponse("**/api/address-lookup*");
await streetField.pressSequentially("456 Elm", { delay: 50 });
-
- await responsePromise;
-
- await page.getByRole("option", { name: /456 Elm St/ }).click();
+ const suggestion = page.getByRole("option", { name: /456 Elm St/ });
+ await expect(suggestion).toBeVisible();
+ await suggestion.click();🤖 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 64 - 67, The test
currently awaits the first network response via page.waitForResponse
(responsePromise) after calling streetField.pressSequentially, which is flaky
because pressSequentially triggers multiple requests; instead, after calling
streetField.pressSequentially("456 Elm", { delay: 50 }) wait for the UI
suggestion to appear using a DOM/web-first assertion (e.g., wait for the
specific suggestion element or role/text to be visible) rather than awaiting
responsePromise, and remove or stop relying on
page.waitForResponse("**/api/address-lookup*"); reference the symbols
page.waitForResponse, pressSequentially, responsePromise and streetField to
locate and update the code.
Comment on lines
+515
to
+519
| const isContentEditable = await page | ||
| .getByTestId("editor") | ||
| .evaluate((el) => el.getAttribute("contenteditable")); | ||
|
|
||
| if (isContentEditable) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the difference between HTMLElement.isContentEditable and getAttribute("contenteditable"), especially for contenteditable="" and inherited editability?
💡 Result:
What each one means
el.getAttribute("contenteditable")returns the element’s literal markup attribute value (a string) ornullif the attribute is missing. It does not account for inheritance ordocument.designMode. [1]el.isContentEditablereturns a boolean for the element’s effective editability:trueif the element is an editing host or otherwise editable,falseotherwise (this reflects inheritance anddesignMode). [3][4]
contenteditable="" (empty string)
In HTML, contenteditable is an enumerated attribute where "true" or the empty string means editable. So:
el.getAttribute("contenteditable")→""(empty string)el.isContentEditable→true[2][4]
Also, writing <div contenteditable> (no value) is treated the same as the empty string. [2]
Inherited editability (attribute missing/invalid)
If contenteditable is missing (or invalid), its state is inherited from the parent (per HTML’s contenteditable rules). [2]
So if a parent is editable:
<div contenteditable="true">
<span id="x"></span>
</div>x.getAttribute("contenteditable")→null(it’s not set on the<span>) [1]x.isContentEditable→true(effective/inherited editability) [2][3][4]
Related: el.contentEditable vs getAttribute(...)
If you actually want a string state that corresponds to the contenteditable mode, use el.contentEditable: it returns "true", "false", "plaintext-only", or "inherit" (when the attribute isn’t explicitly set / is removed). [3]
Use isContentEditable instead of getAttribute("contenteditable").
getAttribute("contenteditable") only returns the literal attribute value ("" or "true") and returns null when the attribute is missing, even if the element inherits editability from a parent. HTMLElement.isContentEditable returns a boolean reflecting the element's effective editability, including inherited state.
Suggested doc fix
const isContentEditable = await page
.getByTestId("editor")
- .evaluate((el) => el.getAttribute("contenteditable"));
+ .evaluate((el) => (el as HTMLElement).isContentEditable);📝 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
| const isContentEditable = await page | |
| .getByTestId("editor") | |
| .evaluate((el) => el.getAttribute("contenteditable")); | |
| if (isContentEditable) { | |
| const isContentEditable = await page | |
| .getByTestId("editor") | |
| .evaluate((el) => (el as HTMLElement).isContentEditable); | |
| if (isContentEditable) { |
🤖 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 515 - 519, The current
code calls evaluate and reads getAttribute("contenteditable"), which returns
literal attribute strings or null and misses inherited editability; change the
evaluate callback used on page.getByTestId("editor") to return the element's
isContentEditable property (el.isContentEditable) so isContentEditable is a
proper boolean reflecting effective editability.
waltergalvao
approved these changes
Mar 13, 2026
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