From c03b86abd03a82dbae30b50858e4fc15c2d479a9 Mon Sep 17 00:00:00 2001 From: Adam Dierkens Date: Thu, 16 Jul 2026 15:16:25 -0400 Subject: [PATCH 1/6] Add ADR-backed component review guidance Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68a3cde5-7a50-4d4e-97d9-194c8b112e88 --- .../agents/primer-component-reviewer.agent.md | 27 ++ .../adr-review-guidance.instructions.md | 26 ++ .../architecture-api-language.instructions.md | 24 ++ ...architecture-button-layout.instructions.md | 18 + .../architecture-color.instructions.md | 36 ++ ...architecture-notifications.instructions.md | 18 + .../architecture-select-panel.instructions.md | 43 +++ .../generated/component-api.instructions.md | 90 +++++ .../component-structure.instructions.md | 36 ++ .../generated/css.instructions.md | 48 +++ ...eprecations-and-versioning.instructions.md | 30 ++ .../generated/stories.instructions.md | 36 ++ .../generated/testing.instructions.md | 36 ++ .../skills/primer-component-review/SKILL.md | 44 +++ .../references/generated-policy.md | 333 ++++++++++++++++++ .github/workflows/ci.yml | 2 + .github/workflows/lint-autofix.yml | 4 + .prettierignore | 2 + AGENTS.md | 22 ++ contributor-docs/adrs/adr-000-template.md | 8 + contributor-docs/review-guidance/README.md | 37 ++ .../review-guidance/internal-policy.json | 121 +++++++ contributor-docs/review-guidance/policy.json | 325 +++++++++++++++++ package.json | 4 + script/review-guidance.mjs | 276 +++++++++++++++ script/review-guidance.test.mjs | 74 ++++ 26 files changed, 1720 insertions(+) create mode 100644 .github/agents/primer-component-reviewer.agent.md create mode 100644 .github/instructions/adr-review-guidance.instructions.md create mode 100644 .github/instructions/generated/architecture-api-language.instructions.md create mode 100644 .github/instructions/generated/architecture-button-layout.instructions.md create mode 100644 .github/instructions/generated/architecture-color.instructions.md create mode 100644 .github/instructions/generated/architecture-notifications.instructions.md create mode 100644 .github/instructions/generated/architecture-select-panel.instructions.md create mode 100644 .github/instructions/generated/component-api.instructions.md create mode 100644 .github/instructions/generated/component-structure.instructions.md create mode 100644 .github/instructions/generated/css.instructions.md create mode 100644 .github/instructions/generated/deprecations-and-versioning.instructions.md create mode 100644 .github/instructions/generated/stories.instructions.md create mode 100644 .github/instructions/generated/testing.instructions.md create mode 100644 .github/skills/primer-component-review/SKILL.md create mode 100644 .github/skills/primer-component-review/references/generated-policy.md create mode 100644 AGENTS.md create mode 100644 contributor-docs/review-guidance/README.md create mode 100644 contributor-docs/review-guidance/internal-policy.json create mode 100644 contributor-docs/review-guidance/policy.json create mode 100644 script/review-guidance.mjs create mode 100644 script/review-guidance.test.mjs diff --git a/.github/agents/primer-component-reviewer.agent.md b/.github/agents/primer-component-reviewer.agent.md new file mode 100644 index 00000000000..107844ffdce --- /dev/null +++ b/.github/agents/primer-component-reviewer.agent.md @@ -0,0 +1,27 @@ +--- +name: primer-component-reviewer +description: Reviews Primer React component changes against accepted architectural decisions and maintained repository guidance. +tools: + - read + - search + - execute +skills: + - primer-component-review +--- + +You are a read-only reviewer for Primer React component changes. Never modify +files. + +Use the `primer-component-review` skill and its generated policy reference. +Review component APIs, implementation structure, accessibility, behavior, +stories, tests, CSS, deprecations, documentation metadata, exports, versioning, +and changesets when relevant to the diff. + +Report only concrete, actionable mismatches introduced or expanded by the +change. Cite the changed file and line and the governing policy rule ID. Explain +the impact and recommend the smallest design-level correction. + +Do not expose or speculate about private architectural sources. Rules labeled +`Primer architecture policy` are complete outcome-only guidance. + +If the change follows the applicable policy, say so directly. diff --git a/.github/instructions/adr-review-guidance.instructions.md b/.github/instructions/adr-review-guidance.instructions.md new file mode 100644 index 00000000000..9b99342fd87 --- /dev/null +++ b/.github/instructions/adr-review-guidance.instructions.md @@ -0,0 +1,26 @@ +--- +applyTo: 'contributor-docs/adrs/**/*.md,contributor-docs/CONTRIBUTING.md,contributor-docs/style.md,contributor-docs/testing.md,contributor-docs/authoring-css.md,contributor-docs/behaviors.md,contributor-docs/component-contents-api-patterns.md,contributor-docs/deprecating-components.md,contributor-docs/versioning.md' +--- + +# Maintaining component review guidance + +When an ADR or maintained contributor guide changes an actionable decision about +component APIs, implementation, accessibility, stories, testing, CSS, +deprecations, documentation, or versioning: + +1. Update, add, supersede, or remove the corresponding rule in + `contributor-docs/review-guidance/policy.json`. +2. Keep each rule outcome-focused and suitable for public review comments. Do not + copy private rationale or implementation history into a rule. +3. Use `enforce` only for accepted, current expectations. Use `advisory` for + guidance that should apply only while a related contract is being designed or + expanded. +4. Ensure each affected rule cites the exact changed source in its `source` + field. +5. Run `npm run accept:review-guidance-sources` only after reviewing all rules + mapped to the changed source. +6. Run `npm run build:review-guidance`. +7. Run `npm run check:review-guidance` and + `npm run test:review-guidance`. + +If the source change has no component-review impact, do not add a rule. diff --git a/.github/instructions/generated/architecture-api-language.instructions.md b/.github/instructions/generated/architecture-api-language.instructions.md new file mode 100644 index 00000000000..542508f2689 --- /dev/null +++ b/.github/instructions/generated/architecture-api-language.instructions.md @@ -0,0 +1,24 @@ +--- +applyTo: 'packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json,packages/react/src/**/*.hookDocs.json' +--- + + + +# Component API language + +Policy set: `architecture-api-language`. Source digest: `sanitized-import`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `architecture-api-language.semantic-size-names` (advisory) + +- **Check:** New public size, density, thickness, and viewport-range APIs use the established semantic vocabulary instead of abbreviations, CSS literals, or default as a size name. +- **Prefer:** Use descriptive full names and semantic values that remain meaningful across platforms. +- **Authority:** Primer architecture policy: `architecture-api-language.semantic-size-names` + +### `architecture-api-language.component-references` (advisory) + +- **Check:** Documentation and stories use the exported code name when referring to a specific React component and sentence case when discussing a generic interface pattern. +- **Prefer:** Keep API references unambiguous without presenting every generic pattern name as a code identifier. +- **Authority:** Primer architecture policy: `architecture-api-language.component-references` + diff --git a/.github/instructions/generated/architecture-button-layout.instructions.md b/.github/instructions/generated/architecture-button-layout.instructions.md new file mode 100644 index 00000000000..1f711462fcb --- /dev/null +++ b/.github/instructions/generated/architecture-button-layout.instructions.md @@ -0,0 +1,18 @@ +--- +applyTo: 'packages/react/src/**/*Button*.tsx,packages/react/src/**/*Button*.css,packages/react/src/SelectPanel/**/*.tsx,packages/react/src/experimental/SelectPanel2/**/*.tsx' +--- + + + +# Button layout semantics + +Policy set: `architecture-button-layout`. Source digest: `sanitized-import`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `architecture-button-layout.full-width-alignment` (advisory) + +- **Check:** A full-width call-to-action centers its content, while a full-width selection trigger aligns its primary content to the start and keeps any trailing affordance at the end. +- **Prefer:** Use alignment to communicate whether the control performs an action or exposes a selection. +- **Authority:** Primer architecture policy: `architecture-button-layout.full-width-alignment` + diff --git a/.github/instructions/generated/architecture-color.instructions.md b/.github/instructions/generated/architecture-color.instructions.md new file mode 100644 index 00000000000..4b8ddcdbfd5 --- /dev/null +++ b/.github/instructions/generated/architecture-color.instructions.md @@ -0,0 +1,36 @@ +--- +applyTo: 'packages/react/src/**/*.module.css' +--- + + + +# Semantic color and contrast + +Policy set: `architecture-color`. Source digest: `sanitized-import`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `architecture-color.state-roles` (advisory) + +- **Check:** Open and closed states use their corresponding semantic functional color roles instead of success and danger roles. +- **Prefer:** Preserve the distinction between lifecycle state and validation or outcome semantics. +- **Authority:** Primer architecture policy: `architecture-color.state-roles` + +### `architecture-color.canonical-variants` + +- **Check:** The change does not introduce deprecated subtle functional color variants. +- **Prefer:** Use muted for secondary or less-prominent treatment and emphasis for stronger treatment. +- **Authority:** Primer architecture policy: `architecture-color.canonical-variants` + +### `architecture-color.text-and-links` (advisory) + +- **Check:** Non-decorative text and placeholders maintain at least 4.5:1 contrast against every supported background, and links remain distinguishable from surrounding text without relying only on color. +- **Prefer:** Use supported token pairs and retain a visible non-color link affordance. +- **Authority:** Primer architecture policy: `architecture-color.text-and-links` + +### `architecture-color.focus-and-state` (advisory) + +- **Check:** Focus indicators and simultaneously visible interactive states maintain at least 3:1 contrast against adjacent colors. +- **Prefer:** Verify contrast across supported themes without imposing a contrast requirement on purely decorative or inactive control borders. +- **Authority:** Primer architecture policy: `architecture-color.focus-and-state` + diff --git a/.github/instructions/generated/architecture-notifications.instructions.md b/.github/instructions/generated/architecture-notifications.instructions.md new file mode 100644 index 00000000000..28664c4cf3c --- /dev/null +++ b/.github/instructions/generated/architecture-notifications.instructions.md @@ -0,0 +1,18 @@ +--- +applyTo: 'packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json' +--- + + + +# Notification patterns + +Policy set: `architecture-notifications`. Source digest: `sanitized-import`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `architecture-notifications.no-toast-pattern` + +- **Check:** The change does not add, restore, or recommend a toast component or toast recipe. +- **Prefer:** Use a supported feedback pattern appropriate to the interaction, such as a banner, inline validation, inline message, or dialog. +- **Authority:** Primer architecture policy: `architecture-notifications.no-toast-pattern` + diff --git a/.github/instructions/generated/architecture-select-panel.instructions.md b/.github/instructions/generated/architecture-select-panel.instructions.md new file mode 100644 index 00000000000..4b1e8ae2a62 --- /dev/null +++ b/.github/instructions/generated/architecture-select-panel.instructions.md @@ -0,0 +1,43 @@ +--- +applyTo: 'packages/react/src/SelectPanel/**/*' +--- + + + +# SelectPanel interaction contract + +Policy set: `architecture-select-panel`. Source digest: `sanitized-import`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `architecture-select-panel.focus-restoration` + +- **Check:** Opening moves focus to the configured initial target and closing restores focus to the trigger. +- **Prefer:** Focus the filter input by default while preserving explicit deferred-focus behavior and the component's established secondary-focus semantics. +- **Authority:** Primer architecture policy: `architecture-select-panel.focus-restoration` + +### `architecture-select-panel.commit-behavior` + +- **Check:** Anchored dismissal preserves the latest selection, while modal Save finalizes changes and modal cancellation or dismissal invokes the cancellation contract. +- **Prefer:** Preserve distinct anchored and modal commit semantics across every close path so consumers can restore the prior committed modal selection. +- **Authority:** Primer architecture policy: `architecture-select-panel.commit-behavior` + +### `architecture-select-panel.filter-focus` + +- **Check:** Input focus and filtering preserve a valid active option whenever option activation is enabled. +- **Prefer:** Keep the input and list behavior connected and normally activate the first available option after filtering. +- **Exceptions:** An explicit deferred-initial-focus configuration may wait for user interaction before assigning the active option. +- **Authority:** Primer architecture policy: `architecture-select-panel.filter-focus` + +### `architecture-select-panel.keyboard-navigation` + +- **Check:** Keyboard behavior supports toggling the active option, sequential tab navigation, directional option navigation, first and last option shortcuts, and Escape dismissal. +- **Prefer:** Maintain the complete interaction contract instead of adding isolated key handlers. +- **Authority:** Primer architecture policy: `architecture-select-panel.keyboard-navigation` + +### `architecture-select-panel.announcements` + +- **Check:** Screen-reader announcements communicate the active option, its position, and selection state without announcing on every filtering keystroke. +- **Prefer:** Use polite, delayed announcements and debounce filtering updates. +- **Authority:** Primer architecture policy: `architecture-select-panel.announcements` + diff --git a/.github/instructions/generated/component-api.instructions.md b/.github/instructions/generated/component-api.instructions.md new file mode 100644 index 00000000000..cce228c00c1 --- /dev/null +++ b/.github/instructions/generated/component-api.instructions.md @@ -0,0 +1,90 @@ +--- +applyTo: 'packages/react/src/**/*.ts,packages/react/src/**/*.tsx' +--- + + + +# Component API and implementation + +Policy set: `component-api`. Source digest: `e2ac6c7a9525`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `component-api.spectrum-of-abstraction` + +- **Check:** New component APIs begin with flexible presentational components and behavior hooks unless the product pattern and its defaults are already stable. +- **Prefer:** Build config components by composing established presentational parts and hooks instead of duplicating their behavior or markup. +- **Authority:** `contributor-docs/style.md` + +### `component-api.children-for-content` + +- **Check:** Content uses React children when consumers own the rendered elements or need free-form composition. +- **Prefer:** Use data props when the component must own, transform, order, or constrain the rendered elements. Do not support equivalent children and data APIs simultaneously without a concrete need. +- **Authority:** `contributor-docs/component-contents-api-patterns.md` + +### `component-api.no-speculative-parts` + +- **Check:** Compound parts, render props, child-prop pass-through APIs, and escape hatches address demonstrated use cases rather than speculative flexibility. +- **Prefer:** Start with the smallest API that supports known scenarios and open it up when a concrete need appears. +- **Authority:** `contributor-docs/adrs/adr-004-children-as-api.md` + +### `component-api.avoid-slots-by-default` + +- **Check:** Slots, useSlots, and __SLOT__ markers are only introduced when a parent must identify a specific child or the requested API requires child extraction. +- **Prefer:** Use normal React composition and preserve consumer-authored child order. +- **Authority:** `contributor-docs/style.md` + +### `component-api.reuse-base-primitives` + +- **Check:** Components reuse existing base components, accessibility primitives, hooks, and utilities before introducing parallel implementations. +- **Prefer:** Use primitives such as ButtonBase for Primer-owned native semantics, interactions, and reset styling. +- **Authority:** `contributor-docs/style.md` + +### `component-api.controlled-state` + +- **Check:** Components supporting controlled and uncontrolled state use the shared useControllableState pattern. +- **Prefer:** Keep controlled value, default value, and change callback behavior aligned with the repository helper. +- **Authority:** `contributor-docs/style.md` + +### `component-api.root-rest-props` + +- **Check:** Forwarded rest props and className are applied to the component root unless the public contract explicitly identifies another element. +- **Prefer:** Test that forwarded props reach the documented element. +- **Authority:** `contributor-docs/style.md` + +### `component-api.extensible-callbacks` + +- **Check:** New public callback props use a single object argument when the callback payload may grow. +- **Prefer:** Use a named object payload instead of positional arguments. +- **Authority:** `contributor-docs/style.md` + +### `component-api.ssr` + +- **Check:** Render paths avoid browser-only APIs and hydration-dependent layout changes. +- **Prefer:** Progressively enhance after hydration and move browser access into appropriate effects or event handlers. +- **Authority:** `contributor-docs/style.md` + +### `component-api.focus-from-events` + +- **Check:** Focus moves as a direct result of user events rather than unrelated effect dependency changes. +- **Prefer:** Manage focus in the event handler that caused the transition. +- **Authority:** `contributor-docs/style.md` + +### `component-api.hook-ref-input` + +- **Check:** Hooks that operate on a caller-owned DOM node accept a ref instead of creating and returning an incompatible ref. +- **Prefer:** Let callers compose multiple behaviors around the same ref. +- **Authority:** `contributor-docs/style.md` + +### `component-api.stable-hook-callbacks` + +- **Check:** Hook effects do not repeatedly resubscribe solely because a callback argument changed identity. +- **Prefer:** Use the repository stable-callback pattern so subscriptions always call the latest callback. +- **Authority:** `contributor-docs/style.md` + +### `component-api.public-hooks-need-consumers` + +- **Check:** New public hooks represent a clear consumer capability rather than exposing subcomponent implementation details. +- **Prefer:** Keep behavior hooks internal until a supported external use case and complete public contract exist. +- **Authority:** `contributor-docs/style.md` + diff --git a/.github/instructions/generated/component-structure.instructions.md b/.github/instructions/generated/component-structure.instructions.md new file mode 100644 index 00000000000..9e9ab732705 --- /dev/null +++ b/.github/instructions/generated/component-structure.instructions.md @@ -0,0 +1,36 @@ +--- +applyTo: 'packages/react/src/**/*' +--- + + + +# Component files and public surface + +Policy set: `component-structure`. Source digest: `21badb7273eb`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `component-structure.colocate-files` + +- **Check:** New components use a PascalCase directory and colocate implementation, CSS, tests, stories, docs metadata, and exports according to repository conventions. +- **Prefer:** Name subcomponent files with the parent component prefix for discoverability. +- **Authority:** `contributor-docs/adrs/adr-013-file-structure.md` + +### `component-structure.internal-modules` + +- **Check:** New shared modules that are not public API live under packages/react/src/internal and are not exported from public entrypoints. +- **Prefer:** Keep implementation details internal until a supported public API is intentionally designed. +- **Authority:** `contributor-docs/adrs/adr-015-internal-modules.md` + +### `component-structure.stable-identifiers` (advisory) + +- **Check:** New or changed component roots, public subcomponents, and meaningful structural parts follow the established data-component naming contract. +- **Prefer:** Use PascalCase component API names, keep state in separate data attributes, and test stable identifier values. +- **Authority:** `contributor-docs/adrs/adr-023-stable-selectors-api.md` + +### `component-structure.complete-public-surface` + +- **Check:** Public API changes update exports, export snapshots, docs metadata, tests, stories, and changesets as applicable. +- **Prefer:** Treat all public surfaces as one component contract. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + diff --git a/.github/instructions/generated/css.instructions.md b/.github/instructions/generated/css.instructions.md new file mode 100644 index 00000000000..0247be59d82 --- /dev/null +++ b/.github/instructions/generated/css.instructions.md @@ -0,0 +1,48 @@ +--- +applyTo: 'packages/react/src/**/*.css,packages/react/src/**/*.tsx' +--- + + + +# Component CSS + +Policy set: `css`. Source digest: `043e538ad1a2`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `css.modules` + +- **Check:** Component styles use colocated CSS Modules and Primer primitive custom properties. +- **Prefer:** Avoid new runtime-injected component styling and hardcoded design values. +- **Authority:** `contributor-docs/adrs/adr-016-css.md` + +### `css.root-classname` + +- **Check:** A consumer className is merged with clsx on the top-level element and cannot be overwritten by prop spread order. +- **Prefer:** Use CSS custom properties instead of multiple child className escape hatches. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.data-attributes-for-variants` + +- **Check:** Component variants and state use data attributes or native pseudo-classes rather than modifier class names. +- **Prefer:** Use :where() around data-attribute selectors to keep specificity low. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.javascript-bridge` + +- **Check:** Dynamic values crossing from JavaScript into CSS use custom properties instead of authored visual styles inline. +- **Prefer:** Keep layout and visual rules in the CSS Module. +- **Authority:** `contributor-docs/style.md` + +### `css.no-primitive-fallbacks` + +- **Check:** Primer primitive custom properties are used without manually authored fallback values. +- **Prefer:** Rely on the primitive build output to provide supported fallbacks. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.browser-support` + +- **Check:** CSS newer than Baseline 2022 includes an appropriate fallback and broadly matching selectors avoid expensive :has() invalidation. +- **Prefer:** Scope necessary :has() usage to a CSS Module class with a justified local stylelint disable. +- **Authority:** `.github/instructions/css.instructions.md` + diff --git a/.github/instructions/generated/deprecations-and-versioning.instructions.md b/.github/instructions/generated/deprecations-and-versioning.instructions.md new file mode 100644 index 00000000000..7c6a00072c2 --- /dev/null +++ b/.github/instructions/generated/deprecations-and-versioning.instructions.md @@ -0,0 +1,30 @@ +--- +applyTo: 'packages/react/src/**/*,.changeset/*.md' +--- + + + +# Deprecations, versioning, and changesets + +Policy set: `deprecations-and-versioning`. Source digest: `9d4c59fd70d0`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `deprecations-and-versioning.complete-deprecation-signal` + +- **Check:** Deprecations update JSDoc, docs metadata, migration guidance, and the recommended replacement where applicable. +- **Prefer:** Keep runtime behavior unchanged unless the change explicitly includes migration behavior. +- **Authority:** `.github/skills/deprecations/SKILL.md` + +### `deprecations-and-versioning.consumer-impact` + +- **Check:** The changeset bump matches the largest consumer-facing API, behavior, CSS, accessibility, or stable-identifier impact. +- **Prefer:** Use the repository versioning table and describe the outcome from the consumer perspective. +- **Authority:** `contributor-docs/versioning.md` + +### `deprecations-and-versioning.changeset-required` + +- **Check:** Published runtime or public API changes include an accurate changeset; docs-only, test-only, story-only, and infrastructure-only changes do not. +- **Prefer:** Keep each changeset terse and scoped to one consumer-facing change. +- **Authority:** `.github/skills/changesets/SKILL.md` + diff --git a/.github/instructions/generated/stories.instructions.md b/.github/instructions/generated/stories.instructions.md new file mode 100644 index 00000000000..d049e4bea82 --- /dev/null +++ b/.github/instructions/generated/stories.instructions.md @@ -0,0 +1,36 @@ +--- +applyTo: 'packages/react/src/**/*.stories.tsx' +--- + + + +# Storybook stories + +Policy set: `stories`. Source digest: `ce5b186845a5`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `stories.default-and-playground` + +- **Check:** The default story file uses the Components/ComponentName title and includes Default and Playground when configurable controls are supported. +- **Prefer:** Keep the primary documentation stories in ComponentName.stories.tsx. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + +### `stories.separate-features-and-examples` + +- **Check:** Feature coverage and scenario examples are separated from the default story file. +- **Prefer:** Use ComponentName.features.stories.tsx for features and ComponentName.examples.stories.tsx for distinct scenarios. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + +### `stories.document-supported-api` + +- **Check:** Stories demonstrate supported props, variants, and composition rather than inventing undocumented APIs. +- **Prefer:** Keep stories aligned with component docs metadata and real consumer-facing behavior. +- **Authority:** `.github/skills/storybook/SKILL.md` + +### `stories.keep-interactions-in-tests` + +- **Check:** Stories do not add play functions as a substitute for interaction coverage. +- **Prefer:** Exercise browser interactions through Playwright against the story and keep unit-level interactions in focused tests. +- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` + diff --git a/.github/instructions/generated/testing.instructions.md b/.github/instructions/generated/testing.instructions.md new file mode 100644 index 00000000000..fe2c0b0e4e9 --- /dev/null +++ b/.github/instructions/generated/testing.instructions.md @@ -0,0 +1,36 @@ +--- +applyTo: 'packages/react/src/**/*.test.ts,packages/react/src/**/*.test.tsx,e2e/**/*.test.ts' +--- + + + +# Component testing + +Policy set: `testing`. Source digest: `c5645620dafd`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `testing.observable-behavior` + +- **Check:** Unit tests assert observable semantics and behavior from the user perspective rather than implementation details. +- **Prefer:** Use Testing Library roles, accessible names, userEvent, and explicit contract assertions. +- **Authority:** `contributor-docs/testing.md` + +### `testing.no-catch-all-render-snapshots` + +- **Check:** Tests do not add catch-all snapshots of rendered React trees. +- **Prefer:** Reserve snapshots for static objects with a clear review signal, such as package export lists. +- **Authority:** `contributor-docs/adrs/adr-011-snapshot-tests.md` + +### `testing.playwright-for-browser-behavior` + +- **Check:** Behavior requiring real layout, browser interaction, visual regression, or automated accessibility coverage is tested through the Playwright e2e suite. +- **Prefer:** Use @vrt and @avt coverage where visual appearance or accessibility output changes. +- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` + +### `testing.public-contract-coverage` + +- **Check:** New public props, forwarded DOM props, stable identifiers, keyboard behavior, and accessibility semantics have focused regression coverage. +- **Prefer:** Name tests for the contract they protect. +- **Authority:** `contributor-docs/testing.md` + diff --git a/.github/skills/primer-component-review/SKILL.md b/.github/skills/primer-component-review/SKILL.md new file mode 100644 index 00000000000..64e271323f2 --- /dev/null +++ b/.github/skills/primer-component-review/SKILL.md @@ -0,0 +1,44 @@ +--- +name: primer-component-review +description: 'Use when: reviewing Primer React component changes for repository architectural decisions and maintained component guidance. Covers component APIs, implementation structure, accessibility, stories, testing, CSS, deprecations, versioning, and changesets.' +--- + +# Primer component review + +Use this skill for code review of Primer React components and their supporting +files. + +## Sources + +Read `references/generated-policy.md` and apply only the policy sets relevant to +the changed files. Path-specific generated instructions under +`.github/instructions/generated/` contain the same rules scoped for Copilot code +review. + +Public ADRs and maintained contributor guides are authoritative. Rules labeled +`Primer architecture policy` contain only public-safe outcomes imported from a +private decision source. Do not seek, infer, mention, or disclose the private +source or rationale. + +## Review procedure + +1. Determine which public contracts and behaviors the change adds, removes, or + modifies. +2. Select the relevant policy sets from the generated reference. +3. Compare changed code and directly related tests, stories, docs, exports, and + changesets with enforced rules. +4. Apply advisory rules only when the change is designing or expanding the + affected contract and the mismatch has concrete consumer impact. +5. Report only findings introduced or made worse by the change. + +For each finding: + +- cite the changed file and line; +- cite the policy rule ID; +- describe the concrete consumer, accessibility, maintenance, or compatibility + impact; +- suggest the smallest design-level correction. + +Do not report formatting issues, preferences without policy support, or +pre-existing migration debt. If two active rules conflict, report the policy +conflict instead of choosing one. diff --git a/.github/skills/primer-component-review/references/generated-policy.md b/.github/skills/primer-component-review/references/generated-policy.md new file mode 100644 index 00000000000..c105a9ad90c --- /dev/null +++ b/.github/skills/primer-component-review/references/generated-policy.md @@ -0,0 +1,333 @@ + + +# Primer component review policy + +Apply only the policy sets relevant to the changed files. Enforced rules may produce findings. Advisory rules should only produce a finding when the change is designing or expanding the affected contract and the mismatch has a concrete consumer impact. + +## Component API language + +Applies to: `packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json,packages/react/src/**/*.hookDocs.json` + +### `architecture-api-language.semantic-size-names` (advisory) + +- **Check:** New public size, density, thickness, and viewport-range APIs use the established semantic vocabulary instead of abbreviations, CSS literals, or default as a size name. +- **Prefer:** Use descriptive full names and semantic values that remain meaningful across platforms. +- **Authority:** Primer architecture policy: `architecture-api-language.semantic-size-names` + +### `architecture-api-language.component-references` (advisory) + +- **Check:** Documentation and stories use the exported code name when referring to a specific React component and sentence case when discussing a generic interface pattern. +- **Prefer:** Keep API references unambiguous without presenting every generic pattern name as a code identifier. +- **Authority:** Primer architecture policy: `architecture-api-language.component-references` + +## Button layout semantics + +Applies to: `packages/react/src/**/*Button*.tsx,packages/react/src/**/*Button*.css,packages/react/src/SelectPanel/**/*.tsx,packages/react/src/experimental/SelectPanel2/**/*.tsx` + +### `architecture-button-layout.full-width-alignment` (advisory) + +- **Check:** A full-width call-to-action centers its content, while a full-width selection trigger aligns its primary content to the start and keeps any trailing affordance at the end. +- **Prefer:** Use alignment to communicate whether the control performs an action or exposes a selection. +- **Authority:** Primer architecture policy: `architecture-button-layout.full-width-alignment` + +## Semantic color and contrast + +Applies to: `packages/react/src/**/*.module.css` + +### `architecture-color.state-roles` (advisory) + +- **Check:** Open and closed states use their corresponding semantic functional color roles instead of success and danger roles. +- **Prefer:** Preserve the distinction between lifecycle state and validation or outcome semantics. +- **Authority:** Primer architecture policy: `architecture-color.state-roles` + +### `architecture-color.canonical-variants` + +- **Check:** The change does not introduce deprecated subtle functional color variants. +- **Prefer:** Use muted for secondary or less-prominent treatment and emphasis for stronger treatment. +- **Authority:** Primer architecture policy: `architecture-color.canonical-variants` + +### `architecture-color.text-and-links` (advisory) + +- **Check:** Non-decorative text and placeholders maintain at least 4.5:1 contrast against every supported background, and links remain distinguishable from surrounding text without relying only on color. +- **Prefer:** Use supported token pairs and retain a visible non-color link affordance. +- **Authority:** Primer architecture policy: `architecture-color.text-and-links` + +### `architecture-color.focus-and-state` (advisory) + +- **Check:** Focus indicators and simultaneously visible interactive states maintain at least 3:1 contrast against adjacent colors. +- **Prefer:** Verify contrast across supported themes without imposing a contrast requirement on purely decorative or inactive control borders. +- **Authority:** Primer architecture policy: `architecture-color.focus-and-state` + +## Notification patterns + +Applies to: `packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json` + +### `architecture-notifications.no-toast-pattern` + +- **Check:** The change does not add, restore, or recommend a toast component or toast recipe. +- **Prefer:** Use a supported feedback pattern appropriate to the interaction, such as a banner, inline validation, inline message, or dialog. +- **Authority:** Primer architecture policy: `architecture-notifications.no-toast-pattern` + +## SelectPanel interaction contract + +Applies to: `packages/react/src/SelectPanel/**/*` + +### `architecture-select-panel.focus-restoration` + +- **Check:** Opening moves focus to the configured initial target and closing restores focus to the trigger. +- **Prefer:** Focus the filter input by default while preserving explicit deferred-focus behavior and the component's established secondary-focus semantics. +- **Authority:** Primer architecture policy: `architecture-select-panel.focus-restoration` + +### `architecture-select-panel.commit-behavior` + +- **Check:** Anchored dismissal preserves the latest selection, while modal Save finalizes changes and modal cancellation or dismissal invokes the cancellation contract. +- **Prefer:** Preserve distinct anchored and modal commit semantics across every close path so consumers can restore the prior committed modal selection. +- **Authority:** Primer architecture policy: `architecture-select-panel.commit-behavior` + +### `architecture-select-panel.filter-focus` + +- **Check:** Input focus and filtering preserve a valid active option whenever option activation is enabled. +- **Prefer:** Keep the input and list behavior connected and normally activate the first available option after filtering. +- **Exceptions:** An explicit deferred-initial-focus configuration may wait for user interaction before assigning the active option. +- **Authority:** Primer architecture policy: `architecture-select-panel.filter-focus` + +### `architecture-select-panel.keyboard-navigation` + +- **Check:** Keyboard behavior supports toggling the active option, sequential tab navigation, directional option navigation, first and last option shortcuts, and Escape dismissal. +- **Prefer:** Maintain the complete interaction contract instead of adding isolated key handlers. +- **Authority:** Primer architecture policy: `architecture-select-panel.keyboard-navigation` + +### `architecture-select-panel.announcements` + +- **Check:** Screen-reader announcements communicate the active option, its position, and selection state without announcing on every filtering keystroke. +- **Prefer:** Use polite, delayed announcements and debounce filtering updates. +- **Authority:** Primer architecture policy: `architecture-select-panel.announcements` + +## Component API and implementation + +Applies to: `packages/react/src/**/*.ts,packages/react/src/**/*.tsx` + +### `component-api.spectrum-of-abstraction` + +- **Check:** New component APIs begin with flexible presentational components and behavior hooks unless the product pattern and its defaults are already stable. +- **Prefer:** Build config components by composing established presentational parts and hooks instead of duplicating their behavior or markup. +- **Authority:** `contributor-docs/style.md` + +### `component-api.children-for-content` + +- **Check:** Content uses React children when consumers own the rendered elements or need free-form composition. +- **Prefer:** Use data props when the component must own, transform, order, or constrain the rendered elements. Do not support equivalent children and data APIs simultaneously without a concrete need. +- **Authority:** `contributor-docs/component-contents-api-patterns.md` + +### `component-api.no-speculative-parts` + +- **Check:** Compound parts, render props, child-prop pass-through APIs, and escape hatches address demonstrated use cases rather than speculative flexibility. +- **Prefer:** Start with the smallest API that supports known scenarios and open it up when a concrete need appears. +- **Authority:** `contributor-docs/adrs/adr-004-children-as-api.md` + +### `component-api.avoid-slots-by-default` + +- **Check:** Slots, useSlots, and __SLOT__ markers are only introduced when a parent must identify a specific child or the requested API requires child extraction. +- **Prefer:** Use normal React composition and preserve consumer-authored child order. +- **Authority:** `contributor-docs/style.md` + +### `component-api.reuse-base-primitives` + +- **Check:** Components reuse existing base components, accessibility primitives, hooks, and utilities before introducing parallel implementations. +- **Prefer:** Use primitives such as ButtonBase for Primer-owned native semantics, interactions, and reset styling. +- **Authority:** `contributor-docs/style.md` + +### `component-api.controlled-state` + +- **Check:** Components supporting controlled and uncontrolled state use the shared useControllableState pattern. +- **Prefer:** Keep controlled value, default value, and change callback behavior aligned with the repository helper. +- **Authority:** `contributor-docs/style.md` + +### `component-api.root-rest-props` + +- **Check:** Forwarded rest props and className are applied to the component root unless the public contract explicitly identifies another element. +- **Prefer:** Test that forwarded props reach the documented element. +- **Authority:** `contributor-docs/style.md` + +### `component-api.extensible-callbacks` + +- **Check:** New public callback props use a single object argument when the callback payload may grow. +- **Prefer:** Use a named object payload instead of positional arguments. +- **Authority:** `contributor-docs/style.md` + +### `component-api.ssr` + +- **Check:** Render paths avoid browser-only APIs and hydration-dependent layout changes. +- **Prefer:** Progressively enhance after hydration and move browser access into appropriate effects or event handlers. +- **Authority:** `contributor-docs/style.md` + +### `component-api.focus-from-events` + +- **Check:** Focus moves as a direct result of user events rather than unrelated effect dependency changes. +- **Prefer:** Manage focus in the event handler that caused the transition. +- **Authority:** `contributor-docs/style.md` + +### `component-api.hook-ref-input` + +- **Check:** Hooks that operate on a caller-owned DOM node accept a ref instead of creating and returning an incompatible ref. +- **Prefer:** Let callers compose multiple behaviors around the same ref. +- **Authority:** `contributor-docs/style.md` + +### `component-api.stable-hook-callbacks` + +- **Check:** Hook effects do not repeatedly resubscribe solely because a callback argument changed identity. +- **Prefer:** Use the repository stable-callback pattern so subscriptions always call the latest callback. +- **Authority:** `contributor-docs/style.md` + +### `component-api.public-hooks-need-consumers` + +- **Check:** New public hooks represent a clear consumer capability rather than exposing subcomponent implementation details. +- **Prefer:** Keep behavior hooks internal until a supported external use case and complete public contract exist. +- **Authority:** `contributor-docs/style.md` + +## Component files and public surface + +Applies to: `packages/react/src/**/*` + +### `component-structure.colocate-files` + +- **Check:** New components use a PascalCase directory and colocate implementation, CSS, tests, stories, docs metadata, and exports according to repository conventions. +- **Prefer:** Name subcomponent files with the parent component prefix for discoverability. +- **Authority:** `contributor-docs/adrs/adr-013-file-structure.md` + +### `component-structure.internal-modules` + +- **Check:** New shared modules that are not public API live under packages/react/src/internal and are not exported from public entrypoints. +- **Prefer:** Keep implementation details internal until a supported public API is intentionally designed. +- **Authority:** `contributor-docs/adrs/adr-015-internal-modules.md` + +### `component-structure.stable-identifiers` (advisory) + +- **Check:** New or changed component roots, public subcomponents, and meaningful structural parts follow the established data-component naming contract. +- **Prefer:** Use PascalCase component API names, keep state in separate data attributes, and test stable identifier values. +- **Authority:** `contributor-docs/adrs/adr-023-stable-selectors-api.md` + +### `component-structure.complete-public-surface` + +- **Check:** Public API changes update exports, export snapshots, docs metadata, tests, stories, and changesets as applicable. +- **Prefer:** Treat all public surfaces as one component contract. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + +## Component CSS + +Applies to: `packages/react/src/**/*.css,packages/react/src/**/*.tsx` + +### `css.modules` + +- **Check:** Component styles use colocated CSS Modules and Primer primitive custom properties. +- **Prefer:** Avoid new runtime-injected component styling and hardcoded design values. +- **Authority:** `contributor-docs/adrs/adr-016-css.md` + +### `css.root-classname` + +- **Check:** A consumer className is merged with clsx on the top-level element and cannot be overwritten by prop spread order. +- **Prefer:** Use CSS custom properties instead of multiple child className escape hatches. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.data-attributes-for-variants` + +- **Check:** Component variants and state use data attributes or native pseudo-classes rather than modifier class names. +- **Prefer:** Use :where() around data-attribute selectors to keep specificity low. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.javascript-bridge` + +- **Check:** Dynamic values crossing from JavaScript into CSS use custom properties instead of authored visual styles inline. +- **Prefer:** Keep layout and visual rules in the CSS Module. +- **Authority:** `contributor-docs/style.md` + +### `css.no-primitive-fallbacks` + +- **Check:** Primer primitive custom properties are used without manually authored fallback values. +- **Prefer:** Rely on the primitive build output to provide supported fallbacks. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.browser-support` + +- **Check:** CSS newer than Baseline 2022 includes an appropriate fallback and broadly matching selectors avoid expensive :has() invalidation. +- **Prefer:** Scope necessary :has() usage to a CSS Module class with a justified local stylelint disable. +- **Authority:** `.github/instructions/css.instructions.md` + +## Deprecations, versioning, and changesets + +Applies to: `packages/react/src/**/*,.changeset/*.md` + +### `deprecations-and-versioning.complete-deprecation-signal` + +- **Check:** Deprecations update JSDoc, docs metadata, migration guidance, and the recommended replacement where applicable. +- **Prefer:** Keep runtime behavior unchanged unless the change explicitly includes migration behavior. +- **Authority:** `.github/skills/deprecations/SKILL.md` + +### `deprecations-and-versioning.consumer-impact` + +- **Check:** The changeset bump matches the largest consumer-facing API, behavior, CSS, accessibility, or stable-identifier impact. +- **Prefer:** Use the repository versioning table and describe the outcome from the consumer perspective. +- **Authority:** `contributor-docs/versioning.md` + +### `deprecations-and-versioning.changeset-required` + +- **Check:** Published runtime or public API changes include an accurate changeset; docs-only, test-only, story-only, and infrastructure-only changes do not. +- **Prefer:** Keep each changeset terse and scoped to one consumer-facing change. +- **Authority:** `.github/skills/changesets/SKILL.md` + +## Storybook stories + +Applies to: `packages/react/src/**/*.stories.tsx` + +### `stories.default-and-playground` + +- **Check:** The default story file uses the Components/ComponentName title and includes Default and Playground when configurable controls are supported. +- **Prefer:** Keep the primary documentation stories in ComponentName.stories.tsx. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + +### `stories.separate-features-and-examples` + +- **Check:** Feature coverage and scenario examples are separated from the default story file. +- **Prefer:** Use ComponentName.features.stories.tsx for features and ComponentName.examples.stories.tsx for distinct scenarios. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + +### `stories.document-supported-api` + +- **Check:** Stories demonstrate supported props, variants, and composition rather than inventing undocumented APIs. +- **Prefer:** Keep stories aligned with component docs metadata and real consumer-facing behavior. +- **Authority:** `.github/skills/storybook/SKILL.md` + +### `stories.keep-interactions-in-tests` + +- **Check:** Stories do not add play functions as a substitute for interaction coverage. +- **Prefer:** Exercise browser interactions through Playwright against the story and keep unit-level interactions in focused tests. +- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` + +## Component testing + +Applies to: `packages/react/src/**/*.test.ts,packages/react/src/**/*.test.tsx,e2e/**/*.test.ts` + +### `testing.observable-behavior` + +- **Check:** Unit tests assert observable semantics and behavior from the user perspective rather than implementation details. +- **Prefer:** Use Testing Library roles, accessible names, userEvent, and explicit contract assertions. +- **Authority:** `contributor-docs/testing.md` + +### `testing.no-catch-all-render-snapshots` + +- **Check:** Tests do not add catch-all snapshots of rendered React trees. +- **Prefer:** Reserve snapshots for static objects with a clear review signal, such as package export lists. +- **Authority:** `contributor-docs/adrs/adr-011-snapshot-tests.md` + +### `testing.playwright-for-browser-behavior` + +- **Check:** Behavior requiring real layout, browser interaction, visual regression, or automated accessibility coverage is tested through the Playwright e2e suite. +- **Prefer:** Use @vrt and @avt coverage where visual appearance or accessibility output changes. +- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` + +### `testing.public-contract-coverage` + +- **Check:** New public props, forwarded DOM props, stable identifiers, keyboard behavior, and accessibility semantics have focused regression coverage. +- **Prefer:** Name tests for the contract they protect. +- **Authority:** `contributor-docs/testing.md` + diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 47069349b69..9b732b802a7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -27,6 +27,8 @@ jobs: cache: 'npm' - name: Install dependencies run: npm ci + - name: Check generated component review guidance + run: npm run test:review-guidance && npm run check:review-guidance - name: Check for unformatted files run: npm run format:diff diff --git a/.github/workflows/lint-autofix.yml b/.github/workflows/lint-autofix.yml index 63f876d067d..234d7a972c3 100644 --- a/.github/workflows/lint-autofix.yml +++ b/.github/workflows/lint-autofix.yml @@ -101,6 +101,10 @@ jobs: if: steps.check_jobs.outputs.should_fix == 'true' && steps.pr.outputs.head_ref != '' run: npm ci + - name: Regenerate component review guidance + if: steps.check_jobs.outputs.format_failed == 'true' && steps.pr.outputs.head_ref != '' + run: npm run build:review-guidance + - name: Run format auto-fix if: steps.check_jobs.outputs.format_failed == 'true' && steps.pr.outputs.head_ref != '' run: npm run format diff --git a/.prettierignore b/.prettierignore index a7d223c884b..2db784530e3 100644 --- a/.prettierignore +++ b/.prettierignore @@ -1,5 +1,7 @@ .changeset .agents +.github/instructions/generated +.github/skills/primer-component-review/references/generated-policy.md packages/react/CHANGELOG.md dist storybook-static diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000000..71f58e13e9b --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,22 @@ +# Primer React agent guidance + +## Component review + +For changes to Primer React components, hooks, stories, tests, CSS, docs metadata, +exports, or changesets: + +- Apply the path-specific instructions in + `.github/instructions/generated/*.instructions.md`. +- Use the `primer-component-review` skill for the review procedure and complete + generated policy catalog. +- Treat accepted public ADRs and maintained contributor guides as authoritative. + Treat imported Primer architecture policy as outcome-only guidance; never + attempt to discover, infer, or disclose its private rationale or source. +- Report only actionable mismatches introduced or expanded by the change. +- Cite the policy rule ID in each finding. +- Do not report pre-existing migration debt unless the change makes it worse. +- Advisory rules apply only while the affected API or contract is being designed + or expanded. + +When guidance conflicts, stop rather than guessing. Identify the conflicting +public rule IDs or source files so maintainers can resolve the policy. diff --git a/contributor-docs/adrs/adr-000-template.md b/contributor-docs/adrs/adr-000-template.md index 710316666e9..cabf4446f02 100644 --- a/contributor-docs/adrs/adr-000-template.md +++ b/contributor-docs/adrs/adr-000-template.md @@ -9,6 +9,14 @@ | Status | Proposed ❓ OR Accepted ✅ OR Superseded by [LINK](https://) ⚠️ OR Deprecated ⛔ | | Implementation | Not planned ⛔ OR Adoption awaiting in [LINK](https://) ⏸️ OR Adopted ✅ | +## Review impact + + + ## Context diff --git a/contributor-docs/review-guidance/README.md b/contributor-docs/review-guidance/README.md new file mode 100644 index 00000000000..5e4f80b26a2 --- /dev/null +++ b/contributor-docs/review-guidance/README.md @@ -0,0 +1,37 @@ +# Component review guidance + +The files in this directory are the structured source for generated Copilot code +review instructions. + +- `policy.json` records public rules derived from accepted ADRs and maintained + contributor documentation. +- `internal-policy.json` is the public-safe import boundary for architectural + outcomes produced outside this repository. It must never contain private + rationale, links, identifiers, titles, filenames, or quoted source text. + +Run `npm run build:review-guidance` after changing either policy. Generated +instructions live in `.github/instructions/generated/`, and the complete +reviewer reference lives in +`.github/skills/primer-component-review/references/generated-policy.md`. + +## Updating an ADR or contributor guide + +When a source changes an actionable component-development decision: + +1. Update, add, supersede, or remove the corresponding rule in `policy.json`. +2. Run `npm run accept:review-guidance-sources` after reviewing every rule mapped + to the changed source. +3. Run `npm run build:review-guidance`. +4. Run `npm run check:review-guidance`. + +If the source does not affect component review, no policy rule is needed. + +## Public-safe imported rules + +Imported rules use a strict outcome-only schema and omit all source fields. They +must contain only the outcome needed to review public code. The generator rejects +unknown fields, URLs, and known private-source markers in +`internal-policy.json`. + +The sanitizer is defense in depth, not a substitute for producing the import +from explicitly public-safe fields in the private source repository. diff --git a/contributor-docs/review-guidance/internal-policy.json b/contributor-docs/review-guidance/internal-policy.json new file mode 100644 index 00000000000..c5bb015132d --- /dev/null +++ b/contributor-docs/review-guidance/internal-policy.json @@ -0,0 +1,121 @@ +{ + "version": 1, + "ruleSets": [ + { + "id": "architecture-api-language", + "title": "Component API language", + "applyTo": "packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json,packages/react/src/**/*.hookDocs.json", + "rules": [ + { + "id": "architecture-api-language.semantic-size-names", + "enforcement": "advisory", + "check": "New public size, density, thickness, and viewport-range APIs use the established semantic vocabulary instead of abbreviations, CSS literals, or default as a size name.", + "prefer": "Use descriptive full names and semantic values that remain meaningful across platforms." + }, + { + "id": "architecture-api-language.component-references", + "enforcement": "advisory", + "check": "Documentation and stories use the exported code name when referring to a specific React component and sentence case when discussing a generic interface pattern.", + "prefer": "Keep API references unambiguous without presenting every generic pattern name as a code identifier." + } + ] + }, + { + "id": "architecture-button-layout", + "title": "Button layout semantics", + "applyTo": "packages/react/src/**/*Button*.tsx,packages/react/src/**/*Button*.css,packages/react/src/SelectPanel/**/*.tsx,packages/react/src/experimental/SelectPanel2/**/*.tsx", + "rules": [ + { + "id": "architecture-button-layout.full-width-alignment", + "enforcement": "advisory", + "check": "A full-width call-to-action centers its content, while a full-width selection trigger aligns its primary content to the start and keeps any trailing affordance at the end.", + "prefer": "Use alignment to communicate whether the control performs an action or exposes a selection." + } + ] + }, + { + "id": "architecture-notifications", + "title": "Notification patterns", + "applyTo": "packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json", + "rules": [ + { + "id": "architecture-notifications.no-toast-pattern", + "enforcement": "enforce", + "check": "The change does not add, restore, or recommend a toast component or toast recipe.", + "prefer": "Use a supported feedback pattern appropriate to the interaction, such as a banner, inline validation, inline message, or dialog." + } + ] + }, + { + "id": "architecture-color", + "title": "Semantic color and contrast", + "applyTo": "packages/react/src/**/*.module.css", + "rules": [ + { + "id": "architecture-color.state-roles", + "enforcement": "advisory", + "check": "Open and closed states use their corresponding semantic functional color roles instead of success and danger roles.", + "prefer": "Preserve the distinction between lifecycle state and validation or outcome semantics." + }, + { + "id": "architecture-color.canonical-variants", + "enforcement": "enforce", + "check": "The change does not introduce deprecated subtle functional color variants.", + "prefer": "Use muted for secondary or less-prominent treatment and emphasis for stronger treatment." + }, + { + "id": "architecture-color.text-and-links", + "enforcement": "advisory", + "check": "Non-decorative text and placeholders maintain at least 4.5:1 contrast against every supported background, and links remain distinguishable from surrounding text without relying only on color.", + "prefer": "Use supported token pairs and retain a visible non-color link affordance." + }, + { + "id": "architecture-color.focus-and-state", + "enforcement": "advisory", + "check": "Focus indicators and simultaneously visible interactive states maintain at least 3:1 contrast against adjacent colors.", + "prefer": "Verify contrast across supported themes without imposing a contrast requirement on purely decorative or inactive control borders." + } + ] + }, + { + "id": "architecture-select-panel", + "title": "SelectPanel interaction contract", + "applyTo": "packages/react/src/SelectPanel/**/*", + "rules": [ + { + "id": "architecture-select-panel.focus-restoration", + "enforcement": "enforce", + "check": "Opening moves focus to the configured initial target and closing restores focus to the trigger.", + "prefer": "Focus the filter input by default while preserving explicit deferred-focus behavior and the component's established secondary-focus semantics." + }, + { + "id": "architecture-select-panel.commit-behavior", + "enforcement": "enforce", + "check": "Anchored dismissal preserves the latest selection, while modal Save finalizes changes and modal cancellation or dismissal invokes the cancellation contract.", + "prefer": "Preserve distinct anchored and modal commit semantics across every close path so consumers can restore the prior committed modal selection." + }, + { + "id": "architecture-select-panel.filter-focus", + "enforcement": "enforce", + "check": "Input focus and filtering preserve a valid active option whenever option activation is enabled.", + "prefer": "Keep the input and list behavior connected and normally activate the first available option after filtering.", + "exceptions": [ + "An explicit deferred-initial-focus configuration may wait for user interaction before assigning the active option." + ] + }, + { + "id": "architecture-select-panel.keyboard-navigation", + "enforcement": "enforce", + "check": "Keyboard behavior supports toggling the active option, sequential tab navigation, directional option navigation, first and last option shortcuts, and Escape dismissal.", + "prefer": "Maintain the complete interaction contract instead of adding isolated key handlers." + }, + { + "id": "architecture-select-panel.announcements", + "enforcement": "enforce", + "check": "Screen-reader announcements communicate the active option, its position, and selection state without announcing on every filtering keystroke.", + "prefer": "Use polite, delayed announcements and debounce filtering updates." + } + ] + } + ] +} diff --git a/contributor-docs/review-guidance/policy.json b/contributor-docs/review-guidance/policy.json new file mode 100644 index 00000000000..8538daf98f7 --- /dev/null +++ b/contributor-docs/review-guidance/policy.json @@ -0,0 +1,325 @@ +{ + "version": 1, + "ruleSets": [ + { + "id": "component-api", + "title": "Component API and implementation", + "applyTo": "packages/react/src/**/*.ts,packages/react/src/**/*.tsx", + "sources": [ + "contributor-docs/style.md", + "contributor-docs/component-contents-api-patterns.md", + "contributor-docs/adrs/adr-004-children-as-api.md" + ], + "rules": [ + { + "id": "component-api.spectrum-of-abstraction", + "enforcement": "enforce", + "check": "New component APIs begin with flexible presentational components and behavior hooks unless the product pattern and its defaults are already stable.", + "prefer": "Build config components by composing established presentational parts and hooks instead of duplicating their behavior or markup.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.children-for-content", + "enforcement": "enforce", + "check": "Content uses React children when consumers own the rendered elements or need free-form composition.", + "prefer": "Use data props when the component must own, transform, order, or constrain the rendered elements. Do not support equivalent children and data APIs simultaneously without a concrete need.", + "source": "contributor-docs/component-contents-api-patterns.md" + }, + { + "id": "component-api.no-speculative-parts", + "enforcement": "enforce", + "check": "Compound parts, render props, child-prop pass-through APIs, and escape hatches address demonstrated use cases rather than speculative flexibility.", + "prefer": "Start with the smallest API that supports known scenarios and open it up when a concrete need appears.", + "source": "contributor-docs/adrs/adr-004-children-as-api.md" + }, + { + "id": "component-api.avoid-slots-by-default", + "enforcement": "enforce", + "check": "Slots, useSlots, and __SLOT__ markers are only introduced when a parent must identify a specific child or the requested API requires child extraction.", + "prefer": "Use normal React composition and preserve consumer-authored child order.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.reuse-base-primitives", + "enforcement": "enforce", + "check": "Components reuse existing base components, accessibility primitives, hooks, and utilities before introducing parallel implementations.", + "prefer": "Use primitives such as ButtonBase for Primer-owned native semantics, interactions, and reset styling.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.controlled-state", + "enforcement": "enforce", + "check": "Components supporting controlled and uncontrolled state use the shared useControllableState pattern.", + "prefer": "Keep controlled value, default value, and change callback behavior aligned with the repository helper.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.root-rest-props", + "enforcement": "enforce", + "check": "Forwarded rest props and className are applied to the component root unless the public contract explicitly identifies another element.", + "prefer": "Test that forwarded props reach the documented element.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.extensible-callbacks", + "enforcement": "enforce", + "check": "New public callback props use a single object argument when the callback payload may grow.", + "prefer": "Use a named object payload instead of positional arguments.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.ssr", + "enforcement": "enforce", + "check": "Render paths avoid browser-only APIs and hydration-dependent layout changes.", + "prefer": "Progressively enhance after hydration and move browser access into appropriate effects or event handlers.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.focus-from-events", + "enforcement": "enforce", + "check": "Focus moves as a direct result of user events rather than unrelated effect dependency changes.", + "prefer": "Manage focus in the event handler that caused the transition.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.hook-ref-input", + "enforcement": "enforce", + "check": "Hooks that operate on a caller-owned DOM node accept a ref instead of creating and returning an incompatible ref.", + "prefer": "Let callers compose multiple behaviors around the same ref.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.stable-hook-callbacks", + "enforcement": "enforce", + "check": "Hook effects do not repeatedly resubscribe solely because a callback argument changed identity.", + "prefer": "Use the repository stable-callback pattern so subscriptions always call the latest callback.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.public-hooks-need-consumers", + "enforcement": "enforce", + "check": "New public hooks represent a clear consumer capability rather than exposing subcomponent implementation details.", + "prefer": "Keep behavior hooks internal until a supported external use case and complete public contract exist.", + "source": "contributor-docs/style.md" + } + ], + "sourceDigest": "e2ac6c7a9525" + }, + { + "id": "component-structure", + "title": "Component files and public surface", + "applyTo": "packages/react/src/**/*", + "sources": [ + "contributor-docs/CONTRIBUTING.md", + "contributor-docs/adrs/adr-013-file-structure.md", + "contributor-docs/adrs/adr-015-internal-modules.md", + "contributor-docs/adrs/adr-023-stable-selectors-api.md" + ], + "rules": [ + { + "id": "component-structure.colocate-files", + "enforcement": "enforce", + "check": "New components use a PascalCase directory and colocate implementation, CSS, tests, stories, docs metadata, and exports according to repository conventions.", + "prefer": "Name subcomponent files with the parent component prefix for discoverability.", + "source": "contributor-docs/adrs/adr-013-file-structure.md" + }, + { + "id": "component-structure.internal-modules", + "enforcement": "enforce", + "check": "New shared modules that are not public API live under packages/react/src/internal and are not exported from public entrypoints.", + "prefer": "Keep implementation details internal until a supported public API is intentionally designed.", + "source": "contributor-docs/adrs/adr-015-internal-modules.md" + }, + { + "id": "component-structure.stable-identifiers", + "enforcement": "advisory", + "check": "New or changed component roots, public subcomponents, and meaningful structural parts follow the established data-component naming contract.", + "prefer": "Use PascalCase component API names, keep state in separate data attributes, and test stable identifier values.", + "source": "contributor-docs/adrs/adr-023-stable-selectors-api.md" + }, + { + "id": "component-structure.complete-public-surface", + "enforcement": "enforce", + "check": "Public API changes update exports, export snapshots, docs metadata, tests, stories, and changesets as applicable.", + "prefer": "Treat all public surfaces as one component contract.", + "source": "contributor-docs/CONTRIBUTING.md" + } + ], + "sourceDigest": "21badb7273eb" + }, + { + "id": "stories", + "title": "Storybook stories", + "applyTo": "packages/react/src/**/*.stories.tsx", + "sources": [ + "contributor-docs/CONTRIBUTING.md", + "contributor-docs/adrs/adr-017-interaction-tests-revisited.md", + ".github/skills/storybook/SKILL.md" + ], + "rules": [ + { + "id": "stories.default-and-playground", + "enforcement": "enforce", + "check": "The default story file uses the Components/ComponentName title and includes Default and Playground when configurable controls are supported.", + "prefer": "Keep the primary documentation stories in ComponentName.stories.tsx.", + "source": "contributor-docs/CONTRIBUTING.md" + }, + { + "id": "stories.separate-features-and-examples", + "enforcement": "enforce", + "check": "Feature coverage and scenario examples are separated from the default story file.", + "prefer": "Use ComponentName.features.stories.tsx for features and ComponentName.examples.stories.tsx for distinct scenarios.", + "source": "contributor-docs/CONTRIBUTING.md" + }, + { + "id": "stories.document-supported-api", + "enforcement": "enforce", + "check": "Stories demonstrate supported props, variants, and composition rather than inventing undocumented APIs.", + "prefer": "Keep stories aligned with component docs metadata and real consumer-facing behavior.", + "source": ".github/skills/storybook/SKILL.md" + }, + { + "id": "stories.keep-interactions-in-tests", + "enforcement": "enforce", + "check": "Stories do not add play functions as a substitute for interaction coverage.", + "prefer": "Exercise browser interactions through Playwright against the story and keep unit-level interactions in focused tests.", + "source": "contributor-docs/adrs/adr-017-interaction-tests-revisited.md" + } + ], + "sourceDigest": "ce5b186845a5" + }, + { + "id": "testing", + "title": "Component testing", + "applyTo": "packages/react/src/**/*.test.ts,packages/react/src/**/*.test.tsx,e2e/**/*.test.ts", + "sources": [ + "contributor-docs/testing.md", + "contributor-docs/adrs/adr-011-snapshot-tests.md", + "contributor-docs/adrs/adr-017-interaction-tests-revisited.md" + ], + "rules": [ + { + "id": "testing.observable-behavior", + "enforcement": "enforce", + "check": "Unit tests assert observable semantics and behavior from the user perspective rather than implementation details.", + "prefer": "Use Testing Library roles, accessible names, userEvent, and explicit contract assertions.", + "source": "contributor-docs/testing.md" + }, + { + "id": "testing.no-catch-all-render-snapshots", + "enforcement": "enforce", + "check": "Tests do not add catch-all snapshots of rendered React trees.", + "prefer": "Reserve snapshots for static objects with a clear review signal, such as package export lists.", + "source": "contributor-docs/adrs/adr-011-snapshot-tests.md" + }, + { + "id": "testing.playwright-for-browser-behavior", + "enforcement": "enforce", + "check": "Behavior requiring real layout, browser interaction, visual regression, or automated accessibility coverage is tested through the Playwright e2e suite.", + "prefer": "Use @vrt and @avt coverage where visual appearance or accessibility output changes.", + "source": "contributor-docs/adrs/adr-017-interaction-tests-revisited.md" + }, + { + "id": "testing.public-contract-coverage", + "enforcement": "enforce", + "check": "New public props, forwarded DOM props, stable identifiers, keyboard behavior, and accessibility semantics have focused regression coverage.", + "prefer": "Name tests for the contract they protect.", + "source": "contributor-docs/testing.md" + } + ], + "sourceDigest": "c5645620dafd" + }, + { + "id": "css", + "title": "Component CSS", + "applyTo": "packages/react/src/**/*.css,packages/react/src/**/*.tsx", + "sources": [ + "contributor-docs/style.md", + "contributor-docs/authoring-css.md", + "contributor-docs/adrs/adr-016-css.md", + ".github/instructions/css.instructions.md" + ], + "rules": [ + { + "id": "css.modules", + "enforcement": "enforce", + "check": "Component styles use colocated CSS Modules and Primer primitive custom properties.", + "prefer": "Avoid new runtime-injected component styling and hardcoded design values.", + "source": "contributor-docs/adrs/adr-016-css.md" + }, + { + "id": "css.root-classname", + "enforcement": "enforce", + "check": "A consumer className is merged with clsx on the top-level element and cannot be overwritten by prop spread order.", + "prefer": "Use CSS custom properties instead of multiple child className escape hatches.", + "source": "contributor-docs/authoring-css.md" + }, + { + "id": "css.data-attributes-for-variants", + "enforcement": "enforce", + "check": "Component variants and state use data attributes or native pseudo-classes rather than modifier class names.", + "prefer": "Use :where() around data-attribute selectors to keep specificity low.", + "source": "contributor-docs/authoring-css.md" + }, + { + "id": "css.javascript-bridge", + "enforcement": "enforce", + "check": "Dynamic values crossing from JavaScript into CSS use custom properties instead of authored visual styles inline.", + "prefer": "Keep layout and visual rules in the CSS Module.", + "source": "contributor-docs/style.md" + }, + { + "id": "css.no-primitive-fallbacks", + "enforcement": "enforce", + "check": "Primer primitive custom properties are used without manually authored fallback values.", + "prefer": "Rely on the primitive build output to provide supported fallbacks.", + "source": "contributor-docs/authoring-css.md" + }, + { + "id": "css.browser-support", + "enforcement": "enforce", + "check": "CSS newer than Baseline 2022 includes an appropriate fallback and broadly matching selectors avoid expensive :has() invalidation.", + "prefer": "Scope necessary :has() usage to a CSS Module class with a justified local stylelint disable.", + "source": ".github/instructions/css.instructions.md" + } + ], + "sourceDigest": "043e538ad1a2" + }, + { + "id": "deprecations-and-versioning", + "title": "Deprecations, versioning, and changesets", + "applyTo": "packages/react/src/**/*,.changeset/*.md", + "sources": [ + "contributor-docs/deprecating-components.md", + "contributor-docs/versioning.md", + "contributor-docs/adrs/adr-019-deprecating-props.md", + ".github/skills/deprecations/SKILL.md", + ".github/skills/changesets/SKILL.md" + ], + "rules": [ + { + "id": "deprecations-and-versioning.complete-deprecation-signal", + "enforcement": "enforce", + "check": "Deprecations update JSDoc, docs metadata, migration guidance, and the recommended replacement where applicable.", + "prefer": "Keep runtime behavior unchanged unless the change explicitly includes migration behavior.", + "source": ".github/skills/deprecations/SKILL.md" + }, + { + "id": "deprecations-and-versioning.consumer-impact", + "enforcement": "enforce", + "check": "The changeset bump matches the largest consumer-facing API, behavior, CSS, accessibility, or stable-identifier impact.", + "prefer": "Use the repository versioning table and describe the outcome from the consumer perspective.", + "source": "contributor-docs/versioning.md" + }, + { + "id": "deprecations-and-versioning.changeset-required", + "enforcement": "enforce", + "check": "Published runtime or public API changes include an accurate changeset; docs-only, test-only, story-only, and infrastructure-only changes do not.", + "prefer": "Keep each changeset terse and scoped to one consumer-facing change.", + "source": ".github/skills/changesets/SKILL.md" + } + ], + "sourceDigest": "9d4c59fd70d0" + } + ] +} diff --git a/package.json b/package.json index fcaaf5ac39b..393f3997814 100644 --- a/package.json +++ b/package.json @@ -22,12 +22,16 @@ "build:docs:preview": "NODE_OPTIONS=--openssl-legacy-provider script/build-docs preview", "build:components.json": "npm run build:components.json -w @primer/react", "build:hooks.json": "npm run build:hooks.json -w @primer/react", + "build:review-guidance": "node script/review-guidance.mjs", + "accept:review-guidance-sources": "node script/review-guidance.mjs --accept-sources", + "check:review-guidance": "node script/review-guidance.mjs --check", "lint": "eslint '**/*.{js,ts,tsx,md,mdx}' --max-warnings=0 --cache", "lint:css": "stylelint --rd -q --cache '**/*.css'", "lint:css:fix": "npm run lint:css -- --fix", "lint:fix": "npm run lint -- --fix", "lint:md": "markdownlint-cli2 \"**/*.{md,mdx}\" \"!.github\" \"!.changeset\" \"!.agents\" \"!**/node_modules/**\" \"!**/CHANGELOG.md\"", "test": "vitest", + "test:review-guidance": "node --test script/review-guidance.test.mjs", "test:type-check": "tsc --noEmit", "test:update": "npm run test -- -u", "test:classname-coverage": "node script/check-classname-tests.mjs", diff --git a/script/review-guidance.mjs b/script/review-guidance.mjs new file mode 100644 index 00000000000..a73c23f79aa --- /dev/null +++ b/script/review-guidance.mjs @@ -0,0 +1,276 @@ +import {createHash} from 'node:crypto' +import {existsSync, mkdirSync, readFileSync, readdirSync, rmSync, writeFileSync} from 'node:fs' +import {dirname, join, relative, resolve} from 'node:path' +import {fileURLToPath, pathToFileURL} from 'node:url' + +const scriptDirectory = dirname(fileURLToPath(import.meta.url)) +const repositoryRoot = resolve(scriptDirectory, '..') +const publicPolicyPath = join(repositoryRoot, 'contributor-docs/review-guidance/policy.json') +const internalPolicyPath = join(repositoryRoot, 'contributor-docs/review-guidance/internal-policy.json') +const instructionsDirectory = join(repositoryRoot, '.github/instructions/generated') +const skillReferencePath = join(repositoryRoot, '.github/skills/primer-component-review/references/generated-policy.md') +const generatedMarker = '' + +function readJson(path) { + return JSON.parse(readFileSync(path, 'utf8')) +} + +export function sourceDigest(sourcePaths) { + const hash = createHash('sha256') + for (const sourcePath of sourcePaths) { + hash.update(sourcePath) + hash.update(readFileSync(join(repositoryRoot, sourcePath))) + } + return hash.digest('hex').slice(0, 12) +} + +function assertAllowedKeys(value, allowedKeys, label) { + for (const key of Object.keys(value)) { + assert(allowedKeys.includes(key), `${label} contains unsupported field: ${key}`) + } +} + +function assert(condition, message) { + if (!condition) { + throw new Error(message) + } +} + +export function validatePolicies(publicPolicy, internalPolicy) { + assertAllowedKeys(publicPolicy, ['version', 'ruleSets'], 'Public policy') + assertAllowedKeys(internalPolicy, ['version', 'ruleSets'], 'Internal policy') + assert(publicPolicy.version === 1, 'Public policy must use version 1') + assert(internalPolicy.version === 1, 'Internal policy must use version 1') + assert(Array.isArray(publicPolicy.ruleSets), 'Public policy must include ruleSets') + assert(Array.isArray(internalPolicy.ruleSets), 'Internal policy must include ruleSets') + + const ruleSetIds = new Set() + const ruleIds = new Set() + const supersededRuleIds = new Set() + const privateText = JSON.stringify(internalPolicy) + const forbiddenPrivatePatterns = [ + /https?:\/\//i, + /github\/primer/i, + /slack/i, + /figma/i, + /discussion/i, + /issue[#\s-]*\d+/i, + /rationale/i, + /alternative/i, + /consequence/i, + ] + + for (const pattern of forbiddenPrivatePatterns) { + assert(!pattern.test(privateText), `Internal policy contains forbidden private-source text: ${pattern}`) + } + + for (const [policyName, policy] of [ + ['public', publicPolicy], + ['internal', internalPolicy], + ]) { + for (const ruleSet of policy.ruleSets) { + assertAllowedKeys( + ruleSet, + policyName === 'public' + ? ['id', 'title', 'applyTo', 'sources', 'sourceDigest', 'rules'] + : ['id', 'title', 'applyTo', 'rules'], + `${policyName} rule set`, + ) + assert(typeof ruleSet.id === 'string' && ruleSet.id.length > 0, `${policyName} rule set needs an id`) + assert(!ruleSetIds.has(ruleSet.id), `Duplicate rule set id: ${ruleSet.id}`) + ruleSetIds.add(ruleSet.id) + assert(typeof ruleSet.title === 'string' && ruleSet.title.length > 0, `${ruleSet.id} needs a title`) + assert(typeof ruleSet.applyTo === 'string' && ruleSet.applyTo.length > 0, `${ruleSet.id} needs applyTo`) + assert(Array.isArray(ruleSet.rules) && ruleSet.rules.length > 0, `${ruleSet.id} needs rules`) + + if (policyName === 'public') { + assert(Array.isArray(ruleSet.sources) && ruleSet.sources.length > 0, `${ruleSet.id} needs sources`) + for (const source of ruleSet.sources) { + assert(existsSync(join(repositoryRoot, source)), `Missing source for ${ruleSet.id}: ${source}`) + } + assert( + ruleSet.sourceDigest === sourceDigest(ruleSet.sources), + `Sources changed for ${ruleSet.id}. Review its rules, then run npm run accept:review-guidance-sources.`, + ) + } else { + assert(ruleSet.sources === undefined, `Internal rule set ${ruleSet.id} must not expose sources`) + } + + for (const rule of ruleSet.rules) { + assertAllowedKeys( + rule, + policyName === 'public' + ? ['id', 'enforcement', 'check', 'prefer', 'exceptions', 'source', 'supersedes'] + : ['id', 'enforcement', 'check', 'prefer', 'exceptions', 'supersedes'], + `${ruleSet.id} rule`, + ) + assert(typeof rule.id === 'string' && rule.id.startsWith(`${ruleSet.id}.`), `Invalid rule id: ${rule.id}`) + assert(!ruleIds.has(rule.id), `Duplicate rule id: ${rule.id}`) + ruleIds.add(rule.id) + assert(['enforce', 'advisory'].includes(rule.enforcement), `Invalid enforcement for ${rule.id}`) + assert(typeof rule.check === 'string' && rule.check.length > 0, `${rule.id} needs a check`) + assert(typeof rule.prefer === 'string' && rule.prefer.length > 0, `${rule.id} needs a preference`) + if (rule.exceptions !== undefined) { + assert(Array.isArray(rule.exceptions), `${rule.id} exceptions must be an array`) + } + if (policyName === 'public') { + assert(ruleSet.sources.includes(rule.source), `${rule.id} needs an exact source from its rule set`) + } + if (rule.supersedes !== undefined) { + assert(Array.isArray(rule.supersedes), `${rule.id} supersedes must be an array`) + for (const supersededRuleId of rule.supersedes) { + assert(!supersededRuleIds.has(supersededRuleId), `${supersededRuleId} is superseded more than once`) + supersededRuleIds.add(supersededRuleId) + } + } + } + } + } + + for (const supersededRuleId of supersededRuleIds) { + assert(ruleIds.has(supersededRuleId), `Unknown superseded rule id: ${supersededRuleId}`) + } +} + +function mergeRuleSets(publicPolicy, internalPolicy) { + return [...publicPolicy.ruleSets, ...internalPolicy.ruleSets].sort((a, b) => a.id.localeCompare(b.id)) +} + +function renderRule(rule, sourceLabel) { + const lines = [ + `### \`${rule.id}\`${rule.enforcement === 'advisory' ? ' (advisory)' : ''}`, + '', + `- **Check:** ${rule.check}`, + `- **Prefer:** ${rule.prefer}`, + ] + + if (rule.exceptions?.length) { + lines.push(`- **Exceptions:** ${rule.exceptions.join(' ')}`) + } + + lines.push(`- **Authority:** ${sourceLabel}`, '') + return lines.join('\n') +} + +export function renderInstruction(ruleSet) { + const digest = ruleSet.sourceDigest ?? 'sanitized-import' + return [ + '---', + `applyTo: '${ruleSet.applyTo}'`, + '---', + '', + generatedMarker, + '', + `# ${ruleSet.title}`, + '', + `Policy set: \`${ruleSet.id}\`. Source digest: \`${digest}\`.`, + '', + 'Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse.', + '', + ...ruleSet.rules.map(rule => + renderRule(rule, rule.source ? `\`${rule.source}\`` : `Primer architecture policy: \`${rule.id}\``), + ), + ].join('\n') +} + +function renderSkillReference(ruleSets) { + return [ + generatedMarker, + '', + '# Primer component review policy', + '', + 'Apply only the policy sets relevant to the changed files. Enforced rules may produce findings. Advisory rules should only produce a finding when the change is designing or expanding the affected contract and the mismatch has a concrete consumer impact.', + '', + ...ruleSets.flatMap(ruleSet => { + return [ + `## ${ruleSet.title}`, + '', + `Applies to: \`${ruleSet.applyTo}\``, + '', + ...ruleSet.rules.map(rule => + renderRule(rule, rule.source ? `\`${rule.source}\`` : `Primer architecture policy: \`${rule.id}\``), + ), + ] + }), + ].join('\n') +} + +function expectedFiles(ruleSets) { + const files = new Map() + for (const ruleSet of ruleSets) { + files.set(join(instructionsDirectory, `${ruleSet.id}.instructions.md`), renderInstruction(ruleSet)) + } + files.set(skillReferencePath, renderSkillReference(ruleSets)) + return files +} + +function checkFiles(files) { + const staleFiles = [] + for (const [path, contents] of files) { + if (!existsSync(path) || readFileSync(path, 'utf8') !== `${contents}\n`) { + staleFiles.push(relative(repositoryRoot, path)) + } + } + + if (existsSync(instructionsDirectory)) { + for (const filename of readdirSync(instructionsDirectory)) { + const path = join(instructionsDirectory, filename) + if (filename.endsWith('.instructions.md') && !files.has(path)) { + staleFiles.push(relative(repositoryRoot, path)) + } + } + } + + if (staleFiles.length > 0) { + throw new Error( + `Generated review guidance is out of date:\n${staleFiles.map(path => `- ${path}`).join('\n')}\nRun npm run build:review-guidance.`, + ) + } +} + +function writeFiles(files) { + mkdirSync(instructionsDirectory, {recursive: true}) + mkdirSync(dirname(skillReferencePath), {recursive: true}) + + for (const filename of readdirSync(instructionsDirectory)) { + const path = join(instructionsDirectory, filename) + if (filename.endsWith('.instructions.md') && !files.has(path)) { + rmSync(path) + } + } + + for (const [path, contents] of files) { + writeFileSync(path, `${contents}\n`) + } +} + +function acceptSourceChanges(publicPolicy) { + for (const ruleSet of publicPolicy.ruleSets) { + ruleSet.sourceDigest = sourceDigest(ruleSet.sources) + } + writeFileSync(publicPolicyPath, `${JSON.stringify(publicPolicy, null, 2)}\n`) +} + +export function build({acceptSources = false, check = false} = {}) { + const publicPolicy = readJson(publicPolicyPath) + const internalPolicy = readJson(internalPolicyPath) + if (acceptSources) { + acceptSourceChanges(publicPolicy) + } + validatePolicies(publicPolicy, internalPolicy) + const files = expectedFiles(mergeRuleSets(publicPolicy, internalPolicy)) + + if (check) { + checkFiles(files) + } else { + writeFiles(files) + } +} + +const isMain = process.argv[1] && import.meta.url === pathToFileURL(resolve(process.argv[1])).href +if (isMain) { + build({ + acceptSources: process.argv.includes('--accept-sources'), + check: process.argv.includes('--check'), + }) +} diff --git a/script/review-guidance.test.mjs b/script/review-guidance.test.mjs new file mode 100644 index 00000000000..7c47d2c5f22 --- /dev/null +++ b/script/review-guidance.test.mjs @@ -0,0 +1,74 @@ +import assert from 'node:assert/strict' +import {test} from 'node:test' +import {renderInstruction, sourceDigest, validatePolicies} from './review-guidance.mjs' + +const validPublicPolicy = { + version: 1, + ruleSets: [ + { + id: 'example', + title: 'Example', + applyTo: 'packages/react/src/**/*.tsx', + sources: ['contributor-docs/style.md'], + sourceDigest: sourceDigest(['contributor-docs/style.md']), + rules: [ + { + id: 'example.rule', + enforcement: 'enforce', + check: 'Check the example.', + prefer: 'Prefer the supported example.', + source: 'contributor-docs/style.md', + }, + ], + }, + ], +} + +test('validates a public policy and sanitized internal import', () => { + assert.doesNotThrow(() => validatePolicies(validPublicPolicy, {version: 1, ruleSets: []})) +}) + +test('rejects private source details from imported policy', () => { + assert.throws( + () => + validatePolicies(validPublicPolicy, { + version: 1, + ruleSets: [ + { + id: 'private', + title: 'Private', + applyTo: '**/*.tsx', + rules: [ + { + id: 'private.rule', + enforcement: 'enforce', + check: 'See https://example.com for the decision.', + prefer: 'Use the decision.', + }, + ], + }, + ], + }), + /forbidden private-source text/, + ) +}) + +test('renders a path-scoped instruction with a rule citation', () => { + const output = renderInstruction(validPublicPolicy.ruleSets[0]) + assert.match(output, /applyTo: 'packages\/react\/src\/\*\*\/\*\.tsx'/) + assert.match(output, /`example\.rule`/) + assert.match(output, /contributor-docs\/style\.md/) +}) + +test('rejects undeclared fields from imported policy', () => { + assert.throws( + () => validatePolicies(validPublicPolicy, {version: 1, ruleSets: [], privateSource: 'hidden'}), + /unsupported field/, + ) +}) + +test('rejects public policy when a mapped source changed', () => { + const stalePolicy = structuredClone(validPublicPolicy) + stalePolicy.ruleSets[0].sourceDigest = 'stale' + assert.throws(() => validatePolicies(stalePolicy, {version: 1, ruleSets: []}), /Sources changed for example/) +}) From 59adc7ff8c9f9ab40bb72284fb2b3a99bdcf747d Mon Sep 17 00:00:00 2001 From: Adam Dierkens Date: Thu, 16 Jul 2026 15:53:09 -0400 Subject: [PATCH 2/6] Revert "Add ADR-backed component review guidance" This reverts commit c03b86abd03a82dbae30b50858e4fc15c2d479a9. --- .../agents/primer-component-reviewer.agent.md | 27 -- .../adr-review-guidance.instructions.md | 26 -- .../architecture-api-language.instructions.md | 24 -- ...architecture-button-layout.instructions.md | 18 - .../architecture-color.instructions.md | 36 -- ...architecture-notifications.instructions.md | 18 - .../architecture-select-panel.instructions.md | 43 --- .../generated/component-api.instructions.md | 90 ----- .../component-structure.instructions.md | 36 -- .../generated/css.instructions.md | 48 --- ...eprecations-and-versioning.instructions.md | 30 -- .../generated/stories.instructions.md | 36 -- .../generated/testing.instructions.md | 36 -- .../skills/primer-component-review/SKILL.md | 44 --- .../references/generated-policy.md | 333 ------------------ .github/workflows/ci.yml | 2 - .github/workflows/lint-autofix.yml | 4 - .prettierignore | 2 - AGENTS.md | 22 -- contributor-docs/adrs/adr-000-template.md | 8 - contributor-docs/review-guidance/README.md | 37 -- .../review-guidance/internal-policy.json | 121 ------- contributor-docs/review-guidance/policy.json | 325 ----------------- package.json | 4 - script/review-guidance.mjs | 276 --------------- script/review-guidance.test.mjs | 74 ---- 26 files changed, 1720 deletions(-) delete mode 100644 .github/agents/primer-component-reviewer.agent.md delete mode 100644 .github/instructions/adr-review-guidance.instructions.md delete mode 100644 .github/instructions/generated/architecture-api-language.instructions.md delete mode 100644 .github/instructions/generated/architecture-button-layout.instructions.md delete mode 100644 .github/instructions/generated/architecture-color.instructions.md delete mode 100644 .github/instructions/generated/architecture-notifications.instructions.md delete mode 100644 .github/instructions/generated/architecture-select-panel.instructions.md delete mode 100644 .github/instructions/generated/component-api.instructions.md delete mode 100644 .github/instructions/generated/component-structure.instructions.md delete mode 100644 .github/instructions/generated/css.instructions.md delete mode 100644 .github/instructions/generated/deprecations-and-versioning.instructions.md delete mode 100644 .github/instructions/generated/stories.instructions.md delete mode 100644 .github/instructions/generated/testing.instructions.md delete mode 100644 .github/skills/primer-component-review/SKILL.md delete mode 100644 .github/skills/primer-component-review/references/generated-policy.md delete mode 100644 AGENTS.md delete mode 100644 contributor-docs/review-guidance/README.md delete mode 100644 contributor-docs/review-guidance/internal-policy.json delete mode 100644 contributor-docs/review-guidance/policy.json delete mode 100644 script/review-guidance.mjs delete mode 100644 script/review-guidance.test.mjs diff --git a/.github/agents/primer-component-reviewer.agent.md b/.github/agents/primer-component-reviewer.agent.md deleted file mode 100644 index 107844ffdce..00000000000 --- a/.github/agents/primer-component-reviewer.agent.md +++ /dev/null @@ -1,27 +0,0 @@ ---- -name: primer-component-reviewer -description: Reviews Primer React component changes against accepted architectural decisions and maintained repository guidance. -tools: - - read - - search - - execute -skills: - - primer-component-review ---- - -You are a read-only reviewer for Primer React component changes. Never modify -files. - -Use the `primer-component-review` skill and its generated policy reference. -Review component APIs, implementation structure, accessibility, behavior, -stories, tests, CSS, deprecations, documentation metadata, exports, versioning, -and changesets when relevant to the diff. - -Report only concrete, actionable mismatches introduced or expanded by the -change. Cite the changed file and line and the governing policy rule ID. Explain -the impact and recommend the smallest design-level correction. - -Do not expose or speculate about private architectural sources. Rules labeled -`Primer architecture policy` are complete outcome-only guidance. - -If the change follows the applicable policy, say so directly. diff --git a/.github/instructions/adr-review-guidance.instructions.md b/.github/instructions/adr-review-guidance.instructions.md deleted file mode 100644 index 9b99342fd87..00000000000 --- a/.github/instructions/adr-review-guidance.instructions.md +++ /dev/null @@ -1,26 +0,0 @@ ---- -applyTo: 'contributor-docs/adrs/**/*.md,contributor-docs/CONTRIBUTING.md,contributor-docs/style.md,contributor-docs/testing.md,contributor-docs/authoring-css.md,contributor-docs/behaviors.md,contributor-docs/component-contents-api-patterns.md,contributor-docs/deprecating-components.md,contributor-docs/versioning.md' ---- - -# Maintaining component review guidance - -When an ADR or maintained contributor guide changes an actionable decision about -component APIs, implementation, accessibility, stories, testing, CSS, -deprecations, documentation, or versioning: - -1. Update, add, supersede, or remove the corresponding rule in - `contributor-docs/review-guidance/policy.json`. -2. Keep each rule outcome-focused and suitable for public review comments. Do not - copy private rationale or implementation history into a rule. -3. Use `enforce` only for accepted, current expectations. Use `advisory` for - guidance that should apply only while a related contract is being designed or - expanded. -4. Ensure each affected rule cites the exact changed source in its `source` - field. -5. Run `npm run accept:review-guidance-sources` only after reviewing all rules - mapped to the changed source. -6. Run `npm run build:review-guidance`. -7. Run `npm run check:review-guidance` and - `npm run test:review-guidance`. - -If the source change has no component-review impact, do not add a rule. diff --git a/.github/instructions/generated/architecture-api-language.instructions.md b/.github/instructions/generated/architecture-api-language.instructions.md deleted file mode 100644 index 542508f2689..00000000000 --- a/.github/instructions/generated/architecture-api-language.instructions.md +++ /dev/null @@ -1,24 +0,0 @@ ---- -applyTo: 'packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json,packages/react/src/**/*.hookDocs.json' ---- - - - -# Component API language - -Policy set: `architecture-api-language`. Source digest: `sanitized-import`. - -Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. - -### `architecture-api-language.semantic-size-names` (advisory) - -- **Check:** New public size, density, thickness, and viewport-range APIs use the established semantic vocabulary instead of abbreviations, CSS literals, or default as a size name. -- **Prefer:** Use descriptive full names and semantic values that remain meaningful across platforms. -- **Authority:** Primer architecture policy: `architecture-api-language.semantic-size-names` - -### `architecture-api-language.component-references` (advisory) - -- **Check:** Documentation and stories use the exported code name when referring to a specific React component and sentence case when discussing a generic interface pattern. -- **Prefer:** Keep API references unambiguous without presenting every generic pattern name as a code identifier. -- **Authority:** Primer architecture policy: `architecture-api-language.component-references` - diff --git a/.github/instructions/generated/architecture-button-layout.instructions.md b/.github/instructions/generated/architecture-button-layout.instructions.md deleted file mode 100644 index 1f711462fcb..00000000000 --- a/.github/instructions/generated/architecture-button-layout.instructions.md +++ /dev/null @@ -1,18 +0,0 @@ ---- -applyTo: 'packages/react/src/**/*Button*.tsx,packages/react/src/**/*Button*.css,packages/react/src/SelectPanel/**/*.tsx,packages/react/src/experimental/SelectPanel2/**/*.tsx' ---- - - - -# Button layout semantics - -Policy set: `architecture-button-layout`. Source digest: `sanitized-import`. - -Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. - -### `architecture-button-layout.full-width-alignment` (advisory) - -- **Check:** A full-width call-to-action centers its content, while a full-width selection trigger aligns its primary content to the start and keeps any trailing affordance at the end. -- **Prefer:** Use alignment to communicate whether the control performs an action or exposes a selection. -- **Authority:** Primer architecture policy: `architecture-button-layout.full-width-alignment` - diff --git a/.github/instructions/generated/architecture-color.instructions.md b/.github/instructions/generated/architecture-color.instructions.md deleted file mode 100644 index 4b8ddcdbfd5..00000000000 --- a/.github/instructions/generated/architecture-color.instructions.md +++ /dev/null @@ -1,36 +0,0 @@ ---- -applyTo: 'packages/react/src/**/*.module.css' ---- - - - -# Semantic color and contrast - -Policy set: `architecture-color`. Source digest: `sanitized-import`. - -Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. - -### `architecture-color.state-roles` (advisory) - -- **Check:** Open and closed states use their corresponding semantic functional color roles instead of success and danger roles. -- **Prefer:** Preserve the distinction between lifecycle state and validation or outcome semantics. -- **Authority:** Primer architecture policy: `architecture-color.state-roles` - -### `architecture-color.canonical-variants` - -- **Check:** The change does not introduce deprecated subtle functional color variants. -- **Prefer:** Use muted for secondary or less-prominent treatment and emphasis for stronger treatment. -- **Authority:** Primer architecture policy: `architecture-color.canonical-variants` - -### `architecture-color.text-and-links` (advisory) - -- **Check:** Non-decorative text and placeholders maintain at least 4.5:1 contrast against every supported background, and links remain distinguishable from surrounding text without relying only on color. -- **Prefer:** Use supported token pairs and retain a visible non-color link affordance. -- **Authority:** Primer architecture policy: `architecture-color.text-and-links` - -### `architecture-color.focus-and-state` (advisory) - -- **Check:** Focus indicators and simultaneously visible interactive states maintain at least 3:1 contrast against adjacent colors. -- **Prefer:** Verify contrast across supported themes without imposing a contrast requirement on purely decorative or inactive control borders. -- **Authority:** Primer architecture policy: `architecture-color.focus-and-state` - diff --git a/.github/instructions/generated/architecture-notifications.instructions.md b/.github/instructions/generated/architecture-notifications.instructions.md deleted file mode 100644 index 28664c4cf3c..00000000000 --- a/.github/instructions/generated/architecture-notifications.instructions.md +++ /dev/null @@ -1,18 +0,0 @@ ---- -applyTo: 'packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json' ---- - - - -# Notification patterns - -Policy set: `architecture-notifications`. Source digest: `sanitized-import`. - -Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. - -### `architecture-notifications.no-toast-pattern` - -- **Check:** The change does not add, restore, or recommend a toast component or toast recipe. -- **Prefer:** Use a supported feedback pattern appropriate to the interaction, such as a banner, inline validation, inline message, or dialog. -- **Authority:** Primer architecture policy: `architecture-notifications.no-toast-pattern` - diff --git a/.github/instructions/generated/architecture-select-panel.instructions.md b/.github/instructions/generated/architecture-select-panel.instructions.md deleted file mode 100644 index 4b1e8ae2a62..00000000000 --- a/.github/instructions/generated/architecture-select-panel.instructions.md +++ /dev/null @@ -1,43 +0,0 @@ ---- -applyTo: 'packages/react/src/SelectPanel/**/*' ---- - - - -# SelectPanel interaction contract - -Policy set: `architecture-select-panel`. Source digest: `sanitized-import`. - -Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. - -### `architecture-select-panel.focus-restoration` - -- **Check:** Opening moves focus to the configured initial target and closing restores focus to the trigger. -- **Prefer:** Focus the filter input by default while preserving explicit deferred-focus behavior and the component's established secondary-focus semantics. -- **Authority:** Primer architecture policy: `architecture-select-panel.focus-restoration` - -### `architecture-select-panel.commit-behavior` - -- **Check:** Anchored dismissal preserves the latest selection, while modal Save finalizes changes and modal cancellation or dismissal invokes the cancellation contract. -- **Prefer:** Preserve distinct anchored and modal commit semantics across every close path so consumers can restore the prior committed modal selection. -- **Authority:** Primer architecture policy: `architecture-select-panel.commit-behavior` - -### `architecture-select-panel.filter-focus` - -- **Check:** Input focus and filtering preserve a valid active option whenever option activation is enabled. -- **Prefer:** Keep the input and list behavior connected and normally activate the first available option after filtering. -- **Exceptions:** An explicit deferred-initial-focus configuration may wait for user interaction before assigning the active option. -- **Authority:** Primer architecture policy: `architecture-select-panel.filter-focus` - -### `architecture-select-panel.keyboard-navigation` - -- **Check:** Keyboard behavior supports toggling the active option, sequential tab navigation, directional option navigation, first and last option shortcuts, and Escape dismissal. -- **Prefer:** Maintain the complete interaction contract instead of adding isolated key handlers. -- **Authority:** Primer architecture policy: `architecture-select-panel.keyboard-navigation` - -### `architecture-select-panel.announcements` - -- **Check:** Screen-reader announcements communicate the active option, its position, and selection state without announcing on every filtering keystroke. -- **Prefer:** Use polite, delayed announcements and debounce filtering updates. -- **Authority:** Primer architecture policy: `architecture-select-panel.announcements` - diff --git a/.github/instructions/generated/component-api.instructions.md b/.github/instructions/generated/component-api.instructions.md deleted file mode 100644 index cce228c00c1..00000000000 --- a/.github/instructions/generated/component-api.instructions.md +++ /dev/null @@ -1,90 +0,0 @@ ---- -applyTo: 'packages/react/src/**/*.ts,packages/react/src/**/*.tsx' ---- - - - -# Component API and implementation - -Policy set: `component-api`. Source digest: `e2ac6c7a9525`. - -Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. - -### `component-api.spectrum-of-abstraction` - -- **Check:** New component APIs begin with flexible presentational components and behavior hooks unless the product pattern and its defaults are already stable. -- **Prefer:** Build config components by composing established presentational parts and hooks instead of duplicating their behavior or markup. -- **Authority:** `contributor-docs/style.md` - -### `component-api.children-for-content` - -- **Check:** Content uses React children when consumers own the rendered elements or need free-form composition. -- **Prefer:** Use data props when the component must own, transform, order, or constrain the rendered elements. Do not support equivalent children and data APIs simultaneously without a concrete need. -- **Authority:** `contributor-docs/component-contents-api-patterns.md` - -### `component-api.no-speculative-parts` - -- **Check:** Compound parts, render props, child-prop pass-through APIs, and escape hatches address demonstrated use cases rather than speculative flexibility. -- **Prefer:** Start with the smallest API that supports known scenarios and open it up when a concrete need appears. -- **Authority:** `contributor-docs/adrs/adr-004-children-as-api.md` - -### `component-api.avoid-slots-by-default` - -- **Check:** Slots, useSlots, and __SLOT__ markers are only introduced when a parent must identify a specific child or the requested API requires child extraction. -- **Prefer:** Use normal React composition and preserve consumer-authored child order. -- **Authority:** `contributor-docs/style.md` - -### `component-api.reuse-base-primitives` - -- **Check:** Components reuse existing base components, accessibility primitives, hooks, and utilities before introducing parallel implementations. -- **Prefer:** Use primitives such as ButtonBase for Primer-owned native semantics, interactions, and reset styling. -- **Authority:** `contributor-docs/style.md` - -### `component-api.controlled-state` - -- **Check:** Components supporting controlled and uncontrolled state use the shared useControllableState pattern. -- **Prefer:** Keep controlled value, default value, and change callback behavior aligned with the repository helper. -- **Authority:** `contributor-docs/style.md` - -### `component-api.root-rest-props` - -- **Check:** Forwarded rest props and className are applied to the component root unless the public contract explicitly identifies another element. -- **Prefer:** Test that forwarded props reach the documented element. -- **Authority:** `contributor-docs/style.md` - -### `component-api.extensible-callbacks` - -- **Check:** New public callback props use a single object argument when the callback payload may grow. -- **Prefer:** Use a named object payload instead of positional arguments. -- **Authority:** `contributor-docs/style.md` - -### `component-api.ssr` - -- **Check:** Render paths avoid browser-only APIs and hydration-dependent layout changes. -- **Prefer:** Progressively enhance after hydration and move browser access into appropriate effects or event handlers. -- **Authority:** `contributor-docs/style.md` - -### `component-api.focus-from-events` - -- **Check:** Focus moves as a direct result of user events rather than unrelated effect dependency changes. -- **Prefer:** Manage focus in the event handler that caused the transition. -- **Authority:** `contributor-docs/style.md` - -### `component-api.hook-ref-input` - -- **Check:** Hooks that operate on a caller-owned DOM node accept a ref instead of creating and returning an incompatible ref. -- **Prefer:** Let callers compose multiple behaviors around the same ref. -- **Authority:** `contributor-docs/style.md` - -### `component-api.stable-hook-callbacks` - -- **Check:** Hook effects do not repeatedly resubscribe solely because a callback argument changed identity. -- **Prefer:** Use the repository stable-callback pattern so subscriptions always call the latest callback. -- **Authority:** `contributor-docs/style.md` - -### `component-api.public-hooks-need-consumers` - -- **Check:** New public hooks represent a clear consumer capability rather than exposing subcomponent implementation details. -- **Prefer:** Keep behavior hooks internal until a supported external use case and complete public contract exist. -- **Authority:** `contributor-docs/style.md` - diff --git a/.github/instructions/generated/component-structure.instructions.md b/.github/instructions/generated/component-structure.instructions.md deleted file mode 100644 index 9e9ab732705..00000000000 --- a/.github/instructions/generated/component-structure.instructions.md +++ /dev/null @@ -1,36 +0,0 @@ ---- -applyTo: 'packages/react/src/**/*' ---- - - - -# Component files and public surface - -Policy set: `component-structure`. Source digest: `21badb7273eb`. - -Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. - -### `component-structure.colocate-files` - -- **Check:** New components use a PascalCase directory and colocate implementation, CSS, tests, stories, docs metadata, and exports according to repository conventions. -- **Prefer:** Name subcomponent files with the parent component prefix for discoverability. -- **Authority:** `contributor-docs/adrs/adr-013-file-structure.md` - -### `component-structure.internal-modules` - -- **Check:** New shared modules that are not public API live under packages/react/src/internal and are not exported from public entrypoints. -- **Prefer:** Keep implementation details internal until a supported public API is intentionally designed. -- **Authority:** `contributor-docs/adrs/adr-015-internal-modules.md` - -### `component-structure.stable-identifiers` (advisory) - -- **Check:** New or changed component roots, public subcomponents, and meaningful structural parts follow the established data-component naming contract. -- **Prefer:** Use PascalCase component API names, keep state in separate data attributes, and test stable identifier values. -- **Authority:** `contributor-docs/adrs/adr-023-stable-selectors-api.md` - -### `component-structure.complete-public-surface` - -- **Check:** Public API changes update exports, export snapshots, docs metadata, tests, stories, and changesets as applicable. -- **Prefer:** Treat all public surfaces as one component contract. -- **Authority:** `contributor-docs/CONTRIBUTING.md` - diff --git a/.github/instructions/generated/css.instructions.md b/.github/instructions/generated/css.instructions.md deleted file mode 100644 index 0247be59d82..00000000000 --- a/.github/instructions/generated/css.instructions.md +++ /dev/null @@ -1,48 +0,0 @@ ---- -applyTo: 'packages/react/src/**/*.css,packages/react/src/**/*.tsx' ---- - - - -# Component CSS - -Policy set: `css`. Source digest: `043e538ad1a2`. - -Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. - -### `css.modules` - -- **Check:** Component styles use colocated CSS Modules and Primer primitive custom properties. -- **Prefer:** Avoid new runtime-injected component styling and hardcoded design values. -- **Authority:** `contributor-docs/adrs/adr-016-css.md` - -### `css.root-classname` - -- **Check:** A consumer className is merged with clsx on the top-level element and cannot be overwritten by prop spread order. -- **Prefer:** Use CSS custom properties instead of multiple child className escape hatches. -- **Authority:** `contributor-docs/authoring-css.md` - -### `css.data-attributes-for-variants` - -- **Check:** Component variants and state use data attributes or native pseudo-classes rather than modifier class names. -- **Prefer:** Use :where() around data-attribute selectors to keep specificity low. -- **Authority:** `contributor-docs/authoring-css.md` - -### `css.javascript-bridge` - -- **Check:** Dynamic values crossing from JavaScript into CSS use custom properties instead of authored visual styles inline. -- **Prefer:** Keep layout and visual rules in the CSS Module. -- **Authority:** `contributor-docs/style.md` - -### `css.no-primitive-fallbacks` - -- **Check:** Primer primitive custom properties are used without manually authored fallback values. -- **Prefer:** Rely on the primitive build output to provide supported fallbacks. -- **Authority:** `contributor-docs/authoring-css.md` - -### `css.browser-support` - -- **Check:** CSS newer than Baseline 2022 includes an appropriate fallback and broadly matching selectors avoid expensive :has() invalidation. -- **Prefer:** Scope necessary :has() usage to a CSS Module class with a justified local stylelint disable. -- **Authority:** `.github/instructions/css.instructions.md` - diff --git a/.github/instructions/generated/deprecations-and-versioning.instructions.md b/.github/instructions/generated/deprecations-and-versioning.instructions.md deleted file mode 100644 index 7c6a00072c2..00000000000 --- a/.github/instructions/generated/deprecations-and-versioning.instructions.md +++ /dev/null @@ -1,30 +0,0 @@ ---- -applyTo: 'packages/react/src/**/*,.changeset/*.md' ---- - - - -# Deprecations, versioning, and changesets - -Policy set: `deprecations-and-versioning`. Source digest: `9d4c59fd70d0`. - -Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. - -### `deprecations-and-versioning.complete-deprecation-signal` - -- **Check:** Deprecations update JSDoc, docs metadata, migration guidance, and the recommended replacement where applicable. -- **Prefer:** Keep runtime behavior unchanged unless the change explicitly includes migration behavior. -- **Authority:** `.github/skills/deprecations/SKILL.md` - -### `deprecations-and-versioning.consumer-impact` - -- **Check:** The changeset bump matches the largest consumer-facing API, behavior, CSS, accessibility, or stable-identifier impact. -- **Prefer:** Use the repository versioning table and describe the outcome from the consumer perspective. -- **Authority:** `contributor-docs/versioning.md` - -### `deprecations-and-versioning.changeset-required` - -- **Check:** Published runtime or public API changes include an accurate changeset; docs-only, test-only, story-only, and infrastructure-only changes do not. -- **Prefer:** Keep each changeset terse and scoped to one consumer-facing change. -- **Authority:** `.github/skills/changesets/SKILL.md` - diff --git a/.github/instructions/generated/stories.instructions.md b/.github/instructions/generated/stories.instructions.md deleted file mode 100644 index d049e4bea82..00000000000 --- a/.github/instructions/generated/stories.instructions.md +++ /dev/null @@ -1,36 +0,0 @@ ---- -applyTo: 'packages/react/src/**/*.stories.tsx' ---- - - - -# Storybook stories - -Policy set: `stories`. Source digest: `ce5b186845a5`. - -Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. - -### `stories.default-and-playground` - -- **Check:** The default story file uses the Components/ComponentName title and includes Default and Playground when configurable controls are supported. -- **Prefer:** Keep the primary documentation stories in ComponentName.stories.tsx. -- **Authority:** `contributor-docs/CONTRIBUTING.md` - -### `stories.separate-features-and-examples` - -- **Check:** Feature coverage and scenario examples are separated from the default story file. -- **Prefer:** Use ComponentName.features.stories.tsx for features and ComponentName.examples.stories.tsx for distinct scenarios. -- **Authority:** `contributor-docs/CONTRIBUTING.md` - -### `stories.document-supported-api` - -- **Check:** Stories demonstrate supported props, variants, and composition rather than inventing undocumented APIs. -- **Prefer:** Keep stories aligned with component docs metadata and real consumer-facing behavior. -- **Authority:** `.github/skills/storybook/SKILL.md` - -### `stories.keep-interactions-in-tests` - -- **Check:** Stories do not add play functions as a substitute for interaction coverage. -- **Prefer:** Exercise browser interactions through Playwright against the story and keep unit-level interactions in focused tests. -- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` - diff --git a/.github/instructions/generated/testing.instructions.md b/.github/instructions/generated/testing.instructions.md deleted file mode 100644 index fe2c0b0e4e9..00000000000 --- a/.github/instructions/generated/testing.instructions.md +++ /dev/null @@ -1,36 +0,0 @@ ---- -applyTo: 'packages/react/src/**/*.test.ts,packages/react/src/**/*.test.tsx,e2e/**/*.test.ts' ---- - - - -# Component testing - -Policy set: `testing`. Source digest: `c5645620dafd`. - -Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. - -### `testing.observable-behavior` - -- **Check:** Unit tests assert observable semantics and behavior from the user perspective rather than implementation details. -- **Prefer:** Use Testing Library roles, accessible names, userEvent, and explicit contract assertions. -- **Authority:** `contributor-docs/testing.md` - -### `testing.no-catch-all-render-snapshots` - -- **Check:** Tests do not add catch-all snapshots of rendered React trees. -- **Prefer:** Reserve snapshots for static objects with a clear review signal, such as package export lists. -- **Authority:** `contributor-docs/adrs/adr-011-snapshot-tests.md` - -### `testing.playwright-for-browser-behavior` - -- **Check:** Behavior requiring real layout, browser interaction, visual regression, or automated accessibility coverage is tested through the Playwright e2e suite. -- **Prefer:** Use @vrt and @avt coverage where visual appearance or accessibility output changes. -- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` - -### `testing.public-contract-coverage` - -- **Check:** New public props, forwarded DOM props, stable identifiers, keyboard behavior, and accessibility semantics have focused regression coverage. -- **Prefer:** Name tests for the contract they protect. -- **Authority:** `contributor-docs/testing.md` - diff --git a/.github/skills/primer-component-review/SKILL.md b/.github/skills/primer-component-review/SKILL.md deleted file mode 100644 index 64e271323f2..00000000000 --- a/.github/skills/primer-component-review/SKILL.md +++ /dev/null @@ -1,44 +0,0 @@ ---- -name: primer-component-review -description: 'Use when: reviewing Primer React component changes for repository architectural decisions and maintained component guidance. Covers component APIs, implementation structure, accessibility, stories, testing, CSS, deprecations, versioning, and changesets.' ---- - -# Primer component review - -Use this skill for code review of Primer React components and their supporting -files. - -## Sources - -Read `references/generated-policy.md` and apply only the policy sets relevant to -the changed files. Path-specific generated instructions under -`.github/instructions/generated/` contain the same rules scoped for Copilot code -review. - -Public ADRs and maintained contributor guides are authoritative. Rules labeled -`Primer architecture policy` contain only public-safe outcomes imported from a -private decision source. Do not seek, infer, mention, or disclose the private -source or rationale. - -## Review procedure - -1. Determine which public contracts and behaviors the change adds, removes, or - modifies. -2. Select the relevant policy sets from the generated reference. -3. Compare changed code and directly related tests, stories, docs, exports, and - changesets with enforced rules. -4. Apply advisory rules only when the change is designing or expanding the - affected contract and the mismatch has concrete consumer impact. -5. Report only findings introduced or made worse by the change. - -For each finding: - -- cite the changed file and line; -- cite the policy rule ID; -- describe the concrete consumer, accessibility, maintenance, or compatibility - impact; -- suggest the smallest design-level correction. - -Do not report formatting issues, preferences without policy support, or -pre-existing migration debt. If two active rules conflict, report the policy -conflict instead of choosing one. diff --git a/.github/skills/primer-component-review/references/generated-policy.md b/.github/skills/primer-component-review/references/generated-policy.md deleted file mode 100644 index c105a9ad90c..00000000000 --- a/.github/skills/primer-component-review/references/generated-policy.md +++ /dev/null @@ -1,333 +0,0 @@ - - -# Primer component review policy - -Apply only the policy sets relevant to the changed files. Enforced rules may produce findings. Advisory rules should only produce a finding when the change is designing or expanding the affected contract and the mismatch has a concrete consumer impact. - -## Component API language - -Applies to: `packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json,packages/react/src/**/*.hookDocs.json` - -### `architecture-api-language.semantic-size-names` (advisory) - -- **Check:** New public size, density, thickness, and viewport-range APIs use the established semantic vocabulary instead of abbreviations, CSS literals, or default as a size name. -- **Prefer:** Use descriptive full names and semantic values that remain meaningful across platforms. -- **Authority:** Primer architecture policy: `architecture-api-language.semantic-size-names` - -### `architecture-api-language.component-references` (advisory) - -- **Check:** Documentation and stories use the exported code name when referring to a specific React component and sentence case when discussing a generic interface pattern. -- **Prefer:** Keep API references unambiguous without presenting every generic pattern name as a code identifier. -- **Authority:** Primer architecture policy: `architecture-api-language.component-references` - -## Button layout semantics - -Applies to: `packages/react/src/**/*Button*.tsx,packages/react/src/**/*Button*.css,packages/react/src/SelectPanel/**/*.tsx,packages/react/src/experimental/SelectPanel2/**/*.tsx` - -### `architecture-button-layout.full-width-alignment` (advisory) - -- **Check:** A full-width call-to-action centers its content, while a full-width selection trigger aligns its primary content to the start and keeps any trailing affordance at the end. -- **Prefer:** Use alignment to communicate whether the control performs an action or exposes a selection. -- **Authority:** Primer architecture policy: `architecture-button-layout.full-width-alignment` - -## Semantic color and contrast - -Applies to: `packages/react/src/**/*.module.css` - -### `architecture-color.state-roles` (advisory) - -- **Check:** Open and closed states use their corresponding semantic functional color roles instead of success and danger roles. -- **Prefer:** Preserve the distinction between lifecycle state and validation or outcome semantics. -- **Authority:** Primer architecture policy: `architecture-color.state-roles` - -### `architecture-color.canonical-variants` - -- **Check:** The change does not introduce deprecated subtle functional color variants. -- **Prefer:** Use muted for secondary or less-prominent treatment and emphasis for stronger treatment. -- **Authority:** Primer architecture policy: `architecture-color.canonical-variants` - -### `architecture-color.text-and-links` (advisory) - -- **Check:** Non-decorative text and placeholders maintain at least 4.5:1 contrast against every supported background, and links remain distinguishable from surrounding text without relying only on color. -- **Prefer:** Use supported token pairs and retain a visible non-color link affordance. -- **Authority:** Primer architecture policy: `architecture-color.text-and-links` - -### `architecture-color.focus-and-state` (advisory) - -- **Check:** Focus indicators and simultaneously visible interactive states maintain at least 3:1 contrast against adjacent colors. -- **Prefer:** Verify contrast across supported themes without imposing a contrast requirement on purely decorative or inactive control borders. -- **Authority:** Primer architecture policy: `architecture-color.focus-and-state` - -## Notification patterns - -Applies to: `packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json` - -### `architecture-notifications.no-toast-pattern` - -- **Check:** The change does not add, restore, or recommend a toast component or toast recipe. -- **Prefer:** Use a supported feedback pattern appropriate to the interaction, such as a banner, inline validation, inline message, or dialog. -- **Authority:** Primer architecture policy: `architecture-notifications.no-toast-pattern` - -## SelectPanel interaction contract - -Applies to: `packages/react/src/SelectPanel/**/*` - -### `architecture-select-panel.focus-restoration` - -- **Check:** Opening moves focus to the configured initial target and closing restores focus to the trigger. -- **Prefer:** Focus the filter input by default while preserving explicit deferred-focus behavior and the component's established secondary-focus semantics. -- **Authority:** Primer architecture policy: `architecture-select-panel.focus-restoration` - -### `architecture-select-panel.commit-behavior` - -- **Check:** Anchored dismissal preserves the latest selection, while modal Save finalizes changes and modal cancellation or dismissal invokes the cancellation contract. -- **Prefer:** Preserve distinct anchored and modal commit semantics across every close path so consumers can restore the prior committed modal selection. -- **Authority:** Primer architecture policy: `architecture-select-panel.commit-behavior` - -### `architecture-select-panel.filter-focus` - -- **Check:** Input focus and filtering preserve a valid active option whenever option activation is enabled. -- **Prefer:** Keep the input and list behavior connected and normally activate the first available option after filtering. -- **Exceptions:** An explicit deferred-initial-focus configuration may wait for user interaction before assigning the active option. -- **Authority:** Primer architecture policy: `architecture-select-panel.filter-focus` - -### `architecture-select-panel.keyboard-navigation` - -- **Check:** Keyboard behavior supports toggling the active option, sequential tab navigation, directional option navigation, first and last option shortcuts, and Escape dismissal. -- **Prefer:** Maintain the complete interaction contract instead of adding isolated key handlers. -- **Authority:** Primer architecture policy: `architecture-select-panel.keyboard-navigation` - -### `architecture-select-panel.announcements` - -- **Check:** Screen-reader announcements communicate the active option, its position, and selection state without announcing on every filtering keystroke. -- **Prefer:** Use polite, delayed announcements and debounce filtering updates. -- **Authority:** Primer architecture policy: `architecture-select-panel.announcements` - -## Component API and implementation - -Applies to: `packages/react/src/**/*.ts,packages/react/src/**/*.tsx` - -### `component-api.spectrum-of-abstraction` - -- **Check:** New component APIs begin with flexible presentational components and behavior hooks unless the product pattern and its defaults are already stable. -- **Prefer:** Build config components by composing established presentational parts and hooks instead of duplicating their behavior or markup. -- **Authority:** `contributor-docs/style.md` - -### `component-api.children-for-content` - -- **Check:** Content uses React children when consumers own the rendered elements or need free-form composition. -- **Prefer:** Use data props when the component must own, transform, order, or constrain the rendered elements. Do not support equivalent children and data APIs simultaneously without a concrete need. -- **Authority:** `contributor-docs/component-contents-api-patterns.md` - -### `component-api.no-speculative-parts` - -- **Check:** Compound parts, render props, child-prop pass-through APIs, and escape hatches address demonstrated use cases rather than speculative flexibility. -- **Prefer:** Start with the smallest API that supports known scenarios and open it up when a concrete need appears. -- **Authority:** `contributor-docs/adrs/adr-004-children-as-api.md` - -### `component-api.avoid-slots-by-default` - -- **Check:** Slots, useSlots, and __SLOT__ markers are only introduced when a parent must identify a specific child or the requested API requires child extraction. -- **Prefer:** Use normal React composition and preserve consumer-authored child order. -- **Authority:** `contributor-docs/style.md` - -### `component-api.reuse-base-primitives` - -- **Check:** Components reuse existing base components, accessibility primitives, hooks, and utilities before introducing parallel implementations. -- **Prefer:** Use primitives such as ButtonBase for Primer-owned native semantics, interactions, and reset styling. -- **Authority:** `contributor-docs/style.md` - -### `component-api.controlled-state` - -- **Check:** Components supporting controlled and uncontrolled state use the shared useControllableState pattern. -- **Prefer:** Keep controlled value, default value, and change callback behavior aligned with the repository helper. -- **Authority:** `contributor-docs/style.md` - -### `component-api.root-rest-props` - -- **Check:** Forwarded rest props and className are applied to the component root unless the public contract explicitly identifies another element. -- **Prefer:** Test that forwarded props reach the documented element. -- **Authority:** `contributor-docs/style.md` - -### `component-api.extensible-callbacks` - -- **Check:** New public callback props use a single object argument when the callback payload may grow. -- **Prefer:** Use a named object payload instead of positional arguments. -- **Authority:** `contributor-docs/style.md` - -### `component-api.ssr` - -- **Check:** Render paths avoid browser-only APIs and hydration-dependent layout changes. -- **Prefer:** Progressively enhance after hydration and move browser access into appropriate effects or event handlers. -- **Authority:** `contributor-docs/style.md` - -### `component-api.focus-from-events` - -- **Check:** Focus moves as a direct result of user events rather than unrelated effect dependency changes. -- **Prefer:** Manage focus in the event handler that caused the transition. -- **Authority:** `contributor-docs/style.md` - -### `component-api.hook-ref-input` - -- **Check:** Hooks that operate on a caller-owned DOM node accept a ref instead of creating and returning an incompatible ref. -- **Prefer:** Let callers compose multiple behaviors around the same ref. -- **Authority:** `contributor-docs/style.md` - -### `component-api.stable-hook-callbacks` - -- **Check:** Hook effects do not repeatedly resubscribe solely because a callback argument changed identity. -- **Prefer:** Use the repository stable-callback pattern so subscriptions always call the latest callback. -- **Authority:** `contributor-docs/style.md` - -### `component-api.public-hooks-need-consumers` - -- **Check:** New public hooks represent a clear consumer capability rather than exposing subcomponent implementation details. -- **Prefer:** Keep behavior hooks internal until a supported external use case and complete public contract exist. -- **Authority:** `contributor-docs/style.md` - -## Component files and public surface - -Applies to: `packages/react/src/**/*` - -### `component-structure.colocate-files` - -- **Check:** New components use a PascalCase directory and colocate implementation, CSS, tests, stories, docs metadata, and exports according to repository conventions. -- **Prefer:** Name subcomponent files with the parent component prefix for discoverability. -- **Authority:** `contributor-docs/adrs/adr-013-file-structure.md` - -### `component-structure.internal-modules` - -- **Check:** New shared modules that are not public API live under packages/react/src/internal and are not exported from public entrypoints. -- **Prefer:** Keep implementation details internal until a supported public API is intentionally designed. -- **Authority:** `contributor-docs/adrs/adr-015-internal-modules.md` - -### `component-structure.stable-identifiers` (advisory) - -- **Check:** New or changed component roots, public subcomponents, and meaningful structural parts follow the established data-component naming contract. -- **Prefer:** Use PascalCase component API names, keep state in separate data attributes, and test stable identifier values. -- **Authority:** `contributor-docs/adrs/adr-023-stable-selectors-api.md` - -### `component-structure.complete-public-surface` - -- **Check:** Public API changes update exports, export snapshots, docs metadata, tests, stories, and changesets as applicable. -- **Prefer:** Treat all public surfaces as one component contract. -- **Authority:** `contributor-docs/CONTRIBUTING.md` - -## Component CSS - -Applies to: `packages/react/src/**/*.css,packages/react/src/**/*.tsx` - -### `css.modules` - -- **Check:** Component styles use colocated CSS Modules and Primer primitive custom properties. -- **Prefer:** Avoid new runtime-injected component styling and hardcoded design values. -- **Authority:** `contributor-docs/adrs/adr-016-css.md` - -### `css.root-classname` - -- **Check:** A consumer className is merged with clsx on the top-level element and cannot be overwritten by prop spread order. -- **Prefer:** Use CSS custom properties instead of multiple child className escape hatches. -- **Authority:** `contributor-docs/authoring-css.md` - -### `css.data-attributes-for-variants` - -- **Check:** Component variants and state use data attributes or native pseudo-classes rather than modifier class names. -- **Prefer:** Use :where() around data-attribute selectors to keep specificity low. -- **Authority:** `contributor-docs/authoring-css.md` - -### `css.javascript-bridge` - -- **Check:** Dynamic values crossing from JavaScript into CSS use custom properties instead of authored visual styles inline. -- **Prefer:** Keep layout and visual rules in the CSS Module. -- **Authority:** `contributor-docs/style.md` - -### `css.no-primitive-fallbacks` - -- **Check:** Primer primitive custom properties are used without manually authored fallback values. -- **Prefer:** Rely on the primitive build output to provide supported fallbacks. -- **Authority:** `contributor-docs/authoring-css.md` - -### `css.browser-support` - -- **Check:** CSS newer than Baseline 2022 includes an appropriate fallback and broadly matching selectors avoid expensive :has() invalidation. -- **Prefer:** Scope necessary :has() usage to a CSS Module class with a justified local stylelint disable. -- **Authority:** `.github/instructions/css.instructions.md` - -## Deprecations, versioning, and changesets - -Applies to: `packages/react/src/**/*,.changeset/*.md` - -### `deprecations-and-versioning.complete-deprecation-signal` - -- **Check:** Deprecations update JSDoc, docs metadata, migration guidance, and the recommended replacement where applicable. -- **Prefer:** Keep runtime behavior unchanged unless the change explicitly includes migration behavior. -- **Authority:** `.github/skills/deprecations/SKILL.md` - -### `deprecations-and-versioning.consumer-impact` - -- **Check:** The changeset bump matches the largest consumer-facing API, behavior, CSS, accessibility, or stable-identifier impact. -- **Prefer:** Use the repository versioning table and describe the outcome from the consumer perspective. -- **Authority:** `contributor-docs/versioning.md` - -### `deprecations-and-versioning.changeset-required` - -- **Check:** Published runtime or public API changes include an accurate changeset; docs-only, test-only, story-only, and infrastructure-only changes do not. -- **Prefer:** Keep each changeset terse and scoped to one consumer-facing change. -- **Authority:** `.github/skills/changesets/SKILL.md` - -## Storybook stories - -Applies to: `packages/react/src/**/*.stories.tsx` - -### `stories.default-and-playground` - -- **Check:** The default story file uses the Components/ComponentName title and includes Default and Playground when configurable controls are supported. -- **Prefer:** Keep the primary documentation stories in ComponentName.stories.tsx. -- **Authority:** `contributor-docs/CONTRIBUTING.md` - -### `stories.separate-features-and-examples` - -- **Check:** Feature coverage and scenario examples are separated from the default story file. -- **Prefer:** Use ComponentName.features.stories.tsx for features and ComponentName.examples.stories.tsx for distinct scenarios. -- **Authority:** `contributor-docs/CONTRIBUTING.md` - -### `stories.document-supported-api` - -- **Check:** Stories demonstrate supported props, variants, and composition rather than inventing undocumented APIs. -- **Prefer:** Keep stories aligned with component docs metadata and real consumer-facing behavior. -- **Authority:** `.github/skills/storybook/SKILL.md` - -### `stories.keep-interactions-in-tests` - -- **Check:** Stories do not add play functions as a substitute for interaction coverage. -- **Prefer:** Exercise browser interactions through Playwright against the story and keep unit-level interactions in focused tests. -- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` - -## Component testing - -Applies to: `packages/react/src/**/*.test.ts,packages/react/src/**/*.test.tsx,e2e/**/*.test.ts` - -### `testing.observable-behavior` - -- **Check:** Unit tests assert observable semantics and behavior from the user perspective rather than implementation details. -- **Prefer:** Use Testing Library roles, accessible names, userEvent, and explicit contract assertions. -- **Authority:** `contributor-docs/testing.md` - -### `testing.no-catch-all-render-snapshots` - -- **Check:** Tests do not add catch-all snapshots of rendered React trees. -- **Prefer:** Reserve snapshots for static objects with a clear review signal, such as package export lists. -- **Authority:** `contributor-docs/adrs/adr-011-snapshot-tests.md` - -### `testing.playwright-for-browser-behavior` - -- **Check:** Behavior requiring real layout, browser interaction, visual regression, or automated accessibility coverage is tested through the Playwright e2e suite. -- **Prefer:** Use @vrt and @avt coverage where visual appearance or accessibility output changes. -- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` - -### `testing.public-contract-coverage` - -- **Check:** New public props, forwarded DOM props, stable identifiers, keyboard behavior, and accessibility semantics have focused regression coverage. -- **Prefer:** Name tests for the contract they protect. -- **Authority:** `contributor-docs/testing.md` - diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9b732b802a7..47069349b69 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -27,8 +27,6 @@ jobs: cache: 'npm' - name: Install dependencies run: npm ci - - name: Check generated component review guidance - run: npm run test:review-guidance && npm run check:review-guidance - name: Check for unformatted files run: npm run format:diff diff --git a/.github/workflows/lint-autofix.yml b/.github/workflows/lint-autofix.yml index 234d7a972c3..63f876d067d 100644 --- a/.github/workflows/lint-autofix.yml +++ b/.github/workflows/lint-autofix.yml @@ -101,10 +101,6 @@ jobs: if: steps.check_jobs.outputs.should_fix == 'true' && steps.pr.outputs.head_ref != '' run: npm ci - - name: Regenerate component review guidance - if: steps.check_jobs.outputs.format_failed == 'true' && steps.pr.outputs.head_ref != '' - run: npm run build:review-guidance - - name: Run format auto-fix if: steps.check_jobs.outputs.format_failed == 'true' && steps.pr.outputs.head_ref != '' run: npm run format diff --git a/.prettierignore b/.prettierignore index 2db784530e3..a7d223c884b 100644 --- a/.prettierignore +++ b/.prettierignore @@ -1,7 +1,5 @@ .changeset .agents -.github/instructions/generated -.github/skills/primer-component-review/references/generated-policy.md packages/react/CHANGELOG.md dist storybook-static diff --git a/AGENTS.md b/AGENTS.md deleted file mode 100644 index 71f58e13e9b..00000000000 --- a/AGENTS.md +++ /dev/null @@ -1,22 +0,0 @@ -# Primer React agent guidance - -## Component review - -For changes to Primer React components, hooks, stories, tests, CSS, docs metadata, -exports, or changesets: - -- Apply the path-specific instructions in - `.github/instructions/generated/*.instructions.md`. -- Use the `primer-component-review` skill for the review procedure and complete - generated policy catalog. -- Treat accepted public ADRs and maintained contributor guides as authoritative. - Treat imported Primer architecture policy as outcome-only guidance; never - attempt to discover, infer, or disclose its private rationale or source. -- Report only actionable mismatches introduced or expanded by the change. -- Cite the policy rule ID in each finding. -- Do not report pre-existing migration debt unless the change makes it worse. -- Advisory rules apply only while the affected API or contract is being designed - or expanded. - -When guidance conflicts, stop rather than guessing. Identify the conflicting -public rule IDs or source files so maintainers can resolve the policy. diff --git a/contributor-docs/adrs/adr-000-template.md b/contributor-docs/adrs/adr-000-template.md index cabf4446f02..710316666e9 100644 --- a/contributor-docs/adrs/adr-000-template.md +++ b/contributor-docs/adrs/adr-000-template.md @@ -9,14 +9,6 @@ | Status | Proposed ❓ OR Accepted ✅ OR Superseded by [LINK](https://) ⚠️ OR Deprecated ⛔ | | Implementation | Not planned ⛔ OR Adoption awaiting in [LINK](https://) ⏸️ OR Adopted ✅ | -## Review impact - - - ## Context diff --git a/contributor-docs/review-guidance/README.md b/contributor-docs/review-guidance/README.md deleted file mode 100644 index 5e4f80b26a2..00000000000 --- a/contributor-docs/review-guidance/README.md +++ /dev/null @@ -1,37 +0,0 @@ -# Component review guidance - -The files in this directory are the structured source for generated Copilot code -review instructions. - -- `policy.json` records public rules derived from accepted ADRs and maintained - contributor documentation. -- `internal-policy.json` is the public-safe import boundary for architectural - outcomes produced outside this repository. It must never contain private - rationale, links, identifiers, titles, filenames, or quoted source text. - -Run `npm run build:review-guidance` after changing either policy. Generated -instructions live in `.github/instructions/generated/`, and the complete -reviewer reference lives in -`.github/skills/primer-component-review/references/generated-policy.md`. - -## Updating an ADR or contributor guide - -When a source changes an actionable component-development decision: - -1. Update, add, supersede, or remove the corresponding rule in `policy.json`. -2. Run `npm run accept:review-guidance-sources` after reviewing every rule mapped - to the changed source. -3. Run `npm run build:review-guidance`. -4. Run `npm run check:review-guidance`. - -If the source does not affect component review, no policy rule is needed. - -## Public-safe imported rules - -Imported rules use a strict outcome-only schema and omit all source fields. They -must contain only the outcome needed to review public code. The generator rejects -unknown fields, URLs, and known private-source markers in -`internal-policy.json`. - -The sanitizer is defense in depth, not a substitute for producing the import -from explicitly public-safe fields in the private source repository. diff --git a/contributor-docs/review-guidance/internal-policy.json b/contributor-docs/review-guidance/internal-policy.json deleted file mode 100644 index c5bb015132d..00000000000 --- a/contributor-docs/review-guidance/internal-policy.json +++ /dev/null @@ -1,121 +0,0 @@ -{ - "version": 1, - "ruleSets": [ - { - "id": "architecture-api-language", - "title": "Component API language", - "applyTo": "packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json,packages/react/src/**/*.hookDocs.json", - "rules": [ - { - "id": "architecture-api-language.semantic-size-names", - "enforcement": "advisory", - "check": "New public size, density, thickness, and viewport-range APIs use the established semantic vocabulary instead of abbreviations, CSS literals, or default as a size name.", - "prefer": "Use descriptive full names and semantic values that remain meaningful across platforms." - }, - { - "id": "architecture-api-language.component-references", - "enforcement": "advisory", - "check": "Documentation and stories use the exported code name when referring to a specific React component and sentence case when discussing a generic interface pattern.", - "prefer": "Keep API references unambiguous without presenting every generic pattern name as a code identifier." - } - ] - }, - { - "id": "architecture-button-layout", - "title": "Button layout semantics", - "applyTo": "packages/react/src/**/*Button*.tsx,packages/react/src/**/*Button*.css,packages/react/src/SelectPanel/**/*.tsx,packages/react/src/experimental/SelectPanel2/**/*.tsx", - "rules": [ - { - "id": "architecture-button-layout.full-width-alignment", - "enforcement": "advisory", - "check": "A full-width call-to-action centers its content, while a full-width selection trigger aligns its primary content to the start and keeps any trailing affordance at the end.", - "prefer": "Use alignment to communicate whether the control performs an action or exposes a selection." - } - ] - }, - { - "id": "architecture-notifications", - "title": "Notification patterns", - "applyTo": "packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json", - "rules": [ - { - "id": "architecture-notifications.no-toast-pattern", - "enforcement": "enforce", - "check": "The change does not add, restore, or recommend a toast component or toast recipe.", - "prefer": "Use a supported feedback pattern appropriate to the interaction, such as a banner, inline validation, inline message, or dialog." - } - ] - }, - { - "id": "architecture-color", - "title": "Semantic color and contrast", - "applyTo": "packages/react/src/**/*.module.css", - "rules": [ - { - "id": "architecture-color.state-roles", - "enforcement": "advisory", - "check": "Open and closed states use their corresponding semantic functional color roles instead of success and danger roles.", - "prefer": "Preserve the distinction between lifecycle state and validation or outcome semantics." - }, - { - "id": "architecture-color.canonical-variants", - "enforcement": "enforce", - "check": "The change does not introduce deprecated subtle functional color variants.", - "prefer": "Use muted for secondary or less-prominent treatment and emphasis for stronger treatment." - }, - { - "id": "architecture-color.text-and-links", - "enforcement": "advisory", - "check": "Non-decorative text and placeholders maintain at least 4.5:1 contrast against every supported background, and links remain distinguishable from surrounding text without relying only on color.", - "prefer": "Use supported token pairs and retain a visible non-color link affordance." - }, - { - "id": "architecture-color.focus-and-state", - "enforcement": "advisory", - "check": "Focus indicators and simultaneously visible interactive states maintain at least 3:1 contrast against adjacent colors.", - "prefer": "Verify contrast across supported themes without imposing a contrast requirement on purely decorative or inactive control borders." - } - ] - }, - { - "id": "architecture-select-panel", - "title": "SelectPanel interaction contract", - "applyTo": "packages/react/src/SelectPanel/**/*", - "rules": [ - { - "id": "architecture-select-panel.focus-restoration", - "enforcement": "enforce", - "check": "Opening moves focus to the configured initial target and closing restores focus to the trigger.", - "prefer": "Focus the filter input by default while preserving explicit deferred-focus behavior and the component's established secondary-focus semantics." - }, - { - "id": "architecture-select-panel.commit-behavior", - "enforcement": "enforce", - "check": "Anchored dismissal preserves the latest selection, while modal Save finalizes changes and modal cancellation or dismissal invokes the cancellation contract.", - "prefer": "Preserve distinct anchored and modal commit semantics across every close path so consumers can restore the prior committed modal selection." - }, - { - "id": "architecture-select-panel.filter-focus", - "enforcement": "enforce", - "check": "Input focus and filtering preserve a valid active option whenever option activation is enabled.", - "prefer": "Keep the input and list behavior connected and normally activate the first available option after filtering.", - "exceptions": [ - "An explicit deferred-initial-focus configuration may wait for user interaction before assigning the active option." - ] - }, - { - "id": "architecture-select-panel.keyboard-navigation", - "enforcement": "enforce", - "check": "Keyboard behavior supports toggling the active option, sequential tab navigation, directional option navigation, first and last option shortcuts, and Escape dismissal.", - "prefer": "Maintain the complete interaction contract instead of adding isolated key handlers." - }, - { - "id": "architecture-select-panel.announcements", - "enforcement": "enforce", - "check": "Screen-reader announcements communicate the active option, its position, and selection state without announcing on every filtering keystroke.", - "prefer": "Use polite, delayed announcements and debounce filtering updates." - } - ] - } - ] -} diff --git a/contributor-docs/review-guidance/policy.json b/contributor-docs/review-guidance/policy.json deleted file mode 100644 index 8538daf98f7..00000000000 --- a/contributor-docs/review-guidance/policy.json +++ /dev/null @@ -1,325 +0,0 @@ -{ - "version": 1, - "ruleSets": [ - { - "id": "component-api", - "title": "Component API and implementation", - "applyTo": "packages/react/src/**/*.ts,packages/react/src/**/*.tsx", - "sources": [ - "contributor-docs/style.md", - "contributor-docs/component-contents-api-patterns.md", - "contributor-docs/adrs/adr-004-children-as-api.md" - ], - "rules": [ - { - "id": "component-api.spectrum-of-abstraction", - "enforcement": "enforce", - "check": "New component APIs begin with flexible presentational components and behavior hooks unless the product pattern and its defaults are already stable.", - "prefer": "Build config components by composing established presentational parts and hooks instead of duplicating their behavior or markup.", - "source": "contributor-docs/style.md" - }, - { - "id": "component-api.children-for-content", - "enforcement": "enforce", - "check": "Content uses React children when consumers own the rendered elements or need free-form composition.", - "prefer": "Use data props when the component must own, transform, order, or constrain the rendered elements. Do not support equivalent children and data APIs simultaneously without a concrete need.", - "source": "contributor-docs/component-contents-api-patterns.md" - }, - { - "id": "component-api.no-speculative-parts", - "enforcement": "enforce", - "check": "Compound parts, render props, child-prop pass-through APIs, and escape hatches address demonstrated use cases rather than speculative flexibility.", - "prefer": "Start with the smallest API that supports known scenarios and open it up when a concrete need appears.", - "source": "contributor-docs/adrs/adr-004-children-as-api.md" - }, - { - "id": "component-api.avoid-slots-by-default", - "enforcement": "enforce", - "check": "Slots, useSlots, and __SLOT__ markers are only introduced when a parent must identify a specific child or the requested API requires child extraction.", - "prefer": "Use normal React composition and preserve consumer-authored child order.", - "source": "contributor-docs/style.md" - }, - { - "id": "component-api.reuse-base-primitives", - "enforcement": "enforce", - "check": "Components reuse existing base components, accessibility primitives, hooks, and utilities before introducing parallel implementations.", - "prefer": "Use primitives such as ButtonBase for Primer-owned native semantics, interactions, and reset styling.", - "source": "contributor-docs/style.md" - }, - { - "id": "component-api.controlled-state", - "enforcement": "enforce", - "check": "Components supporting controlled and uncontrolled state use the shared useControllableState pattern.", - "prefer": "Keep controlled value, default value, and change callback behavior aligned with the repository helper.", - "source": "contributor-docs/style.md" - }, - { - "id": "component-api.root-rest-props", - "enforcement": "enforce", - "check": "Forwarded rest props and className are applied to the component root unless the public contract explicitly identifies another element.", - "prefer": "Test that forwarded props reach the documented element.", - "source": "contributor-docs/style.md" - }, - { - "id": "component-api.extensible-callbacks", - "enforcement": "enforce", - "check": "New public callback props use a single object argument when the callback payload may grow.", - "prefer": "Use a named object payload instead of positional arguments.", - "source": "contributor-docs/style.md" - }, - { - "id": "component-api.ssr", - "enforcement": "enforce", - "check": "Render paths avoid browser-only APIs and hydration-dependent layout changes.", - "prefer": "Progressively enhance after hydration and move browser access into appropriate effects or event handlers.", - "source": "contributor-docs/style.md" - }, - { - "id": "component-api.focus-from-events", - "enforcement": "enforce", - "check": "Focus moves as a direct result of user events rather than unrelated effect dependency changes.", - "prefer": "Manage focus in the event handler that caused the transition.", - "source": "contributor-docs/style.md" - }, - { - "id": "component-api.hook-ref-input", - "enforcement": "enforce", - "check": "Hooks that operate on a caller-owned DOM node accept a ref instead of creating and returning an incompatible ref.", - "prefer": "Let callers compose multiple behaviors around the same ref.", - "source": "contributor-docs/style.md" - }, - { - "id": "component-api.stable-hook-callbacks", - "enforcement": "enforce", - "check": "Hook effects do not repeatedly resubscribe solely because a callback argument changed identity.", - "prefer": "Use the repository stable-callback pattern so subscriptions always call the latest callback.", - "source": "contributor-docs/style.md" - }, - { - "id": "component-api.public-hooks-need-consumers", - "enforcement": "enforce", - "check": "New public hooks represent a clear consumer capability rather than exposing subcomponent implementation details.", - "prefer": "Keep behavior hooks internal until a supported external use case and complete public contract exist.", - "source": "contributor-docs/style.md" - } - ], - "sourceDigest": "e2ac6c7a9525" - }, - { - "id": "component-structure", - "title": "Component files and public surface", - "applyTo": "packages/react/src/**/*", - "sources": [ - "contributor-docs/CONTRIBUTING.md", - "contributor-docs/adrs/adr-013-file-structure.md", - "contributor-docs/adrs/adr-015-internal-modules.md", - "contributor-docs/adrs/adr-023-stable-selectors-api.md" - ], - "rules": [ - { - "id": "component-structure.colocate-files", - "enforcement": "enforce", - "check": "New components use a PascalCase directory and colocate implementation, CSS, tests, stories, docs metadata, and exports according to repository conventions.", - "prefer": "Name subcomponent files with the parent component prefix for discoverability.", - "source": "contributor-docs/adrs/adr-013-file-structure.md" - }, - { - "id": "component-structure.internal-modules", - "enforcement": "enforce", - "check": "New shared modules that are not public API live under packages/react/src/internal and are not exported from public entrypoints.", - "prefer": "Keep implementation details internal until a supported public API is intentionally designed.", - "source": "contributor-docs/adrs/adr-015-internal-modules.md" - }, - { - "id": "component-structure.stable-identifiers", - "enforcement": "advisory", - "check": "New or changed component roots, public subcomponents, and meaningful structural parts follow the established data-component naming contract.", - "prefer": "Use PascalCase component API names, keep state in separate data attributes, and test stable identifier values.", - "source": "contributor-docs/adrs/adr-023-stable-selectors-api.md" - }, - { - "id": "component-structure.complete-public-surface", - "enforcement": "enforce", - "check": "Public API changes update exports, export snapshots, docs metadata, tests, stories, and changesets as applicable.", - "prefer": "Treat all public surfaces as one component contract.", - "source": "contributor-docs/CONTRIBUTING.md" - } - ], - "sourceDigest": "21badb7273eb" - }, - { - "id": "stories", - "title": "Storybook stories", - "applyTo": "packages/react/src/**/*.stories.tsx", - "sources": [ - "contributor-docs/CONTRIBUTING.md", - "contributor-docs/adrs/adr-017-interaction-tests-revisited.md", - ".github/skills/storybook/SKILL.md" - ], - "rules": [ - { - "id": "stories.default-and-playground", - "enforcement": "enforce", - "check": "The default story file uses the Components/ComponentName title and includes Default and Playground when configurable controls are supported.", - "prefer": "Keep the primary documentation stories in ComponentName.stories.tsx.", - "source": "contributor-docs/CONTRIBUTING.md" - }, - { - "id": "stories.separate-features-and-examples", - "enforcement": "enforce", - "check": "Feature coverage and scenario examples are separated from the default story file.", - "prefer": "Use ComponentName.features.stories.tsx for features and ComponentName.examples.stories.tsx for distinct scenarios.", - "source": "contributor-docs/CONTRIBUTING.md" - }, - { - "id": "stories.document-supported-api", - "enforcement": "enforce", - "check": "Stories demonstrate supported props, variants, and composition rather than inventing undocumented APIs.", - "prefer": "Keep stories aligned with component docs metadata and real consumer-facing behavior.", - "source": ".github/skills/storybook/SKILL.md" - }, - { - "id": "stories.keep-interactions-in-tests", - "enforcement": "enforce", - "check": "Stories do not add play functions as a substitute for interaction coverage.", - "prefer": "Exercise browser interactions through Playwright against the story and keep unit-level interactions in focused tests.", - "source": "contributor-docs/adrs/adr-017-interaction-tests-revisited.md" - } - ], - "sourceDigest": "ce5b186845a5" - }, - { - "id": "testing", - "title": "Component testing", - "applyTo": "packages/react/src/**/*.test.ts,packages/react/src/**/*.test.tsx,e2e/**/*.test.ts", - "sources": [ - "contributor-docs/testing.md", - "contributor-docs/adrs/adr-011-snapshot-tests.md", - "contributor-docs/adrs/adr-017-interaction-tests-revisited.md" - ], - "rules": [ - { - "id": "testing.observable-behavior", - "enforcement": "enforce", - "check": "Unit tests assert observable semantics and behavior from the user perspective rather than implementation details.", - "prefer": "Use Testing Library roles, accessible names, userEvent, and explicit contract assertions.", - "source": "contributor-docs/testing.md" - }, - { - "id": "testing.no-catch-all-render-snapshots", - "enforcement": "enforce", - "check": "Tests do not add catch-all snapshots of rendered React trees.", - "prefer": "Reserve snapshots for static objects with a clear review signal, such as package export lists.", - "source": "contributor-docs/adrs/adr-011-snapshot-tests.md" - }, - { - "id": "testing.playwright-for-browser-behavior", - "enforcement": "enforce", - "check": "Behavior requiring real layout, browser interaction, visual regression, or automated accessibility coverage is tested through the Playwright e2e suite.", - "prefer": "Use @vrt and @avt coverage where visual appearance or accessibility output changes.", - "source": "contributor-docs/adrs/adr-017-interaction-tests-revisited.md" - }, - { - "id": "testing.public-contract-coverage", - "enforcement": "enforce", - "check": "New public props, forwarded DOM props, stable identifiers, keyboard behavior, and accessibility semantics have focused regression coverage.", - "prefer": "Name tests for the contract they protect.", - "source": "contributor-docs/testing.md" - } - ], - "sourceDigest": "c5645620dafd" - }, - { - "id": "css", - "title": "Component CSS", - "applyTo": "packages/react/src/**/*.css,packages/react/src/**/*.tsx", - "sources": [ - "contributor-docs/style.md", - "contributor-docs/authoring-css.md", - "contributor-docs/adrs/adr-016-css.md", - ".github/instructions/css.instructions.md" - ], - "rules": [ - { - "id": "css.modules", - "enforcement": "enforce", - "check": "Component styles use colocated CSS Modules and Primer primitive custom properties.", - "prefer": "Avoid new runtime-injected component styling and hardcoded design values.", - "source": "contributor-docs/adrs/adr-016-css.md" - }, - { - "id": "css.root-classname", - "enforcement": "enforce", - "check": "A consumer className is merged with clsx on the top-level element and cannot be overwritten by prop spread order.", - "prefer": "Use CSS custom properties instead of multiple child className escape hatches.", - "source": "contributor-docs/authoring-css.md" - }, - { - "id": "css.data-attributes-for-variants", - "enforcement": "enforce", - "check": "Component variants and state use data attributes or native pseudo-classes rather than modifier class names.", - "prefer": "Use :where() around data-attribute selectors to keep specificity low.", - "source": "contributor-docs/authoring-css.md" - }, - { - "id": "css.javascript-bridge", - "enforcement": "enforce", - "check": "Dynamic values crossing from JavaScript into CSS use custom properties instead of authored visual styles inline.", - "prefer": "Keep layout and visual rules in the CSS Module.", - "source": "contributor-docs/style.md" - }, - { - "id": "css.no-primitive-fallbacks", - "enforcement": "enforce", - "check": "Primer primitive custom properties are used without manually authored fallback values.", - "prefer": "Rely on the primitive build output to provide supported fallbacks.", - "source": "contributor-docs/authoring-css.md" - }, - { - "id": "css.browser-support", - "enforcement": "enforce", - "check": "CSS newer than Baseline 2022 includes an appropriate fallback and broadly matching selectors avoid expensive :has() invalidation.", - "prefer": "Scope necessary :has() usage to a CSS Module class with a justified local stylelint disable.", - "source": ".github/instructions/css.instructions.md" - } - ], - "sourceDigest": "043e538ad1a2" - }, - { - "id": "deprecations-and-versioning", - "title": "Deprecations, versioning, and changesets", - "applyTo": "packages/react/src/**/*,.changeset/*.md", - "sources": [ - "contributor-docs/deprecating-components.md", - "contributor-docs/versioning.md", - "contributor-docs/adrs/adr-019-deprecating-props.md", - ".github/skills/deprecations/SKILL.md", - ".github/skills/changesets/SKILL.md" - ], - "rules": [ - { - "id": "deprecations-and-versioning.complete-deprecation-signal", - "enforcement": "enforce", - "check": "Deprecations update JSDoc, docs metadata, migration guidance, and the recommended replacement where applicable.", - "prefer": "Keep runtime behavior unchanged unless the change explicitly includes migration behavior.", - "source": ".github/skills/deprecations/SKILL.md" - }, - { - "id": "deprecations-and-versioning.consumer-impact", - "enforcement": "enforce", - "check": "The changeset bump matches the largest consumer-facing API, behavior, CSS, accessibility, or stable-identifier impact.", - "prefer": "Use the repository versioning table and describe the outcome from the consumer perspective.", - "source": "contributor-docs/versioning.md" - }, - { - "id": "deprecations-and-versioning.changeset-required", - "enforcement": "enforce", - "check": "Published runtime or public API changes include an accurate changeset; docs-only, test-only, story-only, and infrastructure-only changes do not.", - "prefer": "Keep each changeset terse and scoped to one consumer-facing change.", - "source": ".github/skills/changesets/SKILL.md" - } - ], - "sourceDigest": "9d4c59fd70d0" - } - ] -} diff --git a/package.json b/package.json index 393f3997814..fcaaf5ac39b 100644 --- a/package.json +++ b/package.json @@ -22,16 +22,12 @@ "build:docs:preview": "NODE_OPTIONS=--openssl-legacy-provider script/build-docs preview", "build:components.json": "npm run build:components.json -w @primer/react", "build:hooks.json": "npm run build:hooks.json -w @primer/react", - "build:review-guidance": "node script/review-guidance.mjs", - "accept:review-guidance-sources": "node script/review-guidance.mjs --accept-sources", - "check:review-guidance": "node script/review-guidance.mjs --check", "lint": "eslint '**/*.{js,ts,tsx,md,mdx}' --max-warnings=0 --cache", "lint:css": "stylelint --rd -q --cache '**/*.css'", "lint:css:fix": "npm run lint:css -- --fix", "lint:fix": "npm run lint -- --fix", "lint:md": "markdownlint-cli2 \"**/*.{md,mdx}\" \"!.github\" \"!.changeset\" \"!.agents\" \"!**/node_modules/**\" \"!**/CHANGELOG.md\"", "test": "vitest", - "test:review-guidance": "node --test script/review-guidance.test.mjs", "test:type-check": "tsc --noEmit", "test:update": "npm run test -- -u", "test:classname-coverage": "node script/check-classname-tests.mjs", diff --git a/script/review-guidance.mjs b/script/review-guidance.mjs deleted file mode 100644 index a73c23f79aa..00000000000 --- a/script/review-guidance.mjs +++ /dev/null @@ -1,276 +0,0 @@ -import {createHash} from 'node:crypto' -import {existsSync, mkdirSync, readFileSync, readdirSync, rmSync, writeFileSync} from 'node:fs' -import {dirname, join, relative, resolve} from 'node:path' -import {fileURLToPath, pathToFileURL} from 'node:url' - -const scriptDirectory = dirname(fileURLToPath(import.meta.url)) -const repositoryRoot = resolve(scriptDirectory, '..') -const publicPolicyPath = join(repositoryRoot, 'contributor-docs/review-guidance/policy.json') -const internalPolicyPath = join(repositoryRoot, 'contributor-docs/review-guidance/internal-policy.json') -const instructionsDirectory = join(repositoryRoot, '.github/instructions/generated') -const skillReferencePath = join(repositoryRoot, '.github/skills/primer-component-review/references/generated-policy.md') -const generatedMarker = '' - -function readJson(path) { - return JSON.parse(readFileSync(path, 'utf8')) -} - -export function sourceDigest(sourcePaths) { - const hash = createHash('sha256') - for (const sourcePath of sourcePaths) { - hash.update(sourcePath) - hash.update(readFileSync(join(repositoryRoot, sourcePath))) - } - return hash.digest('hex').slice(0, 12) -} - -function assertAllowedKeys(value, allowedKeys, label) { - for (const key of Object.keys(value)) { - assert(allowedKeys.includes(key), `${label} contains unsupported field: ${key}`) - } -} - -function assert(condition, message) { - if (!condition) { - throw new Error(message) - } -} - -export function validatePolicies(publicPolicy, internalPolicy) { - assertAllowedKeys(publicPolicy, ['version', 'ruleSets'], 'Public policy') - assertAllowedKeys(internalPolicy, ['version', 'ruleSets'], 'Internal policy') - assert(publicPolicy.version === 1, 'Public policy must use version 1') - assert(internalPolicy.version === 1, 'Internal policy must use version 1') - assert(Array.isArray(publicPolicy.ruleSets), 'Public policy must include ruleSets') - assert(Array.isArray(internalPolicy.ruleSets), 'Internal policy must include ruleSets') - - const ruleSetIds = new Set() - const ruleIds = new Set() - const supersededRuleIds = new Set() - const privateText = JSON.stringify(internalPolicy) - const forbiddenPrivatePatterns = [ - /https?:\/\//i, - /github\/primer/i, - /slack/i, - /figma/i, - /discussion/i, - /issue[#\s-]*\d+/i, - /rationale/i, - /alternative/i, - /consequence/i, - ] - - for (const pattern of forbiddenPrivatePatterns) { - assert(!pattern.test(privateText), `Internal policy contains forbidden private-source text: ${pattern}`) - } - - for (const [policyName, policy] of [ - ['public', publicPolicy], - ['internal', internalPolicy], - ]) { - for (const ruleSet of policy.ruleSets) { - assertAllowedKeys( - ruleSet, - policyName === 'public' - ? ['id', 'title', 'applyTo', 'sources', 'sourceDigest', 'rules'] - : ['id', 'title', 'applyTo', 'rules'], - `${policyName} rule set`, - ) - assert(typeof ruleSet.id === 'string' && ruleSet.id.length > 0, `${policyName} rule set needs an id`) - assert(!ruleSetIds.has(ruleSet.id), `Duplicate rule set id: ${ruleSet.id}`) - ruleSetIds.add(ruleSet.id) - assert(typeof ruleSet.title === 'string' && ruleSet.title.length > 0, `${ruleSet.id} needs a title`) - assert(typeof ruleSet.applyTo === 'string' && ruleSet.applyTo.length > 0, `${ruleSet.id} needs applyTo`) - assert(Array.isArray(ruleSet.rules) && ruleSet.rules.length > 0, `${ruleSet.id} needs rules`) - - if (policyName === 'public') { - assert(Array.isArray(ruleSet.sources) && ruleSet.sources.length > 0, `${ruleSet.id} needs sources`) - for (const source of ruleSet.sources) { - assert(existsSync(join(repositoryRoot, source)), `Missing source for ${ruleSet.id}: ${source}`) - } - assert( - ruleSet.sourceDigest === sourceDigest(ruleSet.sources), - `Sources changed for ${ruleSet.id}. Review its rules, then run npm run accept:review-guidance-sources.`, - ) - } else { - assert(ruleSet.sources === undefined, `Internal rule set ${ruleSet.id} must not expose sources`) - } - - for (const rule of ruleSet.rules) { - assertAllowedKeys( - rule, - policyName === 'public' - ? ['id', 'enforcement', 'check', 'prefer', 'exceptions', 'source', 'supersedes'] - : ['id', 'enforcement', 'check', 'prefer', 'exceptions', 'supersedes'], - `${ruleSet.id} rule`, - ) - assert(typeof rule.id === 'string' && rule.id.startsWith(`${ruleSet.id}.`), `Invalid rule id: ${rule.id}`) - assert(!ruleIds.has(rule.id), `Duplicate rule id: ${rule.id}`) - ruleIds.add(rule.id) - assert(['enforce', 'advisory'].includes(rule.enforcement), `Invalid enforcement for ${rule.id}`) - assert(typeof rule.check === 'string' && rule.check.length > 0, `${rule.id} needs a check`) - assert(typeof rule.prefer === 'string' && rule.prefer.length > 0, `${rule.id} needs a preference`) - if (rule.exceptions !== undefined) { - assert(Array.isArray(rule.exceptions), `${rule.id} exceptions must be an array`) - } - if (policyName === 'public') { - assert(ruleSet.sources.includes(rule.source), `${rule.id} needs an exact source from its rule set`) - } - if (rule.supersedes !== undefined) { - assert(Array.isArray(rule.supersedes), `${rule.id} supersedes must be an array`) - for (const supersededRuleId of rule.supersedes) { - assert(!supersededRuleIds.has(supersededRuleId), `${supersededRuleId} is superseded more than once`) - supersededRuleIds.add(supersededRuleId) - } - } - } - } - } - - for (const supersededRuleId of supersededRuleIds) { - assert(ruleIds.has(supersededRuleId), `Unknown superseded rule id: ${supersededRuleId}`) - } -} - -function mergeRuleSets(publicPolicy, internalPolicy) { - return [...publicPolicy.ruleSets, ...internalPolicy.ruleSets].sort((a, b) => a.id.localeCompare(b.id)) -} - -function renderRule(rule, sourceLabel) { - const lines = [ - `### \`${rule.id}\`${rule.enforcement === 'advisory' ? ' (advisory)' : ''}`, - '', - `- **Check:** ${rule.check}`, - `- **Prefer:** ${rule.prefer}`, - ] - - if (rule.exceptions?.length) { - lines.push(`- **Exceptions:** ${rule.exceptions.join(' ')}`) - } - - lines.push(`- **Authority:** ${sourceLabel}`, '') - return lines.join('\n') -} - -export function renderInstruction(ruleSet) { - const digest = ruleSet.sourceDigest ?? 'sanitized-import' - return [ - '---', - `applyTo: '${ruleSet.applyTo}'`, - '---', - '', - generatedMarker, - '', - `# ${ruleSet.title}`, - '', - `Policy set: \`${ruleSet.id}\`. Source digest: \`${digest}\`.`, - '', - 'Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse.', - '', - ...ruleSet.rules.map(rule => - renderRule(rule, rule.source ? `\`${rule.source}\`` : `Primer architecture policy: \`${rule.id}\``), - ), - ].join('\n') -} - -function renderSkillReference(ruleSets) { - return [ - generatedMarker, - '', - '# Primer component review policy', - '', - 'Apply only the policy sets relevant to the changed files. Enforced rules may produce findings. Advisory rules should only produce a finding when the change is designing or expanding the affected contract and the mismatch has a concrete consumer impact.', - '', - ...ruleSets.flatMap(ruleSet => { - return [ - `## ${ruleSet.title}`, - '', - `Applies to: \`${ruleSet.applyTo}\``, - '', - ...ruleSet.rules.map(rule => - renderRule(rule, rule.source ? `\`${rule.source}\`` : `Primer architecture policy: \`${rule.id}\``), - ), - ] - }), - ].join('\n') -} - -function expectedFiles(ruleSets) { - const files = new Map() - for (const ruleSet of ruleSets) { - files.set(join(instructionsDirectory, `${ruleSet.id}.instructions.md`), renderInstruction(ruleSet)) - } - files.set(skillReferencePath, renderSkillReference(ruleSets)) - return files -} - -function checkFiles(files) { - const staleFiles = [] - for (const [path, contents] of files) { - if (!existsSync(path) || readFileSync(path, 'utf8') !== `${contents}\n`) { - staleFiles.push(relative(repositoryRoot, path)) - } - } - - if (existsSync(instructionsDirectory)) { - for (const filename of readdirSync(instructionsDirectory)) { - const path = join(instructionsDirectory, filename) - if (filename.endsWith('.instructions.md') && !files.has(path)) { - staleFiles.push(relative(repositoryRoot, path)) - } - } - } - - if (staleFiles.length > 0) { - throw new Error( - `Generated review guidance is out of date:\n${staleFiles.map(path => `- ${path}`).join('\n')}\nRun npm run build:review-guidance.`, - ) - } -} - -function writeFiles(files) { - mkdirSync(instructionsDirectory, {recursive: true}) - mkdirSync(dirname(skillReferencePath), {recursive: true}) - - for (const filename of readdirSync(instructionsDirectory)) { - const path = join(instructionsDirectory, filename) - if (filename.endsWith('.instructions.md') && !files.has(path)) { - rmSync(path) - } - } - - for (const [path, contents] of files) { - writeFileSync(path, `${contents}\n`) - } -} - -function acceptSourceChanges(publicPolicy) { - for (const ruleSet of publicPolicy.ruleSets) { - ruleSet.sourceDigest = sourceDigest(ruleSet.sources) - } - writeFileSync(publicPolicyPath, `${JSON.stringify(publicPolicy, null, 2)}\n`) -} - -export function build({acceptSources = false, check = false} = {}) { - const publicPolicy = readJson(publicPolicyPath) - const internalPolicy = readJson(internalPolicyPath) - if (acceptSources) { - acceptSourceChanges(publicPolicy) - } - validatePolicies(publicPolicy, internalPolicy) - const files = expectedFiles(mergeRuleSets(publicPolicy, internalPolicy)) - - if (check) { - checkFiles(files) - } else { - writeFiles(files) - } -} - -const isMain = process.argv[1] && import.meta.url === pathToFileURL(resolve(process.argv[1])).href -if (isMain) { - build({ - acceptSources: process.argv.includes('--accept-sources'), - check: process.argv.includes('--check'), - }) -} diff --git a/script/review-guidance.test.mjs b/script/review-guidance.test.mjs deleted file mode 100644 index 7c47d2c5f22..00000000000 --- a/script/review-guidance.test.mjs +++ /dev/null @@ -1,74 +0,0 @@ -import assert from 'node:assert/strict' -import {test} from 'node:test' -import {renderInstruction, sourceDigest, validatePolicies} from './review-guidance.mjs' - -const validPublicPolicy = { - version: 1, - ruleSets: [ - { - id: 'example', - title: 'Example', - applyTo: 'packages/react/src/**/*.tsx', - sources: ['contributor-docs/style.md'], - sourceDigest: sourceDigest(['contributor-docs/style.md']), - rules: [ - { - id: 'example.rule', - enforcement: 'enforce', - check: 'Check the example.', - prefer: 'Prefer the supported example.', - source: 'contributor-docs/style.md', - }, - ], - }, - ], -} - -test('validates a public policy and sanitized internal import', () => { - assert.doesNotThrow(() => validatePolicies(validPublicPolicy, {version: 1, ruleSets: []})) -}) - -test('rejects private source details from imported policy', () => { - assert.throws( - () => - validatePolicies(validPublicPolicy, { - version: 1, - ruleSets: [ - { - id: 'private', - title: 'Private', - applyTo: '**/*.tsx', - rules: [ - { - id: 'private.rule', - enforcement: 'enforce', - check: 'See https://example.com for the decision.', - prefer: 'Use the decision.', - }, - ], - }, - ], - }), - /forbidden private-source text/, - ) -}) - -test('renders a path-scoped instruction with a rule citation', () => { - const output = renderInstruction(validPublicPolicy.ruleSets[0]) - assert.match(output, /applyTo: 'packages\/react\/src\/\*\*\/\*\.tsx'/) - assert.match(output, /`example\.rule`/) - assert.match(output, /contributor-docs\/style\.md/) -}) - -test('rejects undeclared fields from imported policy', () => { - assert.throws( - () => validatePolicies(validPublicPolicy, {version: 1, ruleSets: [], privateSource: 'hidden'}), - /unsupported field/, - ) -}) - -test('rejects public policy when a mapped source changed', () => { - const stalePolicy = structuredClone(validPublicPolicy) - stalePolicy.ruleSets[0].sourceDigest = 'stale' - assert.throws(() => validatePolicies(stalePolicy, {version: 1, ruleSets: []}), /Sources changed for example/) -}) From 3828db8e0addc865d91ae35e028b10080fba3e11 Mon Sep 17 00:00:00 2001 From: Adam Dierkens Date: Thu, 16 Jul 2026 15:53:25 -0400 Subject: [PATCH 3/6] Add trial ADR-backed review rules Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68a3cde5-7a50-4d4e-97d9-194c8b112e88 --- .../component-review.instructions.md | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 .github/instructions/component-review.instructions.md diff --git a/.github/instructions/component-review.instructions.md b/.github/instructions/component-review.instructions.md new file mode 100644 index 00000000000..0e7a55c23d1 --- /dev/null +++ b/.github/instructions/component-review.instructions.md @@ -0,0 +1,44 @@ +--- +applyTo: 'packages/react/src/**/*' +--- + +# ADR-backed component review trial + +Apply these rules only to behavior or contracts introduced or expanded by the +change. Report concrete impact, cite the rule ID, and do not report pre-existing +migration debt. + +## `component-review.children-as-api` + +- **Check:** Content uses React children when consumers own the rendered + elements or need free-form composition. +- **Prefer:** Use data props when the component must own, transform, order, or + constrain the rendered elements. Do not support equivalent children and data + APIs without a concrete need. +- **Authority:** `contributor-docs/adrs/adr-004-children-as-api.md` + +## `component-review.file-structure` + +- **Check:** New components use a PascalCase directory and colocate + implementation, CSS, tests, stories, docs metadata, and exports according to + repository conventions. +- **Prefer:** Name subcomponent files with the parent component prefix for + discoverability. +- **Authority:** `contributor-docs/adrs/adr-013-file-structure.md` + +## `component-review.internal-modules` + +- **Check:** New shared modules that are not public API live under + `packages/react/src/internal` and are not exported from public entrypoints. +- **Prefer:** Keep implementation details internal until a supported public API + is intentionally designed. +- **Authority:** `contributor-docs/adrs/adr-015-internal-modules.md` + +## `component-review.stable-identifiers` (advisory) + +- **Check:** New or changed component roots, public subcomponents, and + meaningful structural parts follow the established `data-component` naming + contract. +- **Prefer:** Use PascalCase component API names, keep state in separate data + attributes, and test stable identifier values. +- **Authority:** `contributor-docs/adrs/adr-023-stable-selectors-api.md` From 443313ba2f5240d27409bb10189bef0fad591ad8 Mon Sep 17 00:00:00 2001 From: Adam Dierkens Date: Thu, 16 Jul 2026 15:56:50 -0400 Subject: [PATCH 4/6] Narrow trial review rule scope Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68a3cde5-7a50-4d4e-97d9-194c8b112e88 --- .github/instructions/component-review.instructions.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/.github/instructions/component-review.instructions.md b/.github/instructions/component-review.instructions.md index 0e7a55c23d1..288f2c7da6e 100644 --- a/.github/instructions/component-review.instructions.md +++ b/.github/instructions/component-review.instructions.md @@ -20,8 +20,8 @@ migration debt. ## `component-review.file-structure` - **Check:** New components use a PascalCase directory and colocate - implementation, CSS, tests, stories, docs metadata, and exports according to - repository conventions. + applicable implementation, styles, tests, stories, docs metadata, and exports + according to repository conventions. - **Prefer:** Name subcomponent files with the parent component prefix for discoverability. - **Authority:** `contributor-docs/adrs/adr-013-file-structure.md` @@ -36,9 +36,8 @@ migration debt. ## `component-review.stable-identifiers` (advisory) -- **Check:** New or changed component roots, public subcomponents, and - meaningful structural parts follow the established `data-component` naming - contract. +- **Check:** Newly added component roots, public subcomponents, and meaningful + structural parts follow the established `data-component` naming contract. - **Prefer:** Use PascalCase component API names, keep state in separate data attributes, and test stable identifier values. - **Authority:** `contributor-docs/adrs/adr-023-stable-selectors-api.md` From 423d2363c694b0ca2cd8d7672b576f9e9833ea0e Mon Sep 17 00:00:00 2001 From: Adam Dierkens Date: Thu, 16 Jul 2026 15:16:25 -0400 Subject: [PATCH 5/6] Add ADR-backed component review guidance Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68a3cde5-7a50-4d4e-97d9-194c8b112e88 --- .../agents/primer-component-reviewer.agent.md | 27 ++ .../adr-review-guidance.instructions.md | 26 ++ .../architecture-api-language.instructions.md | 24 ++ ...architecture-button-layout.instructions.md | 18 + .../architecture-color.instructions.md | 36 ++ ...architecture-notifications.instructions.md | 18 + .../architecture-select-panel.instructions.md | 43 +++ .../generated/component-api.instructions.md | 90 +++++ .../component-structure.instructions.md | 36 ++ .../generated/css.instructions.md | 48 +++ ...eprecations-and-versioning.instructions.md | 30 ++ .../generated/stories.instructions.md | 36 ++ .../generated/testing.instructions.md | 36 ++ .../skills/primer-component-review/SKILL.md | 44 +++ .../references/generated-policy.md | 333 ++++++++++++++++++ .github/workflows/ci.yml | 2 + .github/workflows/lint-autofix.yml | 4 + .prettierignore | 2 + AGENTS.md | 22 ++ contributor-docs/adrs/adr-000-template.md | 8 + contributor-docs/review-guidance/README.md | 37 ++ .../review-guidance/internal-policy.json | 121 +++++++ contributor-docs/review-guidance/policy.json | 325 +++++++++++++++++ package.json | 4 + script/review-guidance.mjs | 276 +++++++++++++++ script/review-guidance.test.mjs | 74 ++++ 26 files changed, 1720 insertions(+) create mode 100644 .github/agents/primer-component-reviewer.agent.md create mode 100644 .github/instructions/adr-review-guidance.instructions.md create mode 100644 .github/instructions/generated/architecture-api-language.instructions.md create mode 100644 .github/instructions/generated/architecture-button-layout.instructions.md create mode 100644 .github/instructions/generated/architecture-color.instructions.md create mode 100644 .github/instructions/generated/architecture-notifications.instructions.md create mode 100644 .github/instructions/generated/architecture-select-panel.instructions.md create mode 100644 .github/instructions/generated/component-api.instructions.md create mode 100644 .github/instructions/generated/component-structure.instructions.md create mode 100644 .github/instructions/generated/css.instructions.md create mode 100644 .github/instructions/generated/deprecations-and-versioning.instructions.md create mode 100644 .github/instructions/generated/stories.instructions.md create mode 100644 .github/instructions/generated/testing.instructions.md create mode 100644 .github/skills/primer-component-review/SKILL.md create mode 100644 .github/skills/primer-component-review/references/generated-policy.md create mode 100644 AGENTS.md create mode 100644 contributor-docs/review-guidance/README.md create mode 100644 contributor-docs/review-guidance/internal-policy.json create mode 100644 contributor-docs/review-guidance/policy.json create mode 100644 script/review-guidance.mjs create mode 100644 script/review-guidance.test.mjs diff --git a/.github/agents/primer-component-reviewer.agent.md b/.github/agents/primer-component-reviewer.agent.md new file mode 100644 index 00000000000..107844ffdce --- /dev/null +++ b/.github/agents/primer-component-reviewer.agent.md @@ -0,0 +1,27 @@ +--- +name: primer-component-reviewer +description: Reviews Primer React component changes against accepted architectural decisions and maintained repository guidance. +tools: + - read + - search + - execute +skills: + - primer-component-review +--- + +You are a read-only reviewer for Primer React component changes. Never modify +files. + +Use the `primer-component-review` skill and its generated policy reference. +Review component APIs, implementation structure, accessibility, behavior, +stories, tests, CSS, deprecations, documentation metadata, exports, versioning, +and changesets when relevant to the diff. + +Report only concrete, actionable mismatches introduced or expanded by the +change. Cite the changed file and line and the governing policy rule ID. Explain +the impact and recommend the smallest design-level correction. + +Do not expose or speculate about private architectural sources. Rules labeled +`Primer architecture policy` are complete outcome-only guidance. + +If the change follows the applicable policy, say so directly. diff --git a/.github/instructions/adr-review-guidance.instructions.md b/.github/instructions/adr-review-guidance.instructions.md new file mode 100644 index 00000000000..9b99342fd87 --- /dev/null +++ b/.github/instructions/adr-review-guidance.instructions.md @@ -0,0 +1,26 @@ +--- +applyTo: 'contributor-docs/adrs/**/*.md,contributor-docs/CONTRIBUTING.md,contributor-docs/style.md,contributor-docs/testing.md,contributor-docs/authoring-css.md,contributor-docs/behaviors.md,contributor-docs/component-contents-api-patterns.md,contributor-docs/deprecating-components.md,contributor-docs/versioning.md' +--- + +# Maintaining component review guidance + +When an ADR or maintained contributor guide changes an actionable decision about +component APIs, implementation, accessibility, stories, testing, CSS, +deprecations, documentation, or versioning: + +1. Update, add, supersede, or remove the corresponding rule in + `contributor-docs/review-guidance/policy.json`. +2. Keep each rule outcome-focused and suitable for public review comments. Do not + copy private rationale or implementation history into a rule. +3. Use `enforce` only for accepted, current expectations. Use `advisory` for + guidance that should apply only while a related contract is being designed or + expanded. +4. Ensure each affected rule cites the exact changed source in its `source` + field. +5. Run `npm run accept:review-guidance-sources` only after reviewing all rules + mapped to the changed source. +6. Run `npm run build:review-guidance`. +7. Run `npm run check:review-guidance` and + `npm run test:review-guidance`. + +If the source change has no component-review impact, do not add a rule. diff --git a/.github/instructions/generated/architecture-api-language.instructions.md b/.github/instructions/generated/architecture-api-language.instructions.md new file mode 100644 index 00000000000..542508f2689 --- /dev/null +++ b/.github/instructions/generated/architecture-api-language.instructions.md @@ -0,0 +1,24 @@ +--- +applyTo: 'packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json,packages/react/src/**/*.hookDocs.json' +--- + + + +# Component API language + +Policy set: `architecture-api-language`. Source digest: `sanitized-import`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `architecture-api-language.semantic-size-names` (advisory) + +- **Check:** New public size, density, thickness, and viewport-range APIs use the established semantic vocabulary instead of abbreviations, CSS literals, or default as a size name. +- **Prefer:** Use descriptive full names and semantic values that remain meaningful across platforms. +- **Authority:** Primer architecture policy: `architecture-api-language.semantic-size-names` + +### `architecture-api-language.component-references` (advisory) + +- **Check:** Documentation and stories use the exported code name when referring to a specific React component and sentence case when discussing a generic interface pattern. +- **Prefer:** Keep API references unambiguous without presenting every generic pattern name as a code identifier. +- **Authority:** Primer architecture policy: `architecture-api-language.component-references` + diff --git a/.github/instructions/generated/architecture-button-layout.instructions.md b/.github/instructions/generated/architecture-button-layout.instructions.md new file mode 100644 index 00000000000..1f711462fcb --- /dev/null +++ b/.github/instructions/generated/architecture-button-layout.instructions.md @@ -0,0 +1,18 @@ +--- +applyTo: 'packages/react/src/**/*Button*.tsx,packages/react/src/**/*Button*.css,packages/react/src/SelectPanel/**/*.tsx,packages/react/src/experimental/SelectPanel2/**/*.tsx' +--- + + + +# Button layout semantics + +Policy set: `architecture-button-layout`. Source digest: `sanitized-import`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `architecture-button-layout.full-width-alignment` (advisory) + +- **Check:** A full-width call-to-action centers its content, while a full-width selection trigger aligns its primary content to the start and keeps any trailing affordance at the end. +- **Prefer:** Use alignment to communicate whether the control performs an action or exposes a selection. +- **Authority:** Primer architecture policy: `architecture-button-layout.full-width-alignment` + diff --git a/.github/instructions/generated/architecture-color.instructions.md b/.github/instructions/generated/architecture-color.instructions.md new file mode 100644 index 00000000000..4b8ddcdbfd5 --- /dev/null +++ b/.github/instructions/generated/architecture-color.instructions.md @@ -0,0 +1,36 @@ +--- +applyTo: 'packages/react/src/**/*.module.css' +--- + + + +# Semantic color and contrast + +Policy set: `architecture-color`. Source digest: `sanitized-import`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `architecture-color.state-roles` (advisory) + +- **Check:** Open and closed states use their corresponding semantic functional color roles instead of success and danger roles. +- **Prefer:** Preserve the distinction between lifecycle state and validation or outcome semantics. +- **Authority:** Primer architecture policy: `architecture-color.state-roles` + +### `architecture-color.canonical-variants` + +- **Check:** The change does not introduce deprecated subtle functional color variants. +- **Prefer:** Use muted for secondary or less-prominent treatment and emphasis for stronger treatment. +- **Authority:** Primer architecture policy: `architecture-color.canonical-variants` + +### `architecture-color.text-and-links` (advisory) + +- **Check:** Non-decorative text and placeholders maintain at least 4.5:1 contrast against every supported background, and links remain distinguishable from surrounding text without relying only on color. +- **Prefer:** Use supported token pairs and retain a visible non-color link affordance. +- **Authority:** Primer architecture policy: `architecture-color.text-and-links` + +### `architecture-color.focus-and-state` (advisory) + +- **Check:** Focus indicators and simultaneously visible interactive states maintain at least 3:1 contrast against adjacent colors. +- **Prefer:** Verify contrast across supported themes without imposing a contrast requirement on purely decorative or inactive control borders. +- **Authority:** Primer architecture policy: `architecture-color.focus-and-state` + diff --git a/.github/instructions/generated/architecture-notifications.instructions.md b/.github/instructions/generated/architecture-notifications.instructions.md new file mode 100644 index 00000000000..28664c4cf3c --- /dev/null +++ b/.github/instructions/generated/architecture-notifications.instructions.md @@ -0,0 +1,18 @@ +--- +applyTo: 'packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json' +--- + + + +# Notification patterns + +Policy set: `architecture-notifications`. Source digest: `sanitized-import`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `architecture-notifications.no-toast-pattern` + +- **Check:** The change does not add, restore, or recommend a toast component or toast recipe. +- **Prefer:** Use a supported feedback pattern appropriate to the interaction, such as a banner, inline validation, inline message, or dialog. +- **Authority:** Primer architecture policy: `architecture-notifications.no-toast-pattern` + diff --git a/.github/instructions/generated/architecture-select-panel.instructions.md b/.github/instructions/generated/architecture-select-panel.instructions.md new file mode 100644 index 00000000000..4b1e8ae2a62 --- /dev/null +++ b/.github/instructions/generated/architecture-select-panel.instructions.md @@ -0,0 +1,43 @@ +--- +applyTo: 'packages/react/src/SelectPanel/**/*' +--- + + + +# SelectPanel interaction contract + +Policy set: `architecture-select-panel`. Source digest: `sanitized-import`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `architecture-select-panel.focus-restoration` + +- **Check:** Opening moves focus to the configured initial target and closing restores focus to the trigger. +- **Prefer:** Focus the filter input by default while preserving explicit deferred-focus behavior and the component's established secondary-focus semantics. +- **Authority:** Primer architecture policy: `architecture-select-panel.focus-restoration` + +### `architecture-select-panel.commit-behavior` + +- **Check:** Anchored dismissal preserves the latest selection, while modal Save finalizes changes and modal cancellation or dismissal invokes the cancellation contract. +- **Prefer:** Preserve distinct anchored and modal commit semantics across every close path so consumers can restore the prior committed modal selection. +- **Authority:** Primer architecture policy: `architecture-select-panel.commit-behavior` + +### `architecture-select-panel.filter-focus` + +- **Check:** Input focus and filtering preserve a valid active option whenever option activation is enabled. +- **Prefer:** Keep the input and list behavior connected and normally activate the first available option after filtering. +- **Exceptions:** An explicit deferred-initial-focus configuration may wait for user interaction before assigning the active option. +- **Authority:** Primer architecture policy: `architecture-select-panel.filter-focus` + +### `architecture-select-panel.keyboard-navigation` + +- **Check:** Keyboard behavior supports toggling the active option, sequential tab navigation, directional option navigation, first and last option shortcuts, and Escape dismissal. +- **Prefer:** Maintain the complete interaction contract instead of adding isolated key handlers. +- **Authority:** Primer architecture policy: `architecture-select-panel.keyboard-navigation` + +### `architecture-select-panel.announcements` + +- **Check:** Screen-reader announcements communicate the active option, its position, and selection state without announcing on every filtering keystroke. +- **Prefer:** Use polite, delayed announcements and debounce filtering updates. +- **Authority:** Primer architecture policy: `architecture-select-panel.announcements` + diff --git a/.github/instructions/generated/component-api.instructions.md b/.github/instructions/generated/component-api.instructions.md new file mode 100644 index 00000000000..cce228c00c1 --- /dev/null +++ b/.github/instructions/generated/component-api.instructions.md @@ -0,0 +1,90 @@ +--- +applyTo: 'packages/react/src/**/*.ts,packages/react/src/**/*.tsx' +--- + + + +# Component API and implementation + +Policy set: `component-api`. Source digest: `e2ac6c7a9525`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `component-api.spectrum-of-abstraction` + +- **Check:** New component APIs begin with flexible presentational components and behavior hooks unless the product pattern and its defaults are already stable. +- **Prefer:** Build config components by composing established presentational parts and hooks instead of duplicating their behavior or markup. +- **Authority:** `contributor-docs/style.md` + +### `component-api.children-for-content` + +- **Check:** Content uses React children when consumers own the rendered elements or need free-form composition. +- **Prefer:** Use data props when the component must own, transform, order, or constrain the rendered elements. Do not support equivalent children and data APIs simultaneously without a concrete need. +- **Authority:** `contributor-docs/component-contents-api-patterns.md` + +### `component-api.no-speculative-parts` + +- **Check:** Compound parts, render props, child-prop pass-through APIs, and escape hatches address demonstrated use cases rather than speculative flexibility. +- **Prefer:** Start with the smallest API that supports known scenarios and open it up when a concrete need appears. +- **Authority:** `contributor-docs/adrs/adr-004-children-as-api.md` + +### `component-api.avoid-slots-by-default` + +- **Check:** Slots, useSlots, and __SLOT__ markers are only introduced when a parent must identify a specific child or the requested API requires child extraction. +- **Prefer:** Use normal React composition and preserve consumer-authored child order. +- **Authority:** `contributor-docs/style.md` + +### `component-api.reuse-base-primitives` + +- **Check:** Components reuse existing base components, accessibility primitives, hooks, and utilities before introducing parallel implementations. +- **Prefer:** Use primitives such as ButtonBase for Primer-owned native semantics, interactions, and reset styling. +- **Authority:** `contributor-docs/style.md` + +### `component-api.controlled-state` + +- **Check:** Components supporting controlled and uncontrolled state use the shared useControllableState pattern. +- **Prefer:** Keep controlled value, default value, and change callback behavior aligned with the repository helper. +- **Authority:** `contributor-docs/style.md` + +### `component-api.root-rest-props` + +- **Check:** Forwarded rest props and className are applied to the component root unless the public contract explicitly identifies another element. +- **Prefer:** Test that forwarded props reach the documented element. +- **Authority:** `contributor-docs/style.md` + +### `component-api.extensible-callbacks` + +- **Check:** New public callback props use a single object argument when the callback payload may grow. +- **Prefer:** Use a named object payload instead of positional arguments. +- **Authority:** `contributor-docs/style.md` + +### `component-api.ssr` + +- **Check:** Render paths avoid browser-only APIs and hydration-dependent layout changes. +- **Prefer:** Progressively enhance after hydration and move browser access into appropriate effects or event handlers. +- **Authority:** `contributor-docs/style.md` + +### `component-api.focus-from-events` + +- **Check:** Focus moves as a direct result of user events rather than unrelated effect dependency changes. +- **Prefer:** Manage focus in the event handler that caused the transition. +- **Authority:** `contributor-docs/style.md` + +### `component-api.hook-ref-input` + +- **Check:** Hooks that operate on a caller-owned DOM node accept a ref instead of creating and returning an incompatible ref. +- **Prefer:** Let callers compose multiple behaviors around the same ref. +- **Authority:** `contributor-docs/style.md` + +### `component-api.stable-hook-callbacks` + +- **Check:** Hook effects do not repeatedly resubscribe solely because a callback argument changed identity. +- **Prefer:** Use the repository stable-callback pattern so subscriptions always call the latest callback. +- **Authority:** `contributor-docs/style.md` + +### `component-api.public-hooks-need-consumers` + +- **Check:** New public hooks represent a clear consumer capability rather than exposing subcomponent implementation details. +- **Prefer:** Keep behavior hooks internal until a supported external use case and complete public contract exist. +- **Authority:** `contributor-docs/style.md` + diff --git a/.github/instructions/generated/component-structure.instructions.md b/.github/instructions/generated/component-structure.instructions.md new file mode 100644 index 00000000000..9e9ab732705 --- /dev/null +++ b/.github/instructions/generated/component-structure.instructions.md @@ -0,0 +1,36 @@ +--- +applyTo: 'packages/react/src/**/*' +--- + + + +# Component files and public surface + +Policy set: `component-structure`. Source digest: `21badb7273eb`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `component-structure.colocate-files` + +- **Check:** New components use a PascalCase directory and colocate implementation, CSS, tests, stories, docs metadata, and exports according to repository conventions. +- **Prefer:** Name subcomponent files with the parent component prefix for discoverability. +- **Authority:** `contributor-docs/adrs/adr-013-file-structure.md` + +### `component-structure.internal-modules` + +- **Check:** New shared modules that are not public API live under packages/react/src/internal and are not exported from public entrypoints. +- **Prefer:** Keep implementation details internal until a supported public API is intentionally designed. +- **Authority:** `contributor-docs/adrs/adr-015-internal-modules.md` + +### `component-structure.stable-identifiers` (advisory) + +- **Check:** New or changed component roots, public subcomponents, and meaningful structural parts follow the established data-component naming contract. +- **Prefer:** Use PascalCase component API names, keep state in separate data attributes, and test stable identifier values. +- **Authority:** `contributor-docs/adrs/adr-023-stable-selectors-api.md` + +### `component-structure.complete-public-surface` + +- **Check:** Public API changes update exports, export snapshots, docs metadata, tests, stories, and changesets as applicable. +- **Prefer:** Treat all public surfaces as one component contract. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + diff --git a/.github/instructions/generated/css.instructions.md b/.github/instructions/generated/css.instructions.md new file mode 100644 index 00000000000..0247be59d82 --- /dev/null +++ b/.github/instructions/generated/css.instructions.md @@ -0,0 +1,48 @@ +--- +applyTo: 'packages/react/src/**/*.css,packages/react/src/**/*.tsx' +--- + + + +# Component CSS + +Policy set: `css`. Source digest: `043e538ad1a2`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `css.modules` + +- **Check:** Component styles use colocated CSS Modules and Primer primitive custom properties. +- **Prefer:** Avoid new runtime-injected component styling and hardcoded design values. +- **Authority:** `contributor-docs/adrs/adr-016-css.md` + +### `css.root-classname` + +- **Check:** A consumer className is merged with clsx on the top-level element and cannot be overwritten by prop spread order. +- **Prefer:** Use CSS custom properties instead of multiple child className escape hatches. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.data-attributes-for-variants` + +- **Check:** Component variants and state use data attributes or native pseudo-classes rather than modifier class names. +- **Prefer:** Use :where() around data-attribute selectors to keep specificity low. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.javascript-bridge` + +- **Check:** Dynamic values crossing from JavaScript into CSS use custom properties instead of authored visual styles inline. +- **Prefer:** Keep layout and visual rules in the CSS Module. +- **Authority:** `contributor-docs/style.md` + +### `css.no-primitive-fallbacks` + +- **Check:** Primer primitive custom properties are used without manually authored fallback values. +- **Prefer:** Rely on the primitive build output to provide supported fallbacks. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.browser-support` + +- **Check:** CSS newer than Baseline 2022 includes an appropriate fallback and broadly matching selectors avoid expensive :has() invalidation. +- **Prefer:** Scope necessary :has() usage to a CSS Module class with a justified local stylelint disable. +- **Authority:** `.github/instructions/css.instructions.md` + diff --git a/.github/instructions/generated/deprecations-and-versioning.instructions.md b/.github/instructions/generated/deprecations-and-versioning.instructions.md new file mode 100644 index 00000000000..7c6a00072c2 --- /dev/null +++ b/.github/instructions/generated/deprecations-and-versioning.instructions.md @@ -0,0 +1,30 @@ +--- +applyTo: 'packages/react/src/**/*,.changeset/*.md' +--- + + + +# Deprecations, versioning, and changesets + +Policy set: `deprecations-and-versioning`. Source digest: `9d4c59fd70d0`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `deprecations-and-versioning.complete-deprecation-signal` + +- **Check:** Deprecations update JSDoc, docs metadata, migration guidance, and the recommended replacement where applicable. +- **Prefer:** Keep runtime behavior unchanged unless the change explicitly includes migration behavior. +- **Authority:** `.github/skills/deprecations/SKILL.md` + +### `deprecations-and-versioning.consumer-impact` + +- **Check:** The changeset bump matches the largest consumer-facing API, behavior, CSS, accessibility, or stable-identifier impact. +- **Prefer:** Use the repository versioning table and describe the outcome from the consumer perspective. +- **Authority:** `contributor-docs/versioning.md` + +### `deprecations-and-versioning.changeset-required` + +- **Check:** Published runtime or public API changes include an accurate changeset; docs-only, test-only, story-only, and infrastructure-only changes do not. +- **Prefer:** Keep each changeset terse and scoped to one consumer-facing change. +- **Authority:** `.github/skills/changesets/SKILL.md` + diff --git a/.github/instructions/generated/stories.instructions.md b/.github/instructions/generated/stories.instructions.md new file mode 100644 index 00000000000..d049e4bea82 --- /dev/null +++ b/.github/instructions/generated/stories.instructions.md @@ -0,0 +1,36 @@ +--- +applyTo: 'packages/react/src/**/*.stories.tsx' +--- + + + +# Storybook stories + +Policy set: `stories`. Source digest: `ce5b186845a5`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `stories.default-and-playground` + +- **Check:** The default story file uses the Components/ComponentName title and includes Default and Playground when configurable controls are supported. +- **Prefer:** Keep the primary documentation stories in ComponentName.stories.tsx. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + +### `stories.separate-features-and-examples` + +- **Check:** Feature coverage and scenario examples are separated from the default story file. +- **Prefer:** Use ComponentName.features.stories.tsx for features and ComponentName.examples.stories.tsx for distinct scenarios. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + +### `stories.document-supported-api` + +- **Check:** Stories demonstrate supported props, variants, and composition rather than inventing undocumented APIs. +- **Prefer:** Keep stories aligned with component docs metadata and real consumer-facing behavior. +- **Authority:** `.github/skills/storybook/SKILL.md` + +### `stories.keep-interactions-in-tests` + +- **Check:** Stories do not add play functions as a substitute for interaction coverage. +- **Prefer:** Exercise browser interactions through Playwright against the story and keep unit-level interactions in focused tests. +- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` + diff --git a/.github/instructions/generated/testing.instructions.md b/.github/instructions/generated/testing.instructions.md new file mode 100644 index 00000000000..fe2c0b0e4e9 --- /dev/null +++ b/.github/instructions/generated/testing.instructions.md @@ -0,0 +1,36 @@ +--- +applyTo: 'packages/react/src/**/*.test.ts,packages/react/src/**/*.test.tsx,e2e/**/*.test.ts' +--- + + + +# Component testing + +Policy set: `testing`. Source digest: `c5645620dafd`. + +Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse. + +### `testing.observable-behavior` + +- **Check:** Unit tests assert observable semantics and behavior from the user perspective rather than implementation details. +- **Prefer:** Use Testing Library roles, accessible names, userEvent, and explicit contract assertions. +- **Authority:** `contributor-docs/testing.md` + +### `testing.no-catch-all-render-snapshots` + +- **Check:** Tests do not add catch-all snapshots of rendered React trees. +- **Prefer:** Reserve snapshots for static objects with a clear review signal, such as package export lists. +- **Authority:** `contributor-docs/adrs/adr-011-snapshot-tests.md` + +### `testing.playwright-for-browser-behavior` + +- **Check:** Behavior requiring real layout, browser interaction, visual regression, or automated accessibility coverage is tested through the Playwright e2e suite. +- **Prefer:** Use @vrt and @avt coverage where visual appearance or accessibility output changes. +- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` + +### `testing.public-contract-coverage` + +- **Check:** New public props, forwarded DOM props, stable identifiers, keyboard behavior, and accessibility semantics have focused regression coverage. +- **Prefer:** Name tests for the contract they protect. +- **Authority:** `contributor-docs/testing.md` + diff --git a/.github/skills/primer-component-review/SKILL.md b/.github/skills/primer-component-review/SKILL.md new file mode 100644 index 00000000000..64e271323f2 --- /dev/null +++ b/.github/skills/primer-component-review/SKILL.md @@ -0,0 +1,44 @@ +--- +name: primer-component-review +description: 'Use when: reviewing Primer React component changes for repository architectural decisions and maintained component guidance. Covers component APIs, implementation structure, accessibility, stories, testing, CSS, deprecations, versioning, and changesets.' +--- + +# Primer component review + +Use this skill for code review of Primer React components and their supporting +files. + +## Sources + +Read `references/generated-policy.md` and apply only the policy sets relevant to +the changed files. Path-specific generated instructions under +`.github/instructions/generated/` contain the same rules scoped for Copilot code +review. + +Public ADRs and maintained contributor guides are authoritative. Rules labeled +`Primer architecture policy` contain only public-safe outcomes imported from a +private decision source. Do not seek, infer, mention, or disclose the private +source or rationale. + +## Review procedure + +1. Determine which public contracts and behaviors the change adds, removes, or + modifies. +2. Select the relevant policy sets from the generated reference. +3. Compare changed code and directly related tests, stories, docs, exports, and + changesets with enforced rules. +4. Apply advisory rules only when the change is designing or expanding the + affected contract and the mismatch has concrete consumer impact. +5. Report only findings introduced or made worse by the change. + +For each finding: + +- cite the changed file and line; +- cite the policy rule ID; +- describe the concrete consumer, accessibility, maintenance, or compatibility + impact; +- suggest the smallest design-level correction. + +Do not report formatting issues, preferences without policy support, or +pre-existing migration debt. If two active rules conflict, report the policy +conflict instead of choosing one. diff --git a/.github/skills/primer-component-review/references/generated-policy.md b/.github/skills/primer-component-review/references/generated-policy.md new file mode 100644 index 00000000000..c105a9ad90c --- /dev/null +++ b/.github/skills/primer-component-review/references/generated-policy.md @@ -0,0 +1,333 @@ + + +# Primer component review policy + +Apply only the policy sets relevant to the changed files. Enforced rules may produce findings. Advisory rules should only produce a finding when the change is designing or expanding the affected contract and the mismatch has a concrete consumer impact. + +## Component API language + +Applies to: `packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json,packages/react/src/**/*.hookDocs.json` + +### `architecture-api-language.semantic-size-names` (advisory) + +- **Check:** New public size, density, thickness, and viewport-range APIs use the established semantic vocabulary instead of abbreviations, CSS literals, or default as a size name. +- **Prefer:** Use descriptive full names and semantic values that remain meaningful across platforms. +- **Authority:** Primer architecture policy: `architecture-api-language.semantic-size-names` + +### `architecture-api-language.component-references` (advisory) + +- **Check:** Documentation and stories use the exported code name when referring to a specific React component and sentence case when discussing a generic interface pattern. +- **Prefer:** Keep API references unambiguous without presenting every generic pattern name as a code identifier. +- **Authority:** Primer architecture policy: `architecture-api-language.component-references` + +## Button layout semantics + +Applies to: `packages/react/src/**/*Button*.tsx,packages/react/src/**/*Button*.css,packages/react/src/SelectPanel/**/*.tsx,packages/react/src/experimental/SelectPanel2/**/*.tsx` + +### `architecture-button-layout.full-width-alignment` (advisory) + +- **Check:** A full-width call-to-action centers its content, while a full-width selection trigger aligns its primary content to the start and keeps any trailing affordance at the end. +- **Prefer:** Use alignment to communicate whether the control performs an action or exposes a selection. +- **Authority:** Primer architecture policy: `architecture-button-layout.full-width-alignment` + +## Semantic color and contrast + +Applies to: `packages/react/src/**/*.module.css` + +### `architecture-color.state-roles` (advisory) + +- **Check:** Open and closed states use their corresponding semantic functional color roles instead of success and danger roles. +- **Prefer:** Preserve the distinction between lifecycle state and validation or outcome semantics. +- **Authority:** Primer architecture policy: `architecture-color.state-roles` + +### `architecture-color.canonical-variants` + +- **Check:** The change does not introduce deprecated subtle functional color variants. +- **Prefer:** Use muted for secondary or less-prominent treatment and emphasis for stronger treatment. +- **Authority:** Primer architecture policy: `architecture-color.canonical-variants` + +### `architecture-color.text-and-links` (advisory) + +- **Check:** Non-decorative text and placeholders maintain at least 4.5:1 contrast against every supported background, and links remain distinguishable from surrounding text without relying only on color. +- **Prefer:** Use supported token pairs and retain a visible non-color link affordance. +- **Authority:** Primer architecture policy: `architecture-color.text-and-links` + +### `architecture-color.focus-and-state` (advisory) + +- **Check:** Focus indicators and simultaneously visible interactive states maintain at least 3:1 contrast against adjacent colors. +- **Prefer:** Verify contrast across supported themes without imposing a contrast requirement on purely decorative or inactive control borders. +- **Authority:** Primer architecture policy: `architecture-color.focus-and-state` + +## Notification patterns + +Applies to: `packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json` + +### `architecture-notifications.no-toast-pattern` + +- **Check:** The change does not add, restore, or recommend a toast component or toast recipe. +- **Prefer:** Use a supported feedback pattern appropriate to the interaction, such as a banner, inline validation, inline message, or dialog. +- **Authority:** Primer architecture policy: `architecture-notifications.no-toast-pattern` + +## SelectPanel interaction contract + +Applies to: `packages/react/src/SelectPanel/**/*` + +### `architecture-select-panel.focus-restoration` + +- **Check:** Opening moves focus to the configured initial target and closing restores focus to the trigger. +- **Prefer:** Focus the filter input by default while preserving explicit deferred-focus behavior and the component's established secondary-focus semantics. +- **Authority:** Primer architecture policy: `architecture-select-panel.focus-restoration` + +### `architecture-select-panel.commit-behavior` + +- **Check:** Anchored dismissal preserves the latest selection, while modal Save finalizes changes and modal cancellation or dismissal invokes the cancellation contract. +- **Prefer:** Preserve distinct anchored and modal commit semantics across every close path so consumers can restore the prior committed modal selection. +- **Authority:** Primer architecture policy: `architecture-select-panel.commit-behavior` + +### `architecture-select-panel.filter-focus` + +- **Check:** Input focus and filtering preserve a valid active option whenever option activation is enabled. +- **Prefer:** Keep the input and list behavior connected and normally activate the first available option after filtering. +- **Exceptions:** An explicit deferred-initial-focus configuration may wait for user interaction before assigning the active option. +- **Authority:** Primer architecture policy: `architecture-select-panel.filter-focus` + +### `architecture-select-panel.keyboard-navigation` + +- **Check:** Keyboard behavior supports toggling the active option, sequential tab navigation, directional option navigation, first and last option shortcuts, and Escape dismissal. +- **Prefer:** Maintain the complete interaction contract instead of adding isolated key handlers. +- **Authority:** Primer architecture policy: `architecture-select-panel.keyboard-navigation` + +### `architecture-select-panel.announcements` + +- **Check:** Screen-reader announcements communicate the active option, its position, and selection state without announcing on every filtering keystroke. +- **Prefer:** Use polite, delayed announcements and debounce filtering updates. +- **Authority:** Primer architecture policy: `architecture-select-panel.announcements` + +## Component API and implementation + +Applies to: `packages/react/src/**/*.ts,packages/react/src/**/*.tsx` + +### `component-api.spectrum-of-abstraction` + +- **Check:** New component APIs begin with flexible presentational components and behavior hooks unless the product pattern and its defaults are already stable. +- **Prefer:** Build config components by composing established presentational parts and hooks instead of duplicating their behavior or markup. +- **Authority:** `contributor-docs/style.md` + +### `component-api.children-for-content` + +- **Check:** Content uses React children when consumers own the rendered elements or need free-form composition. +- **Prefer:** Use data props when the component must own, transform, order, or constrain the rendered elements. Do not support equivalent children and data APIs simultaneously without a concrete need. +- **Authority:** `contributor-docs/component-contents-api-patterns.md` + +### `component-api.no-speculative-parts` + +- **Check:** Compound parts, render props, child-prop pass-through APIs, and escape hatches address demonstrated use cases rather than speculative flexibility. +- **Prefer:** Start with the smallest API that supports known scenarios and open it up when a concrete need appears. +- **Authority:** `contributor-docs/adrs/adr-004-children-as-api.md` + +### `component-api.avoid-slots-by-default` + +- **Check:** Slots, useSlots, and __SLOT__ markers are only introduced when a parent must identify a specific child or the requested API requires child extraction. +- **Prefer:** Use normal React composition and preserve consumer-authored child order. +- **Authority:** `contributor-docs/style.md` + +### `component-api.reuse-base-primitives` + +- **Check:** Components reuse existing base components, accessibility primitives, hooks, and utilities before introducing parallel implementations. +- **Prefer:** Use primitives such as ButtonBase for Primer-owned native semantics, interactions, and reset styling. +- **Authority:** `contributor-docs/style.md` + +### `component-api.controlled-state` + +- **Check:** Components supporting controlled and uncontrolled state use the shared useControllableState pattern. +- **Prefer:** Keep controlled value, default value, and change callback behavior aligned with the repository helper. +- **Authority:** `contributor-docs/style.md` + +### `component-api.root-rest-props` + +- **Check:** Forwarded rest props and className are applied to the component root unless the public contract explicitly identifies another element. +- **Prefer:** Test that forwarded props reach the documented element. +- **Authority:** `contributor-docs/style.md` + +### `component-api.extensible-callbacks` + +- **Check:** New public callback props use a single object argument when the callback payload may grow. +- **Prefer:** Use a named object payload instead of positional arguments. +- **Authority:** `contributor-docs/style.md` + +### `component-api.ssr` + +- **Check:** Render paths avoid browser-only APIs and hydration-dependent layout changes. +- **Prefer:** Progressively enhance after hydration and move browser access into appropriate effects or event handlers. +- **Authority:** `contributor-docs/style.md` + +### `component-api.focus-from-events` + +- **Check:** Focus moves as a direct result of user events rather than unrelated effect dependency changes. +- **Prefer:** Manage focus in the event handler that caused the transition. +- **Authority:** `contributor-docs/style.md` + +### `component-api.hook-ref-input` + +- **Check:** Hooks that operate on a caller-owned DOM node accept a ref instead of creating and returning an incompatible ref. +- **Prefer:** Let callers compose multiple behaviors around the same ref. +- **Authority:** `contributor-docs/style.md` + +### `component-api.stable-hook-callbacks` + +- **Check:** Hook effects do not repeatedly resubscribe solely because a callback argument changed identity. +- **Prefer:** Use the repository stable-callback pattern so subscriptions always call the latest callback. +- **Authority:** `contributor-docs/style.md` + +### `component-api.public-hooks-need-consumers` + +- **Check:** New public hooks represent a clear consumer capability rather than exposing subcomponent implementation details. +- **Prefer:** Keep behavior hooks internal until a supported external use case and complete public contract exist. +- **Authority:** `contributor-docs/style.md` + +## Component files and public surface + +Applies to: `packages/react/src/**/*` + +### `component-structure.colocate-files` + +- **Check:** New components use a PascalCase directory and colocate implementation, CSS, tests, stories, docs metadata, and exports according to repository conventions. +- **Prefer:** Name subcomponent files with the parent component prefix for discoverability. +- **Authority:** `contributor-docs/adrs/adr-013-file-structure.md` + +### `component-structure.internal-modules` + +- **Check:** New shared modules that are not public API live under packages/react/src/internal and are not exported from public entrypoints. +- **Prefer:** Keep implementation details internal until a supported public API is intentionally designed. +- **Authority:** `contributor-docs/adrs/adr-015-internal-modules.md` + +### `component-structure.stable-identifiers` (advisory) + +- **Check:** New or changed component roots, public subcomponents, and meaningful structural parts follow the established data-component naming contract. +- **Prefer:** Use PascalCase component API names, keep state in separate data attributes, and test stable identifier values. +- **Authority:** `contributor-docs/adrs/adr-023-stable-selectors-api.md` + +### `component-structure.complete-public-surface` + +- **Check:** Public API changes update exports, export snapshots, docs metadata, tests, stories, and changesets as applicable. +- **Prefer:** Treat all public surfaces as one component contract. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + +## Component CSS + +Applies to: `packages/react/src/**/*.css,packages/react/src/**/*.tsx` + +### `css.modules` + +- **Check:** Component styles use colocated CSS Modules and Primer primitive custom properties. +- **Prefer:** Avoid new runtime-injected component styling and hardcoded design values. +- **Authority:** `contributor-docs/adrs/adr-016-css.md` + +### `css.root-classname` + +- **Check:** A consumer className is merged with clsx on the top-level element and cannot be overwritten by prop spread order. +- **Prefer:** Use CSS custom properties instead of multiple child className escape hatches. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.data-attributes-for-variants` + +- **Check:** Component variants and state use data attributes or native pseudo-classes rather than modifier class names. +- **Prefer:** Use :where() around data-attribute selectors to keep specificity low. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.javascript-bridge` + +- **Check:** Dynamic values crossing from JavaScript into CSS use custom properties instead of authored visual styles inline. +- **Prefer:** Keep layout and visual rules in the CSS Module. +- **Authority:** `contributor-docs/style.md` + +### `css.no-primitive-fallbacks` + +- **Check:** Primer primitive custom properties are used without manually authored fallback values. +- **Prefer:** Rely on the primitive build output to provide supported fallbacks. +- **Authority:** `contributor-docs/authoring-css.md` + +### `css.browser-support` + +- **Check:** CSS newer than Baseline 2022 includes an appropriate fallback and broadly matching selectors avoid expensive :has() invalidation. +- **Prefer:** Scope necessary :has() usage to a CSS Module class with a justified local stylelint disable. +- **Authority:** `.github/instructions/css.instructions.md` + +## Deprecations, versioning, and changesets + +Applies to: `packages/react/src/**/*,.changeset/*.md` + +### `deprecations-and-versioning.complete-deprecation-signal` + +- **Check:** Deprecations update JSDoc, docs metadata, migration guidance, and the recommended replacement where applicable. +- **Prefer:** Keep runtime behavior unchanged unless the change explicitly includes migration behavior. +- **Authority:** `.github/skills/deprecations/SKILL.md` + +### `deprecations-and-versioning.consumer-impact` + +- **Check:** The changeset bump matches the largest consumer-facing API, behavior, CSS, accessibility, or stable-identifier impact. +- **Prefer:** Use the repository versioning table and describe the outcome from the consumer perspective. +- **Authority:** `contributor-docs/versioning.md` + +### `deprecations-and-versioning.changeset-required` + +- **Check:** Published runtime or public API changes include an accurate changeset; docs-only, test-only, story-only, and infrastructure-only changes do not. +- **Prefer:** Keep each changeset terse and scoped to one consumer-facing change. +- **Authority:** `.github/skills/changesets/SKILL.md` + +## Storybook stories + +Applies to: `packages/react/src/**/*.stories.tsx` + +### `stories.default-and-playground` + +- **Check:** The default story file uses the Components/ComponentName title and includes Default and Playground when configurable controls are supported. +- **Prefer:** Keep the primary documentation stories in ComponentName.stories.tsx. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + +### `stories.separate-features-and-examples` + +- **Check:** Feature coverage and scenario examples are separated from the default story file. +- **Prefer:** Use ComponentName.features.stories.tsx for features and ComponentName.examples.stories.tsx for distinct scenarios. +- **Authority:** `contributor-docs/CONTRIBUTING.md` + +### `stories.document-supported-api` + +- **Check:** Stories demonstrate supported props, variants, and composition rather than inventing undocumented APIs. +- **Prefer:** Keep stories aligned with component docs metadata and real consumer-facing behavior. +- **Authority:** `.github/skills/storybook/SKILL.md` + +### `stories.keep-interactions-in-tests` + +- **Check:** Stories do not add play functions as a substitute for interaction coverage. +- **Prefer:** Exercise browser interactions through Playwright against the story and keep unit-level interactions in focused tests. +- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` + +## Component testing + +Applies to: `packages/react/src/**/*.test.ts,packages/react/src/**/*.test.tsx,e2e/**/*.test.ts` + +### `testing.observable-behavior` + +- **Check:** Unit tests assert observable semantics and behavior from the user perspective rather than implementation details. +- **Prefer:** Use Testing Library roles, accessible names, userEvent, and explicit contract assertions. +- **Authority:** `contributor-docs/testing.md` + +### `testing.no-catch-all-render-snapshots` + +- **Check:** Tests do not add catch-all snapshots of rendered React trees. +- **Prefer:** Reserve snapshots for static objects with a clear review signal, such as package export lists. +- **Authority:** `contributor-docs/adrs/adr-011-snapshot-tests.md` + +### `testing.playwright-for-browser-behavior` + +- **Check:** Behavior requiring real layout, browser interaction, visual regression, or automated accessibility coverage is tested through the Playwright e2e suite. +- **Prefer:** Use @vrt and @avt coverage where visual appearance or accessibility output changes. +- **Authority:** `contributor-docs/adrs/adr-017-interaction-tests-revisited.md` + +### `testing.public-contract-coverage` + +- **Check:** New public props, forwarded DOM props, stable identifiers, keyboard behavior, and accessibility semantics have focused regression coverage. +- **Prefer:** Name tests for the contract they protect. +- **Authority:** `contributor-docs/testing.md` + diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 47069349b69..9b732b802a7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -27,6 +27,8 @@ jobs: cache: 'npm' - name: Install dependencies run: npm ci + - name: Check generated component review guidance + run: npm run test:review-guidance && npm run check:review-guidance - name: Check for unformatted files run: npm run format:diff diff --git a/.github/workflows/lint-autofix.yml b/.github/workflows/lint-autofix.yml index 63f876d067d..234d7a972c3 100644 --- a/.github/workflows/lint-autofix.yml +++ b/.github/workflows/lint-autofix.yml @@ -101,6 +101,10 @@ jobs: if: steps.check_jobs.outputs.should_fix == 'true' && steps.pr.outputs.head_ref != '' run: npm ci + - name: Regenerate component review guidance + if: steps.check_jobs.outputs.format_failed == 'true' && steps.pr.outputs.head_ref != '' + run: npm run build:review-guidance + - name: Run format auto-fix if: steps.check_jobs.outputs.format_failed == 'true' && steps.pr.outputs.head_ref != '' run: npm run format diff --git a/.prettierignore b/.prettierignore index a7d223c884b..2db784530e3 100644 --- a/.prettierignore +++ b/.prettierignore @@ -1,5 +1,7 @@ .changeset .agents +.github/instructions/generated +.github/skills/primer-component-review/references/generated-policy.md packages/react/CHANGELOG.md dist storybook-static diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000000..71f58e13e9b --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,22 @@ +# Primer React agent guidance + +## Component review + +For changes to Primer React components, hooks, stories, tests, CSS, docs metadata, +exports, or changesets: + +- Apply the path-specific instructions in + `.github/instructions/generated/*.instructions.md`. +- Use the `primer-component-review` skill for the review procedure and complete + generated policy catalog. +- Treat accepted public ADRs and maintained contributor guides as authoritative. + Treat imported Primer architecture policy as outcome-only guidance; never + attempt to discover, infer, or disclose its private rationale or source. +- Report only actionable mismatches introduced or expanded by the change. +- Cite the policy rule ID in each finding. +- Do not report pre-existing migration debt unless the change makes it worse. +- Advisory rules apply only while the affected API or contract is being designed + or expanded. + +When guidance conflicts, stop rather than guessing. Identify the conflicting +public rule IDs or source files so maintainers can resolve the policy. diff --git a/contributor-docs/adrs/adr-000-template.md b/contributor-docs/adrs/adr-000-template.md index 710316666e9..cabf4446f02 100644 --- a/contributor-docs/adrs/adr-000-template.md +++ b/contributor-docs/adrs/adr-000-template.md @@ -9,6 +9,14 @@ | Status | Proposed ❓ OR Accepted ✅ OR Superseded by [LINK](https://) ⚠️ OR Deprecated ⛔ | | Implementation | Not planned ⛔ OR Adoption awaiting in [LINK](https://) ⏸️ OR Adopted ✅ | +## Review impact + + + ## Context diff --git a/contributor-docs/review-guidance/README.md b/contributor-docs/review-guidance/README.md new file mode 100644 index 00000000000..5e4f80b26a2 --- /dev/null +++ b/contributor-docs/review-guidance/README.md @@ -0,0 +1,37 @@ +# Component review guidance + +The files in this directory are the structured source for generated Copilot code +review instructions. + +- `policy.json` records public rules derived from accepted ADRs and maintained + contributor documentation. +- `internal-policy.json` is the public-safe import boundary for architectural + outcomes produced outside this repository. It must never contain private + rationale, links, identifiers, titles, filenames, or quoted source text. + +Run `npm run build:review-guidance` after changing either policy. Generated +instructions live in `.github/instructions/generated/`, and the complete +reviewer reference lives in +`.github/skills/primer-component-review/references/generated-policy.md`. + +## Updating an ADR or contributor guide + +When a source changes an actionable component-development decision: + +1. Update, add, supersede, or remove the corresponding rule in `policy.json`. +2. Run `npm run accept:review-guidance-sources` after reviewing every rule mapped + to the changed source. +3. Run `npm run build:review-guidance`. +4. Run `npm run check:review-guidance`. + +If the source does not affect component review, no policy rule is needed. + +## Public-safe imported rules + +Imported rules use a strict outcome-only schema and omit all source fields. They +must contain only the outcome needed to review public code. The generator rejects +unknown fields, URLs, and known private-source markers in +`internal-policy.json`. + +The sanitizer is defense in depth, not a substitute for producing the import +from explicitly public-safe fields in the private source repository. diff --git a/contributor-docs/review-guidance/internal-policy.json b/contributor-docs/review-guidance/internal-policy.json new file mode 100644 index 00000000000..c5bb015132d --- /dev/null +++ b/contributor-docs/review-guidance/internal-policy.json @@ -0,0 +1,121 @@ +{ + "version": 1, + "ruleSets": [ + { + "id": "architecture-api-language", + "title": "Component API language", + "applyTo": "packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json,packages/react/src/**/*.hookDocs.json", + "rules": [ + { + "id": "architecture-api-language.semantic-size-names", + "enforcement": "advisory", + "check": "New public size, density, thickness, and viewport-range APIs use the established semantic vocabulary instead of abbreviations, CSS literals, or default as a size name.", + "prefer": "Use descriptive full names and semantic values that remain meaningful across platforms." + }, + { + "id": "architecture-api-language.component-references", + "enforcement": "advisory", + "check": "Documentation and stories use the exported code name when referring to a specific React component and sentence case when discussing a generic interface pattern.", + "prefer": "Keep API references unambiguous without presenting every generic pattern name as a code identifier." + } + ] + }, + { + "id": "architecture-button-layout", + "title": "Button layout semantics", + "applyTo": "packages/react/src/**/*Button*.tsx,packages/react/src/**/*Button*.css,packages/react/src/SelectPanel/**/*.tsx,packages/react/src/experimental/SelectPanel2/**/*.tsx", + "rules": [ + { + "id": "architecture-button-layout.full-width-alignment", + "enforcement": "advisory", + "check": "A full-width call-to-action centers its content, while a full-width selection trigger aligns its primary content to the start and keeps any trailing affordance at the end.", + "prefer": "Use alignment to communicate whether the control performs an action or exposes a selection." + } + ] + }, + { + "id": "architecture-notifications", + "title": "Notification patterns", + "applyTo": "packages/react/src/**/*.ts,packages/react/src/**/*.tsx,packages/react/src/**/*.docs.json", + "rules": [ + { + "id": "architecture-notifications.no-toast-pattern", + "enforcement": "enforce", + "check": "The change does not add, restore, or recommend a toast component or toast recipe.", + "prefer": "Use a supported feedback pattern appropriate to the interaction, such as a banner, inline validation, inline message, or dialog." + } + ] + }, + { + "id": "architecture-color", + "title": "Semantic color and contrast", + "applyTo": "packages/react/src/**/*.module.css", + "rules": [ + { + "id": "architecture-color.state-roles", + "enforcement": "advisory", + "check": "Open and closed states use their corresponding semantic functional color roles instead of success and danger roles.", + "prefer": "Preserve the distinction between lifecycle state and validation or outcome semantics." + }, + { + "id": "architecture-color.canonical-variants", + "enforcement": "enforce", + "check": "The change does not introduce deprecated subtle functional color variants.", + "prefer": "Use muted for secondary or less-prominent treatment and emphasis for stronger treatment." + }, + { + "id": "architecture-color.text-and-links", + "enforcement": "advisory", + "check": "Non-decorative text and placeholders maintain at least 4.5:1 contrast against every supported background, and links remain distinguishable from surrounding text without relying only on color.", + "prefer": "Use supported token pairs and retain a visible non-color link affordance." + }, + { + "id": "architecture-color.focus-and-state", + "enforcement": "advisory", + "check": "Focus indicators and simultaneously visible interactive states maintain at least 3:1 contrast against adjacent colors.", + "prefer": "Verify contrast across supported themes without imposing a contrast requirement on purely decorative or inactive control borders." + } + ] + }, + { + "id": "architecture-select-panel", + "title": "SelectPanel interaction contract", + "applyTo": "packages/react/src/SelectPanel/**/*", + "rules": [ + { + "id": "architecture-select-panel.focus-restoration", + "enforcement": "enforce", + "check": "Opening moves focus to the configured initial target and closing restores focus to the trigger.", + "prefer": "Focus the filter input by default while preserving explicit deferred-focus behavior and the component's established secondary-focus semantics." + }, + { + "id": "architecture-select-panel.commit-behavior", + "enforcement": "enforce", + "check": "Anchored dismissal preserves the latest selection, while modal Save finalizes changes and modal cancellation or dismissal invokes the cancellation contract.", + "prefer": "Preserve distinct anchored and modal commit semantics across every close path so consumers can restore the prior committed modal selection." + }, + { + "id": "architecture-select-panel.filter-focus", + "enforcement": "enforce", + "check": "Input focus and filtering preserve a valid active option whenever option activation is enabled.", + "prefer": "Keep the input and list behavior connected and normally activate the first available option after filtering.", + "exceptions": [ + "An explicit deferred-initial-focus configuration may wait for user interaction before assigning the active option." + ] + }, + { + "id": "architecture-select-panel.keyboard-navigation", + "enforcement": "enforce", + "check": "Keyboard behavior supports toggling the active option, sequential tab navigation, directional option navigation, first and last option shortcuts, and Escape dismissal.", + "prefer": "Maintain the complete interaction contract instead of adding isolated key handlers." + }, + { + "id": "architecture-select-panel.announcements", + "enforcement": "enforce", + "check": "Screen-reader announcements communicate the active option, its position, and selection state without announcing on every filtering keystroke.", + "prefer": "Use polite, delayed announcements and debounce filtering updates." + } + ] + } + ] +} diff --git a/contributor-docs/review-guidance/policy.json b/contributor-docs/review-guidance/policy.json new file mode 100644 index 00000000000..8538daf98f7 --- /dev/null +++ b/contributor-docs/review-guidance/policy.json @@ -0,0 +1,325 @@ +{ + "version": 1, + "ruleSets": [ + { + "id": "component-api", + "title": "Component API and implementation", + "applyTo": "packages/react/src/**/*.ts,packages/react/src/**/*.tsx", + "sources": [ + "contributor-docs/style.md", + "contributor-docs/component-contents-api-patterns.md", + "contributor-docs/adrs/adr-004-children-as-api.md" + ], + "rules": [ + { + "id": "component-api.spectrum-of-abstraction", + "enforcement": "enforce", + "check": "New component APIs begin with flexible presentational components and behavior hooks unless the product pattern and its defaults are already stable.", + "prefer": "Build config components by composing established presentational parts and hooks instead of duplicating their behavior or markup.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.children-for-content", + "enforcement": "enforce", + "check": "Content uses React children when consumers own the rendered elements or need free-form composition.", + "prefer": "Use data props when the component must own, transform, order, or constrain the rendered elements. Do not support equivalent children and data APIs simultaneously without a concrete need.", + "source": "contributor-docs/component-contents-api-patterns.md" + }, + { + "id": "component-api.no-speculative-parts", + "enforcement": "enforce", + "check": "Compound parts, render props, child-prop pass-through APIs, and escape hatches address demonstrated use cases rather than speculative flexibility.", + "prefer": "Start with the smallest API that supports known scenarios and open it up when a concrete need appears.", + "source": "contributor-docs/adrs/adr-004-children-as-api.md" + }, + { + "id": "component-api.avoid-slots-by-default", + "enforcement": "enforce", + "check": "Slots, useSlots, and __SLOT__ markers are only introduced when a parent must identify a specific child or the requested API requires child extraction.", + "prefer": "Use normal React composition and preserve consumer-authored child order.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.reuse-base-primitives", + "enforcement": "enforce", + "check": "Components reuse existing base components, accessibility primitives, hooks, and utilities before introducing parallel implementations.", + "prefer": "Use primitives such as ButtonBase for Primer-owned native semantics, interactions, and reset styling.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.controlled-state", + "enforcement": "enforce", + "check": "Components supporting controlled and uncontrolled state use the shared useControllableState pattern.", + "prefer": "Keep controlled value, default value, and change callback behavior aligned with the repository helper.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.root-rest-props", + "enforcement": "enforce", + "check": "Forwarded rest props and className are applied to the component root unless the public contract explicitly identifies another element.", + "prefer": "Test that forwarded props reach the documented element.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.extensible-callbacks", + "enforcement": "enforce", + "check": "New public callback props use a single object argument when the callback payload may grow.", + "prefer": "Use a named object payload instead of positional arguments.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.ssr", + "enforcement": "enforce", + "check": "Render paths avoid browser-only APIs and hydration-dependent layout changes.", + "prefer": "Progressively enhance after hydration and move browser access into appropriate effects or event handlers.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.focus-from-events", + "enforcement": "enforce", + "check": "Focus moves as a direct result of user events rather than unrelated effect dependency changes.", + "prefer": "Manage focus in the event handler that caused the transition.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.hook-ref-input", + "enforcement": "enforce", + "check": "Hooks that operate on a caller-owned DOM node accept a ref instead of creating and returning an incompatible ref.", + "prefer": "Let callers compose multiple behaviors around the same ref.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.stable-hook-callbacks", + "enforcement": "enforce", + "check": "Hook effects do not repeatedly resubscribe solely because a callback argument changed identity.", + "prefer": "Use the repository stable-callback pattern so subscriptions always call the latest callback.", + "source": "contributor-docs/style.md" + }, + { + "id": "component-api.public-hooks-need-consumers", + "enforcement": "enforce", + "check": "New public hooks represent a clear consumer capability rather than exposing subcomponent implementation details.", + "prefer": "Keep behavior hooks internal until a supported external use case and complete public contract exist.", + "source": "contributor-docs/style.md" + } + ], + "sourceDigest": "e2ac6c7a9525" + }, + { + "id": "component-structure", + "title": "Component files and public surface", + "applyTo": "packages/react/src/**/*", + "sources": [ + "contributor-docs/CONTRIBUTING.md", + "contributor-docs/adrs/adr-013-file-structure.md", + "contributor-docs/adrs/adr-015-internal-modules.md", + "contributor-docs/adrs/adr-023-stable-selectors-api.md" + ], + "rules": [ + { + "id": "component-structure.colocate-files", + "enforcement": "enforce", + "check": "New components use a PascalCase directory and colocate implementation, CSS, tests, stories, docs metadata, and exports according to repository conventions.", + "prefer": "Name subcomponent files with the parent component prefix for discoverability.", + "source": "contributor-docs/adrs/adr-013-file-structure.md" + }, + { + "id": "component-structure.internal-modules", + "enforcement": "enforce", + "check": "New shared modules that are not public API live under packages/react/src/internal and are not exported from public entrypoints.", + "prefer": "Keep implementation details internal until a supported public API is intentionally designed.", + "source": "contributor-docs/adrs/adr-015-internal-modules.md" + }, + { + "id": "component-structure.stable-identifiers", + "enforcement": "advisory", + "check": "New or changed component roots, public subcomponents, and meaningful structural parts follow the established data-component naming contract.", + "prefer": "Use PascalCase component API names, keep state in separate data attributes, and test stable identifier values.", + "source": "contributor-docs/adrs/adr-023-stable-selectors-api.md" + }, + { + "id": "component-structure.complete-public-surface", + "enforcement": "enforce", + "check": "Public API changes update exports, export snapshots, docs metadata, tests, stories, and changesets as applicable.", + "prefer": "Treat all public surfaces as one component contract.", + "source": "contributor-docs/CONTRIBUTING.md" + } + ], + "sourceDigest": "21badb7273eb" + }, + { + "id": "stories", + "title": "Storybook stories", + "applyTo": "packages/react/src/**/*.stories.tsx", + "sources": [ + "contributor-docs/CONTRIBUTING.md", + "contributor-docs/adrs/adr-017-interaction-tests-revisited.md", + ".github/skills/storybook/SKILL.md" + ], + "rules": [ + { + "id": "stories.default-and-playground", + "enforcement": "enforce", + "check": "The default story file uses the Components/ComponentName title and includes Default and Playground when configurable controls are supported.", + "prefer": "Keep the primary documentation stories in ComponentName.stories.tsx.", + "source": "contributor-docs/CONTRIBUTING.md" + }, + { + "id": "stories.separate-features-and-examples", + "enforcement": "enforce", + "check": "Feature coverage and scenario examples are separated from the default story file.", + "prefer": "Use ComponentName.features.stories.tsx for features and ComponentName.examples.stories.tsx for distinct scenarios.", + "source": "contributor-docs/CONTRIBUTING.md" + }, + { + "id": "stories.document-supported-api", + "enforcement": "enforce", + "check": "Stories demonstrate supported props, variants, and composition rather than inventing undocumented APIs.", + "prefer": "Keep stories aligned with component docs metadata and real consumer-facing behavior.", + "source": ".github/skills/storybook/SKILL.md" + }, + { + "id": "stories.keep-interactions-in-tests", + "enforcement": "enforce", + "check": "Stories do not add play functions as a substitute for interaction coverage.", + "prefer": "Exercise browser interactions through Playwright against the story and keep unit-level interactions in focused tests.", + "source": "contributor-docs/adrs/adr-017-interaction-tests-revisited.md" + } + ], + "sourceDigest": "ce5b186845a5" + }, + { + "id": "testing", + "title": "Component testing", + "applyTo": "packages/react/src/**/*.test.ts,packages/react/src/**/*.test.tsx,e2e/**/*.test.ts", + "sources": [ + "contributor-docs/testing.md", + "contributor-docs/adrs/adr-011-snapshot-tests.md", + "contributor-docs/adrs/adr-017-interaction-tests-revisited.md" + ], + "rules": [ + { + "id": "testing.observable-behavior", + "enforcement": "enforce", + "check": "Unit tests assert observable semantics and behavior from the user perspective rather than implementation details.", + "prefer": "Use Testing Library roles, accessible names, userEvent, and explicit contract assertions.", + "source": "contributor-docs/testing.md" + }, + { + "id": "testing.no-catch-all-render-snapshots", + "enforcement": "enforce", + "check": "Tests do not add catch-all snapshots of rendered React trees.", + "prefer": "Reserve snapshots for static objects with a clear review signal, such as package export lists.", + "source": "contributor-docs/adrs/adr-011-snapshot-tests.md" + }, + { + "id": "testing.playwright-for-browser-behavior", + "enforcement": "enforce", + "check": "Behavior requiring real layout, browser interaction, visual regression, or automated accessibility coverage is tested through the Playwright e2e suite.", + "prefer": "Use @vrt and @avt coverage where visual appearance or accessibility output changes.", + "source": "contributor-docs/adrs/adr-017-interaction-tests-revisited.md" + }, + { + "id": "testing.public-contract-coverage", + "enforcement": "enforce", + "check": "New public props, forwarded DOM props, stable identifiers, keyboard behavior, and accessibility semantics have focused regression coverage.", + "prefer": "Name tests for the contract they protect.", + "source": "contributor-docs/testing.md" + } + ], + "sourceDigest": "c5645620dafd" + }, + { + "id": "css", + "title": "Component CSS", + "applyTo": "packages/react/src/**/*.css,packages/react/src/**/*.tsx", + "sources": [ + "contributor-docs/style.md", + "contributor-docs/authoring-css.md", + "contributor-docs/adrs/adr-016-css.md", + ".github/instructions/css.instructions.md" + ], + "rules": [ + { + "id": "css.modules", + "enforcement": "enforce", + "check": "Component styles use colocated CSS Modules and Primer primitive custom properties.", + "prefer": "Avoid new runtime-injected component styling and hardcoded design values.", + "source": "contributor-docs/adrs/adr-016-css.md" + }, + { + "id": "css.root-classname", + "enforcement": "enforce", + "check": "A consumer className is merged with clsx on the top-level element and cannot be overwritten by prop spread order.", + "prefer": "Use CSS custom properties instead of multiple child className escape hatches.", + "source": "contributor-docs/authoring-css.md" + }, + { + "id": "css.data-attributes-for-variants", + "enforcement": "enforce", + "check": "Component variants and state use data attributes or native pseudo-classes rather than modifier class names.", + "prefer": "Use :where() around data-attribute selectors to keep specificity low.", + "source": "contributor-docs/authoring-css.md" + }, + { + "id": "css.javascript-bridge", + "enforcement": "enforce", + "check": "Dynamic values crossing from JavaScript into CSS use custom properties instead of authored visual styles inline.", + "prefer": "Keep layout and visual rules in the CSS Module.", + "source": "contributor-docs/style.md" + }, + { + "id": "css.no-primitive-fallbacks", + "enforcement": "enforce", + "check": "Primer primitive custom properties are used without manually authored fallback values.", + "prefer": "Rely on the primitive build output to provide supported fallbacks.", + "source": "contributor-docs/authoring-css.md" + }, + { + "id": "css.browser-support", + "enforcement": "enforce", + "check": "CSS newer than Baseline 2022 includes an appropriate fallback and broadly matching selectors avoid expensive :has() invalidation.", + "prefer": "Scope necessary :has() usage to a CSS Module class with a justified local stylelint disable.", + "source": ".github/instructions/css.instructions.md" + } + ], + "sourceDigest": "043e538ad1a2" + }, + { + "id": "deprecations-and-versioning", + "title": "Deprecations, versioning, and changesets", + "applyTo": "packages/react/src/**/*,.changeset/*.md", + "sources": [ + "contributor-docs/deprecating-components.md", + "contributor-docs/versioning.md", + "contributor-docs/adrs/adr-019-deprecating-props.md", + ".github/skills/deprecations/SKILL.md", + ".github/skills/changesets/SKILL.md" + ], + "rules": [ + { + "id": "deprecations-and-versioning.complete-deprecation-signal", + "enforcement": "enforce", + "check": "Deprecations update JSDoc, docs metadata, migration guidance, and the recommended replacement where applicable.", + "prefer": "Keep runtime behavior unchanged unless the change explicitly includes migration behavior.", + "source": ".github/skills/deprecations/SKILL.md" + }, + { + "id": "deprecations-and-versioning.consumer-impact", + "enforcement": "enforce", + "check": "The changeset bump matches the largest consumer-facing API, behavior, CSS, accessibility, or stable-identifier impact.", + "prefer": "Use the repository versioning table and describe the outcome from the consumer perspective.", + "source": "contributor-docs/versioning.md" + }, + { + "id": "deprecations-and-versioning.changeset-required", + "enforcement": "enforce", + "check": "Published runtime or public API changes include an accurate changeset; docs-only, test-only, story-only, and infrastructure-only changes do not.", + "prefer": "Keep each changeset terse and scoped to one consumer-facing change.", + "source": ".github/skills/changesets/SKILL.md" + } + ], + "sourceDigest": "9d4c59fd70d0" + } + ] +} diff --git a/package.json b/package.json index fcaaf5ac39b..393f3997814 100644 --- a/package.json +++ b/package.json @@ -22,12 +22,16 @@ "build:docs:preview": "NODE_OPTIONS=--openssl-legacy-provider script/build-docs preview", "build:components.json": "npm run build:components.json -w @primer/react", "build:hooks.json": "npm run build:hooks.json -w @primer/react", + "build:review-guidance": "node script/review-guidance.mjs", + "accept:review-guidance-sources": "node script/review-guidance.mjs --accept-sources", + "check:review-guidance": "node script/review-guidance.mjs --check", "lint": "eslint '**/*.{js,ts,tsx,md,mdx}' --max-warnings=0 --cache", "lint:css": "stylelint --rd -q --cache '**/*.css'", "lint:css:fix": "npm run lint:css -- --fix", "lint:fix": "npm run lint -- --fix", "lint:md": "markdownlint-cli2 \"**/*.{md,mdx}\" \"!.github\" \"!.changeset\" \"!.agents\" \"!**/node_modules/**\" \"!**/CHANGELOG.md\"", "test": "vitest", + "test:review-guidance": "node --test script/review-guidance.test.mjs", "test:type-check": "tsc --noEmit", "test:update": "npm run test -- -u", "test:classname-coverage": "node script/check-classname-tests.mjs", diff --git a/script/review-guidance.mjs b/script/review-guidance.mjs new file mode 100644 index 00000000000..a73c23f79aa --- /dev/null +++ b/script/review-guidance.mjs @@ -0,0 +1,276 @@ +import {createHash} from 'node:crypto' +import {existsSync, mkdirSync, readFileSync, readdirSync, rmSync, writeFileSync} from 'node:fs' +import {dirname, join, relative, resolve} from 'node:path' +import {fileURLToPath, pathToFileURL} from 'node:url' + +const scriptDirectory = dirname(fileURLToPath(import.meta.url)) +const repositoryRoot = resolve(scriptDirectory, '..') +const publicPolicyPath = join(repositoryRoot, 'contributor-docs/review-guidance/policy.json') +const internalPolicyPath = join(repositoryRoot, 'contributor-docs/review-guidance/internal-policy.json') +const instructionsDirectory = join(repositoryRoot, '.github/instructions/generated') +const skillReferencePath = join(repositoryRoot, '.github/skills/primer-component-review/references/generated-policy.md') +const generatedMarker = '' + +function readJson(path) { + return JSON.parse(readFileSync(path, 'utf8')) +} + +export function sourceDigest(sourcePaths) { + const hash = createHash('sha256') + for (const sourcePath of sourcePaths) { + hash.update(sourcePath) + hash.update(readFileSync(join(repositoryRoot, sourcePath))) + } + return hash.digest('hex').slice(0, 12) +} + +function assertAllowedKeys(value, allowedKeys, label) { + for (const key of Object.keys(value)) { + assert(allowedKeys.includes(key), `${label} contains unsupported field: ${key}`) + } +} + +function assert(condition, message) { + if (!condition) { + throw new Error(message) + } +} + +export function validatePolicies(publicPolicy, internalPolicy) { + assertAllowedKeys(publicPolicy, ['version', 'ruleSets'], 'Public policy') + assertAllowedKeys(internalPolicy, ['version', 'ruleSets'], 'Internal policy') + assert(publicPolicy.version === 1, 'Public policy must use version 1') + assert(internalPolicy.version === 1, 'Internal policy must use version 1') + assert(Array.isArray(publicPolicy.ruleSets), 'Public policy must include ruleSets') + assert(Array.isArray(internalPolicy.ruleSets), 'Internal policy must include ruleSets') + + const ruleSetIds = new Set() + const ruleIds = new Set() + const supersededRuleIds = new Set() + const privateText = JSON.stringify(internalPolicy) + const forbiddenPrivatePatterns = [ + /https?:\/\//i, + /github\/primer/i, + /slack/i, + /figma/i, + /discussion/i, + /issue[#\s-]*\d+/i, + /rationale/i, + /alternative/i, + /consequence/i, + ] + + for (const pattern of forbiddenPrivatePatterns) { + assert(!pattern.test(privateText), `Internal policy contains forbidden private-source text: ${pattern}`) + } + + for (const [policyName, policy] of [ + ['public', publicPolicy], + ['internal', internalPolicy], + ]) { + for (const ruleSet of policy.ruleSets) { + assertAllowedKeys( + ruleSet, + policyName === 'public' + ? ['id', 'title', 'applyTo', 'sources', 'sourceDigest', 'rules'] + : ['id', 'title', 'applyTo', 'rules'], + `${policyName} rule set`, + ) + assert(typeof ruleSet.id === 'string' && ruleSet.id.length > 0, `${policyName} rule set needs an id`) + assert(!ruleSetIds.has(ruleSet.id), `Duplicate rule set id: ${ruleSet.id}`) + ruleSetIds.add(ruleSet.id) + assert(typeof ruleSet.title === 'string' && ruleSet.title.length > 0, `${ruleSet.id} needs a title`) + assert(typeof ruleSet.applyTo === 'string' && ruleSet.applyTo.length > 0, `${ruleSet.id} needs applyTo`) + assert(Array.isArray(ruleSet.rules) && ruleSet.rules.length > 0, `${ruleSet.id} needs rules`) + + if (policyName === 'public') { + assert(Array.isArray(ruleSet.sources) && ruleSet.sources.length > 0, `${ruleSet.id} needs sources`) + for (const source of ruleSet.sources) { + assert(existsSync(join(repositoryRoot, source)), `Missing source for ${ruleSet.id}: ${source}`) + } + assert( + ruleSet.sourceDigest === sourceDigest(ruleSet.sources), + `Sources changed for ${ruleSet.id}. Review its rules, then run npm run accept:review-guidance-sources.`, + ) + } else { + assert(ruleSet.sources === undefined, `Internal rule set ${ruleSet.id} must not expose sources`) + } + + for (const rule of ruleSet.rules) { + assertAllowedKeys( + rule, + policyName === 'public' + ? ['id', 'enforcement', 'check', 'prefer', 'exceptions', 'source', 'supersedes'] + : ['id', 'enforcement', 'check', 'prefer', 'exceptions', 'supersedes'], + `${ruleSet.id} rule`, + ) + assert(typeof rule.id === 'string' && rule.id.startsWith(`${ruleSet.id}.`), `Invalid rule id: ${rule.id}`) + assert(!ruleIds.has(rule.id), `Duplicate rule id: ${rule.id}`) + ruleIds.add(rule.id) + assert(['enforce', 'advisory'].includes(rule.enforcement), `Invalid enforcement for ${rule.id}`) + assert(typeof rule.check === 'string' && rule.check.length > 0, `${rule.id} needs a check`) + assert(typeof rule.prefer === 'string' && rule.prefer.length > 0, `${rule.id} needs a preference`) + if (rule.exceptions !== undefined) { + assert(Array.isArray(rule.exceptions), `${rule.id} exceptions must be an array`) + } + if (policyName === 'public') { + assert(ruleSet.sources.includes(rule.source), `${rule.id} needs an exact source from its rule set`) + } + if (rule.supersedes !== undefined) { + assert(Array.isArray(rule.supersedes), `${rule.id} supersedes must be an array`) + for (const supersededRuleId of rule.supersedes) { + assert(!supersededRuleIds.has(supersededRuleId), `${supersededRuleId} is superseded more than once`) + supersededRuleIds.add(supersededRuleId) + } + } + } + } + } + + for (const supersededRuleId of supersededRuleIds) { + assert(ruleIds.has(supersededRuleId), `Unknown superseded rule id: ${supersededRuleId}`) + } +} + +function mergeRuleSets(publicPolicy, internalPolicy) { + return [...publicPolicy.ruleSets, ...internalPolicy.ruleSets].sort((a, b) => a.id.localeCompare(b.id)) +} + +function renderRule(rule, sourceLabel) { + const lines = [ + `### \`${rule.id}\`${rule.enforcement === 'advisory' ? ' (advisory)' : ''}`, + '', + `- **Check:** ${rule.check}`, + `- **Prefer:** ${rule.prefer}`, + ] + + if (rule.exceptions?.length) { + lines.push(`- **Exceptions:** ${rule.exceptions.join(' ')}`) + } + + lines.push(`- **Authority:** ${sourceLabel}`, '') + return lines.join('\n') +} + +export function renderInstruction(ruleSet) { + const digest = ruleSet.sourceDigest ?? 'sanitized-import' + return [ + '---', + `applyTo: '${ruleSet.applyTo}'`, + '---', + '', + generatedMarker, + '', + `# ${ruleSet.title}`, + '', + `Policy set: \`${ruleSet.id}\`. Source digest: \`${digest}\`.`, + '', + 'Review changed code against these rules. Report only concrete, actionable mismatches introduced or expanded by the change. Cite the rule ID in each finding. Do not report pre-existing migration debt unless the change makes it worse.', + '', + ...ruleSet.rules.map(rule => + renderRule(rule, rule.source ? `\`${rule.source}\`` : `Primer architecture policy: \`${rule.id}\``), + ), + ].join('\n') +} + +function renderSkillReference(ruleSets) { + return [ + generatedMarker, + '', + '# Primer component review policy', + '', + 'Apply only the policy sets relevant to the changed files. Enforced rules may produce findings. Advisory rules should only produce a finding when the change is designing or expanding the affected contract and the mismatch has a concrete consumer impact.', + '', + ...ruleSets.flatMap(ruleSet => { + return [ + `## ${ruleSet.title}`, + '', + `Applies to: \`${ruleSet.applyTo}\``, + '', + ...ruleSet.rules.map(rule => + renderRule(rule, rule.source ? `\`${rule.source}\`` : `Primer architecture policy: \`${rule.id}\``), + ), + ] + }), + ].join('\n') +} + +function expectedFiles(ruleSets) { + const files = new Map() + for (const ruleSet of ruleSets) { + files.set(join(instructionsDirectory, `${ruleSet.id}.instructions.md`), renderInstruction(ruleSet)) + } + files.set(skillReferencePath, renderSkillReference(ruleSets)) + return files +} + +function checkFiles(files) { + const staleFiles = [] + for (const [path, contents] of files) { + if (!existsSync(path) || readFileSync(path, 'utf8') !== `${contents}\n`) { + staleFiles.push(relative(repositoryRoot, path)) + } + } + + if (existsSync(instructionsDirectory)) { + for (const filename of readdirSync(instructionsDirectory)) { + const path = join(instructionsDirectory, filename) + if (filename.endsWith('.instructions.md') && !files.has(path)) { + staleFiles.push(relative(repositoryRoot, path)) + } + } + } + + if (staleFiles.length > 0) { + throw new Error( + `Generated review guidance is out of date:\n${staleFiles.map(path => `- ${path}`).join('\n')}\nRun npm run build:review-guidance.`, + ) + } +} + +function writeFiles(files) { + mkdirSync(instructionsDirectory, {recursive: true}) + mkdirSync(dirname(skillReferencePath), {recursive: true}) + + for (const filename of readdirSync(instructionsDirectory)) { + const path = join(instructionsDirectory, filename) + if (filename.endsWith('.instructions.md') && !files.has(path)) { + rmSync(path) + } + } + + for (const [path, contents] of files) { + writeFileSync(path, `${contents}\n`) + } +} + +function acceptSourceChanges(publicPolicy) { + for (const ruleSet of publicPolicy.ruleSets) { + ruleSet.sourceDigest = sourceDigest(ruleSet.sources) + } + writeFileSync(publicPolicyPath, `${JSON.stringify(publicPolicy, null, 2)}\n`) +} + +export function build({acceptSources = false, check = false} = {}) { + const publicPolicy = readJson(publicPolicyPath) + const internalPolicy = readJson(internalPolicyPath) + if (acceptSources) { + acceptSourceChanges(publicPolicy) + } + validatePolicies(publicPolicy, internalPolicy) + const files = expectedFiles(mergeRuleSets(publicPolicy, internalPolicy)) + + if (check) { + checkFiles(files) + } else { + writeFiles(files) + } +} + +const isMain = process.argv[1] && import.meta.url === pathToFileURL(resolve(process.argv[1])).href +if (isMain) { + build({ + acceptSources: process.argv.includes('--accept-sources'), + check: process.argv.includes('--check'), + }) +} diff --git a/script/review-guidance.test.mjs b/script/review-guidance.test.mjs new file mode 100644 index 00000000000..7c47d2c5f22 --- /dev/null +++ b/script/review-guidance.test.mjs @@ -0,0 +1,74 @@ +import assert from 'node:assert/strict' +import {test} from 'node:test' +import {renderInstruction, sourceDigest, validatePolicies} from './review-guidance.mjs' + +const validPublicPolicy = { + version: 1, + ruleSets: [ + { + id: 'example', + title: 'Example', + applyTo: 'packages/react/src/**/*.tsx', + sources: ['contributor-docs/style.md'], + sourceDigest: sourceDigest(['contributor-docs/style.md']), + rules: [ + { + id: 'example.rule', + enforcement: 'enforce', + check: 'Check the example.', + prefer: 'Prefer the supported example.', + source: 'contributor-docs/style.md', + }, + ], + }, + ], +} + +test('validates a public policy and sanitized internal import', () => { + assert.doesNotThrow(() => validatePolicies(validPublicPolicy, {version: 1, ruleSets: []})) +}) + +test('rejects private source details from imported policy', () => { + assert.throws( + () => + validatePolicies(validPublicPolicy, { + version: 1, + ruleSets: [ + { + id: 'private', + title: 'Private', + applyTo: '**/*.tsx', + rules: [ + { + id: 'private.rule', + enforcement: 'enforce', + check: 'See https://example.com for the decision.', + prefer: 'Use the decision.', + }, + ], + }, + ], + }), + /forbidden private-source text/, + ) +}) + +test('renders a path-scoped instruction with a rule citation', () => { + const output = renderInstruction(validPublicPolicy.ruleSets[0]) + assert.match(output, /applyTo: 'packages\/react\/src\/\*\*\/\*\.tsx'/) + assert.match(output, /`example\.rule`/) + assert.match(output, /contributor-docs\/style\.md/) +}) + +test('rejects undeclared fields from imported policy', () => { + assert.throws( + () => validatePolicies(validPublicPolicy, {version: 1, ruleSets: [], privateSource: 'hidden'}), + /unsupported field/, + ) +}) + +test('rejects public policy when a mapped source changed', () => { + const stalePolicy = structuredClone(validPublicPolicy) + stalePolicy.ruleSets[0].sourceDigest = 'stale' + assert.throws(() => validatePolicies(stalePolicy, {version: 1, ruleSets: []}), /Sources changed for example/) +}) From e9ca907171121b53c529b93ebc5d4555dc5c6c87 Mon Sep 17 00:00:00 2001 From: Adam Dierkens Date: Thu, 16 Jul 2026 15:54:06 -0400 Subject: [PATCH 6/6] Replace trial rules with generated policy Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68a3cde5-7a50-4d4e-97d9-194c8b112e88 --- .../component-review.instructions.md | 43 ------------------- 1 file changed, 43 deletions(-) delete mode 100644 .github/instructions/component-review.instructions.md diff --git a/.github/instructions/component-review.instructions.md b/.github/instructions/component-review.instructions.md deleted file mode 100644 index 288f2c7da6e..00000000000 --- a/.github/instructions/component-review.instructions.md +++ /dev/null @@ -1,43 +0,0 @@ ---- -applyTo: 'packages/react/src/**/*' ---- - -# ADR-backed component review trial - -Apply these rules only to behavior or contracts introduced or expanded by the -change. Report concrete impact, cite the rule ID, and do not report pre-existing -migration debt. - -## `component-review.children-as-api` - -- **Check:** Content uses React children when consumers own the rendered - elements or need free-form composition. -- **Prefer:** Use data props when the component must own, transform, order, or - constrain the rendered elements. Do not support equivalent children and data - APIs without a concrete need. -- **Authority:** `contributor-docs/adrs/adr-004-children-as-api.md` - -## `component-review.file-structure` - -- **Check:** New components use a PascalCase directory and colocate - applicable implementation, styles, tests, stories, docs metadata, and exports - according to repository conventions. -- **Prefer:** Name subcomponent files with the parent component prefix for - discoverability. -- **Authority:** `contributor-docs/adrs/adr-013-file-structure.md` - -## `component-review.internal-modules` - -- **Check:** New shared modules that are not public API live under - `packages/react/src/internal` and are not exported from public entrypoints. -- **Prefer:** Keep implementation details internal until a supported public API - is intentionally designed. -- **Authority:** `contributor-docs/adrs/adr-015-internal-modules.md` - -## `component-review.stable-identifiers` (advisory) - -- **Check:** Newly added component roots, public subcomponents, and meaningful - structural parts follow the established `data-component` naming contract. -- **Prefer:** Use PascalCase component API names, keep state in separate data - attributes, and test stable identifier values. -- **Authority:** `contributor-docs/adrs/adr-023-stable-selectors-api.md`