Skip to content

Update coding guidelines in AGENTS.md #5766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
compulim wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update coding guidelines in AGENTS.md #5766

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ebdaea
Update coding guidelines in AGENTS.md
compulim Mar 4, 2026
eba7179
Clarify snapshot handling instructions
compulim Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### General

- Read `docs/CONTRIBUTING.md` for details on setting up the repository for development
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
- 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

Copilot uses AI. Check for mistakes.
- Unless stated otherwise, avoid Node.js
- Apply our latest coding style to every file changed
- Avoid spaghetti code: on new feature with a similar existing feature, refactor existing one before writing new feature
Expand All @@ -23,6 +24,9 @@
- Prefer uppercase for acronyms instead of Pascal case, e.g. `getURL()` over `getUrl()`
- 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%
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
- 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)

Copilot uses AI. Check for mistakes.
- Deprecation notes should mark the date as 2 years from now
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“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.

Suggested change
- 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.

Copilot uses AI. Check for mistakes.

### Design

Expand All @@ -48,7 +52,11 @@
- Use `{ readonly value: string }` instead of `Readonly<{ value: string }>`
- Use as much `readonly` as possible

### React template
### React

- Always add `displayName`
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
- 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

Copilot uses AI. Check for mistakes.

Follow the template below.

```tsx
import { reactNode, validateProps } from '@msinternal/botframework-webchat-react-valibot';
Expand Down Expand Up @@ -100,6 +108,8 @@ export { MyComponentPropsSchema, type MyComponentProps };
- Use `@testduet/given-when-then` package instead of xUnit style `describe`/`before`/`test`/`after`
- Prefer integration/end-to-end testing than unit testing
- Use as realistic setup as possible, such as using `msw` than mocking calls
- 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
Comment on lines +111 to +112
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
- 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

Copilot uses AI. Check for mistakes.

## PR instructions

Expand Down
Loading