diff --git a/.github/workflows/pr-code-quality.md b/.github/workflows/pr-code-quality.md index 1da265d3..c56ac451 100644 --- a/.github/workflows/pr-code-quality.md +++ b/.github/workflows/pr-code-quality.md @@ -8,6 +8,7 @@ on: - reopened - synchronize - ready_for_review + - review_requested push: branches: - main @@ -73,11 +74,28 @@ You have access to: (lint, unit/component tests, browser tests, accessibility, unused-code checks, benchmarks) when available. - Existing reviews and review comments on the PR. +### Required CI jobs + +The `ci` workflow (`continuous-integration.yml`) defines these required jobs. +Always check **all** of them before submitting the final review: + +| Job | Name | Description | +|-----|------|-------------| +| `lint` | 🔠 Lint project | Linting and formatting | +| `unit` | 🧪 Unit tests | Vitest unit tests with coverage | +| `test` | 🧪 Component tests | Nuxt component tests (Playwright browser) | +| `browser` | 🖥️ Browser tests | E2E browser tests with HTML validation | +| `benchmark` | ⚡ Benchmarks | CodSpeed performance benchmarks | +| `a11y` (dark) | ♿ Accessibility audit (dark) | Lighthouse accessibility audit in dark mode | +| `a11y` (light) | ♿ Accessibility audit (light) | Lighthouse accessibility audit in light mode | +| `knip` | 🧹 Unused code check | Knip unused code detection | + Use the GitHub tools to fetch this information instead of guessing: 1. Fetch the pull request and list of changed files. 2. Fetch the combined status and check runs for the HEAD commit of the PR. -3. Optionally fetch the latest workflow runs for the `ci` workflow on this commit to understand test coverage. +3. Fetch the latest workflow runs for the `ci` workflow on this commit to retrieve + the status of **every required job** listed above. Do **not** re-run heavy checks yourself; rely on the existing CI signals. @@ -94,11 +112,12 @@ When the workflow is triggered by a `pull_request` event: - Test files and configuration changes. 2. **Read CI and checks** - - Check whether: - - Lint and formatting jobs have passed. - - Unit/component/browser tests have passed. - - Accessibility and unused-code checks have passed (when they ran). - - If some checks are missing or skipped, mention this explicitly in your summary. + - Retrieve the status of **every required CI job** from the table above. + - For each job, determine whether it is: ✅ passed, ❌ failed, ⏳ in progress, or ⏭️ skipped. + - If **any required job is still in progress**, note this in your summary and + use `COMMENT` instead of `APPROVE` or `REQUEST_CHANGES` — do not make a final + verdict until all required checks have completed. + - If some checks are missing, skipped, or not yet started, mention this explicitly. 3. **Code review** - For each significant issue you find, create a **line-specific comment** @@ -121,20 +140,83 @@ When the workflow is triggered by a `pull_request` event: - In this case, provide a clear, structured summary in the review `body` with a checklist of what must be fixed. - `event: "APPROVE"` when: - - CI checks relevant to these changes have passed, **and** + - **All required CI jobs have completed and passed**, **and** - You did not find any blocking issues, - Minor nitpicks (style, micro-optimizations) are already commented as suggestions. - The review body can be empty, or a short positive summary. - `event: "COMMENT"` when: - The PR is still a draft, or + - One or more required CI jobs are still in progress (not yet finished), or - You only have non-blocking feedback and prefer not to block merge. - When you request changes, always provide **specific guidance** on how to fix them. - -5. **Tone and style** + - **Never approve if any required CI job has failed or is still running.** + +5. **CI summary report** + - At the end of your review body, always include a **CI Summary** section + listing the status of every required CI job. + - **Always wrap the CI Summary in a collapsed section** so it doesn’t dominate the review: + + ```markdown +
+ 📋 CI Summary + + | Job | Status | + |-----|--------| + | 🔠 Lint | ✅ Passed | + | 🧪 Unit tests | ✅ Passed | + | 🧪 Component tests | ✅ Passed | + | 🖥️ Browser tests | ✅ Passed | + | ⚡ Benchmarks | ✅ Passed | + | ♿ A11y (dark) | ✅ Passed | + | ♿ A11y (light) | ✅ Passed | + | 🧹 Unused code | ✅ Passed | + +
+ ``` + + - Replace the status with the actual result for each job (✅ Passed, ❌ Failed, ⏳ Running, ⏭️ Skipped, ❓ Not found). + - If a job failed, include a brief note of what went wrong (e.g., "lint errors in `file.ts`") when the information is available from the check run. + +6. **Tone and style** - Be concise, constructive and respectful. - Avoid generic feedback like “improve code quality”; always tie comments to lines and concrete improvements. - Prefer “Consider … because …” over imperative language. +7. **Use collapsed sections for long feedback** + - When a review comment or the review body covers **multiple topics or categories**, + wrap each topic in a collapsible `
` block so the comment stays scannable. + - Always leave a **short visible summary** above the collapsed sections so the author + can understand the overall picture at a glance without expanding anything. + - Use this HTML pattern: + + ```markdown + **Short visible summary of the feedback.** + +
+ 🔍 Accessibility concerns (2 items) + + Detailed explanation of accessibility issues … + +
+ +
+ ⚡ Performance notes + + Detailed explanation of performance observations … + +
+ ``` + + - Apply collapsed sections in these situations: + - **Inline review comments** that discuss more than one aspect of a code change + (e.g., accessibility + performance + pattern consistency in the same file). + - **The review body** when you have feedback across multiple categories + (group by: bugs, accessibility, performance, style/patterns). + - **The CI Summary** table — always wrap it in a collapsed section so it doesn’t + dominate the review body. + - Keep each `` line short and descriptive, with an emoji prefix matching the + category (♿ accessibility, ⚡ performance, 🐛 bugs, 🧹 style, 🔒 security). + ## Behavior on push events (no pull request) When the workflow is triggered by a `push` event (e.g. direct push to `main` or `release`):