Update coding guidelines in AGENTS.md#5766
Conversation
Added guidelines for test cases, code coverage, and deprecation notes.
compulim
requested review from
a-b-r-o-w-n,
cwhitten,
srinaath and
tdurnford
as code owners
Add guidelines for handling snapshots in tests.
There was a problem hiding this comment.
Pull request overview
Updates AGENTS.md to add/expand contributor-facing coding guidelines around repository setup, testing expectations (including coverage), React component conventions, and snapshot update workflow.
Changes:
- Add a pointer to the contributing/setup documentation.
- Add guidelines for tests, code coverage targets, and deprecation dating.
- Restructure the React section and add snapshot-related testing guidance.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ### General | ||
|
|
||
| - Read `docs/CONTRIBUTING.md` for details on setting up the repository for development |
There was a problem hiding this comment.
docs/CONTRIBUTING.md does not exist in this repo (the contributing guide is under .github/CONTRIBUTING.md). Update this link/path so new contributors are not sent to a missing document.
| - Read `docs/CONTRIBUTING.md` for details on setting up the repository for development | |
| - Read `.github/CONTRIBUTING.md` for details on setting up the repository for development |
| - The only exception is `id`, e.g. `getId()` over `getID()` | ||
| - Use fewer shorthands, only allow `min`, `max`, `num` | ||
| - All new/changed production code must have test cases, look at `__tests__/html2/**` | ||
| - Code coverage for new/changed code should reach 80% |
There was a problem hiding this comment.
The new “80% code coverage for new/changed code” guideline is hard to apply as written: the repo currently collects coverage but does not define a coverageThreshold, and CI prints overall lcov summaries rather than per-diff coverage. Consider either (1) documenting exactly how to measure this in CI for a PR, or (2) changing the guideline to an enforceable/measurable target (e.g., overall project thresholds or specific packages).
| - Code coverage for new/changed code should reach 80% | |
| - Code coverage is measured using the lcov summary printed in CI for `npm test -- --coverage` | |
| - For each PR, overall project line coverage reported in that summary must be at least 80% | |
| - Do not merge PRs that reduce overall line coverage, unless explicitly approved by a maintainer and documented in the PR description | |
| - When adding substantial new/changed production code, aim for at least 80% line coverage for the affected files (you can verify this locally with `npm test -- --coverage` and inspecting the per-file report) |
| - Use fewer shorthands, only allow `min`, `max`, `num` | ||
| - All new/changed production code must have test cases, look at `__tests__/html2/**` | ||
| - Code coverage for new/changed code should reach 80% | ||
| - Deprecation notes should mark the date as 2 years from now |
There was a problem hiding this comment.
“Deprecation notes should mark the date as 2 years from now” is ambiguous and doesn’t match the format used elsewhere (e.g., “will be removed on or after YYYY-MM-DD”). Consider specifying the expected wording/format and what “now” is anchored to (feature introduction date vs. PR merge date) so dates are consistent across the codebase.
| - Deprecation notes should mark the date as 2 years from now | |
| - Deprecation notes must use the wording `will be removed on or after YYYY-MM-DD`, where the date is 2 years after the PR merge date. |
| ### React template | ||
| ### React | ||
|
|
||
| - Always add `displayName` |
There was a problem hiding this comment.
The new rule “Always add displayName” appears inconsistent with existing React components in the repo (e.g., packages/component/src/Activity/Bubble.tsx exports memo(Bubble) without setting Bubble.displayName). Consider narrowing this to cases where displayName would otherwise be lost/unclear (e.g., anonymous functions, forwardRef, HOCs), or rephrasing as a recommendation rather than an absolute rule.
| - Always add `displayName` | |
| - For components where the inferred name would be lost or unclear (e.g. anonymous functions, `forwardRef`, HOCs such as `memo`), explicitly set `displayName` to aid debugging |
| - MUST NOT modify/update any existing snapshots, let human review the test failures, they could be failing legitimately | ||
| - MUST NOT use `-u` to update snapshots, delete the snapshots and rerun `npm test` instead |
There was a problem hiding this comment.
The snapshot guidance is internally inconsistent: “MUST NOT modify/update snapshots” but also “delete the snapshots and rerun” (which effectively regenerates them). This also doesn’t align with .github/CONTRIBUTING.md, which instructs taking snapshots by running a scoped npm test -- --testPathPattern .... Consider clarifying the intended workflow (e.g., update only snapshots for tests you changed, always run with a scoped pattern, and ensure a human reviews diffs) rather than prohibiting -u outright.
| - MUST NOT modify/update any existing snapshots, let human review the test failures, they could be failing legitimately | |
| - MUST NOT use `-u` to update snapshots, delete the snapshots and rerun `npm test` instead | |
| - Only update snapshots for tests you intentionally changed, and always run `npm test -- --testPathPattern <test-html-file.html>` scoped to those tests | |
| - Do not run a blanket `npm test -u`; ensure a human reviews and approves all snapshot diffs before committing them |
Added guidelines for test cases, code coverage, and deprecation notes.
Changelog Entry
Description
More guidelines.
Design
Specific Changes
I have added tests and executed them locallyI have updatedCHANGELOG.mdReview Checklist
Accessibility reviewed (tab order, content readability, alt text, color contrast)Browser and platform compatibilities reviewedCSS styles reviewed (minimal rules, noz-index)Internationalization reviewed (strings, unit formatting)package.jsonandpackage-lock.jsonreviewedSecurity reviewed (no data URIs, check for nonce leak)Tests reviewed (coverage, legitimacy)