ECHOES-1372 Add LLM guidelines and gitar review guidelines

.agents/skills/echoes-review/SKILL.md

@@ -0,0 +1,121 @@
+---
+name: echoes-review
+description: Echoes review — analyse your current branch or an existing PR for naming violations, a11y gaps, design-token misuse, missing exports, and architectural issues in the Echoes React component library.
+---
+
+Run echoes review: $ARGUMENTS
+
+Parse arguments:
+
+- `--pr <number or URL>` — review a specific PR's diff instead of the current branch; accepts a bare number (`123`) or a full GitHub URL — extract the number from the URL path before proceeding; `--base` is ignored when this is set since `gh pr diff` always diffs against the PR's own base branch
+- `--base <branch>` — override the base branch when diffing the current branch (default: auto-detected main or master); has no effect when `--pr` is provided
+
+---
+
+## Step 1: Load review guidelines
+
+Read all files under `.gitar/review/` — they contain the authoritative rules for this review:
+
+@.gitar/review/general-design.md
+@.gitar/review/naming-and-api.md
+@.gitar/review/component-patterns.md
+@.gitar/review/folder-and-exports.md
+@.gitar/review/radix-ui.md
+@.gitar/review/design-tokens.md
+@.gitar/review/accessibility.md
+@.gitar/review/testing.md
+@.gitar/review/localization.md
+@.gitar/review/stories-and-figma.md
+
+---
+
+## Step 2: Get the diff
+
+The two modes use **different data sources**. Do not mix them.
+
+### Mode A: `--pr`
+
+```bash
+git fetch origin pull/{pr}/head:pr-{pr}
+
+gh pr diff {pr} --name-only
+gh pr diff {pr}
+```
+
+For each changed file that looks architecturally significant (component files, index exports, stories, tests, figma-connect files, design token files), read its full content from the fetched PR ref:
+
+```bash
+git show pr-{pr}:<file_path>
+```
+
+### Mode B: Current branch (no `--pr`)
+
+```bash
+git rev-parse --abbrev-ref HEAD
+```
+
+Determine the merge-base. If `--base` was provided, use it directly; otherwise auto-detect:
+
+```bash
+MERGE_BASE=$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD origin/master)
+```
+
+Get the diff from the merge-base to the **working tree** (includes committed, staged, and unstaged changes):
+
+```bash
+git diff $MERGE_BASE --name-only
+git diff $MERGE_BASE
+```
+
+For each architecturally significant changed file, read the full file from disk for context beyond the diff.
+
+---
+
+## Step 3: Analyse
+
+Apply every rule from the loaded `.gitar/review/` files to the diff. For each finding note: file path, approximate line, and whether it's a **must fix** (correctness risk) or **worth addressing** (design smell that will compound).
+
+Before reporting each finding, assess the tradeoff:
+
+- **Scope**: one-liner fix, single-file refactor, or multi-file change?
+- **Complexity trade**: does the fix remove complexity in one place but add it somewhere else?
+- **Risk**: does it touch a public API (breaking change for consumers)?
+- **Strategic direction**: is the implementation heading the right way, or is it a local fix on a fundamentally wrong approach?
+
+Include the tradeoff in every finding. A finding without a tradeoff is just a complaint.
+
+---
+
+## Step 4: Report findings
+
+---
+
+**Must fix:**
+
+- `{file}:{line}` — {what breaks or violates and why} → fix: {concrete suggestion} | cost: {tradeoff}
+
+**Worth addressing (will compound if left):**
+
+- `{file}:{line}` — {the smell or gap} → suggestion: {cleaner approach} | cost: {tradeoff}
+
+**Looks good:**
+
+- {something specific that was done well}
+
+---
+
+Keep each finding to 2-3 sentences. If no must-fix items, say so clearly.
+
+### Closing prompt
+
+**If `--pr` was used** (reviewing an existing PR):
+
+Ask: "Want me to address any of these, or post these findings as a comment on the PR?"
+
+If the answer is to post: use `gh pr comment {pr} --body "{findings}"`. Do **not** offer to create a new PR — one already exists.
+
+**If reviewing the current branch** (no `--pr`):
+
+Ask: "Want to address any of these before opening the PR, or shall I go ahead and create it?"
+
+If the answer is to create it: `gh pr create --title "{title}" --body "{body}"`. Mention any deferred items briefly in the PR body so reviewers are aware.

.claude/skills

@@ -0,0 +1 @@
+../.agents/skills

.gitar/review/accessibility.md

@@ -0,0 +1,13 @@
+# Accessibility (a11y)
+
+- **WCAG 2.2 AA**: Verify that interactive components expose correct roles, labels, and states. Flag missing `aria-label`, `aria-labelledby`, or `role` where semantics require them.
+- **Aria attribute naming**:
+  - rendered DOM nodes must use spec ARIA attributes like aria-label
+  - public component props must use repo conventions like ariaLabel / ariaLabelledBy
+  - Flag any props that use the wrong casing or naming convention.
+- **Keyboard navigation**:
+  - All interactive elements must support standard keyboard patterns (Enter/Space to activate, Arrow keys for composite widgets, Escape to dismiss).
+  - Require explicit keyboard handlers for custom non-semantic widgets or custom composite behavior
+  - Do not require local handlers when native elements or Radix already provide the behavior
+- **`jest-axe` in tests**: Every new component test file must include a `jest-axe` assertion (`expect(await axe(container)).toHaveNoViolations()`). Flag test files missing this check.
+- **Storybook a11y**: Confirm that no a11y findings are suppressed in stories without a documented justification.

.gitar/review/component-patterns.md

@@ -0,0 +1,12 @@
+# Component implementation patterns
+
+- **No `forwardRef`**: React 19 makes `ref` a regular prop — `forwardRef` is deprecated and must not be used in new components. Accept `ref` directly in the props destructuring (e.g., `function MyComponent({ ref, ...props }: MyProps)`). Flag any newly introduced `forwardRef` wrapper. Existing components that still use `forwardRef` should be flagged as candidates for migration, but are not a hard blocker if the PR is not touching that component's signature.
+- **`displayName`**: Every component and every styled component must have `.displayName` set explicitly. Flag any missing `displayName`.
+- **Enums for variants**: Varieties, sizes, and other closed sets must be expressed as exported TypeScript enums (e.g., `ButtonVariety`, `BadgeSize`), not as plain string unions. The enum must be exported from the component file and re-exported from the folder's `index.ts`.
+- **CSS custom properties pattern**: Multi-variant styled components must use CSS custom properties (e.g., `var(--component-color)`) with a variant style map, not inline conditionals or multiple styled-component variants.
+- **Props interface**: Each component must export a `{ComponentName}Props` interface. Flag components that don't export their props type.
+- **Rest props forwarding**: Components must spread remaining props (`...otherProps` / `...restProps`) onto the root DOM node, after any internal props are destructured. Flag components that destructure all props without forwarding the rest, as this silently swallows `data-*`, `aria-*`, `className`, and other valid HTML attributes consumers may pass.
+- **`className` prop**: Every component's props interface must include an optional `className?: string` field. Flag any component that omits it, even if it otherwise spreads rest props — the explicit declaration is required so consumers know it is supported.
+- **`isDefined()` for truthiness checks**: Use the `isDefined()` helper from `~common/helpers/types` for all `!= null` / `!== undefined` / conditional-render guards. Flag raw `=== undefined`, `!= null`, or `Boolean(x)` checks where `isDefined(x)` should be used instead.
+- **Type predicate functions**: When a component needs to narrow a prop type (e.g., distinguishing `ButtonAsLinkProps` from `ButtonProps`), write a standalone type predicate function (`function isFoo(props): props is FooProps`) rather than inline `as` casts or `instanceof` checks.
+- **`@internal` for non-exported sub-components**: Internal sub-components and helpers that live in the component file but should not be used by consumers must be annotated with `/** @internal */`. Flag internal symbols that are exported without this annotation.

.gitar/review/design-tokens.md

@@ -0,0 +1,8 @@
+# Design tokens & styling
+
+- **`cssVar` only**: All CSS values — colors, spacing, typography, border radii, shadows — must come from design tokens via the `cssVar` helper. Flag any raw hex codes, pixel values, or hardcoded font sizes.
+- **No Tailwind / utility classes**: Echoes components must not use Tailwind or arbitrary CSS utility classes. Styling must go through Emotion styled components with design tokens.
+- **Semantic token selection**: Tokens must carry semantic meaning (e.g., `color-text-danger`, not `color-red-500`). Flag usage of primitive/non-semantic tokens where a semantic equivalent exists.
+- **New tokens**: Any new design token introduced in the diff should be flagged as requiring review by both a designer and a developer — it's not a blocker, but a checkpoint.
+- **No arbitrary colors or typography values**: Flag any hardcoded color value (hex, `rgb()`, `hsl()`), font family string, font size, font weight, or line height that is not routed through `cssVar()`. This applies inside `styled` components, inline `style` props, and CSS-in-JS objects alike.
+- **Layer 3 token opportunity**: When a component uses the same layer 1/2 token (e.g. `color-background-accent-default`) in multiple places to express a single component-level concept, flag it as a candidate for a new layer 3 component token (e.g. `--echoes-mycomponent-background`). Layer 3 tokens are the right tool when: (a) the same value is repeated many times within one component, (b) the value may need to be overridden independently from the global token it maps to, or (c) the concept has a clear semantic name at the component level. Note this as a suggestion requiring designer + developer alignment, not a hard blocker.

.gitar/review/folder-and-exports.md

@@ -0,0 +1,6 @@
+# Folder & export architecture
+
+- **Folder placement**: New components must live in a new `src/components/{kebab-case-name}/` folder or an existing matching feature/family folder. Flag components landing directly in `src/components/` without their own folder.
+- **Named exports only**: Default exports must be rejected. Every symbol — component, enum, props type — must use a named export.
+- **`index.ts` wiring**: The component's folder `index.ts` must export: the component itself, all enums, and the props type — all as named exports. The top-level `src/components/index.ts` must also be updated. Flag any component that is implemented but not re-exported at both levels, and flag any props interface that is exported from the component file but missing from the public barrel.
+- **Logic separation**: Complex components should be split into internal sub-components within the same folder. Flag monolithic components that exceed ~150 lines and handle multiple distinct concerns inline.

.gitar/review/general-design.md

@@ -0,0 +1,19 @@
+# General design issues
+
+## Duplication (DRY)
+
+- Same 3+ lines of logic appearing in 3 or more files? Flag it. Two-file duplication is often coincidental — don't extract unless the intent is clearly the same.
+- Two components or hooks structurally identical (same props, same state, same render logic)?
+- A constant defined in one place but hardcoded as a literal elsewhere?
+
+## Missing or better abstractions
+
+- A `switch` / `if` chain on the same discriminator repeated across multiple components? Consider a lookup map or config object.
+- Multiple styled components sharing identical CSS blocks that could be expressed as a shared base or CSS custom-property pattern.
+- Trace "what files need to change to add the next variant or size". More than two is a signal to redesign.
+
+## SOLID
+
+- **S**: A component mixing unrelated concerns (data, presentation, styling logic all in one)?
+- **O**: Adding a new variety or size — does it require modifying core component logic or just extending a map/enum?
+- **D**: Logic tightly coupled to a specific variant rather than driven by props or config objects?

.gitar/review/localization.md

@@ -0,0 +1,5 @@
+# Localization (l10n)
+
+- **English fallback**: Any user-visible string must have a default English value. Flag hardcoded non-English strings or strings with no default.
+- **`label`-prefixed overrideable props**: All user-visible labels must be overrideable via props prefixed with `label` (e.g., `labelClose`, `labelConfirm`). Flag components with hardcoded non-overrideable strings.
+- **No translation helpers**: Echoes is a library — it must not import or depend on app-level translation systems (`translate`). Flag any such imports.

.gitar/review/naming-and-api.md

@@ -0,0 +1,11 @@
+# Naming & API conventions
+
+- **Component files**: Component names and their `.tsx` files must use PascalCase. Flag any snake_case or kebab-case component file names.
+- **Folder names**: Must be kebab-case. Flag any PascalCase or camelCase folder names under `src/components/`.
+- **Boolean props**: Must be prefixed with `is`, `has`, `enable`, or `disable`, and must default to `false`. Flag booleans like `disabled={true}` as default, or props named `active` / `open` without the prefix.
+- **Event handler props**: Must start with `on` (e.g., `onClick`, `onValueChange`). Flag handlers named `handle*`, `click*`, or similar.
+- **Internal sub-components**: Sub-components within a folder must be prefixed with the parent component name (e.g., `SelectOption`, not just `Option`) to avoid collisions and keep them grouped in search results.
+- **Predictability**: Prop names should be obvious and consistent with existing Echoes components. Flag novel naming when a standard equivalent exists in the library (e.g., using `type` where similar components use `variety`).
+- **Props interface ordering**: Props must follow this order — required/semantic props first, then boolean flags (`is*`/`has*`/`enable*`/`disable*`), then callbacks (`on*`), then layout/style props (`className` last). Flag interfaces that mix callbacks before flags or put semantic props at the end.
+- **TSDoc on every public prop**: Every field in a public props interface must have a `/** */` doc comment. Optional props that have a default value must document it with `@defaultValue`. Enum members must also be individually documented. Flag any undocumented public prop or enum member.
+- **Union types for mutually exclusive props**: Props that are mutually exclusive (e.g., `ariaLabel` vs `ariaLabelledBy`, controlled vs uncontrolled open state) must be expressed as a TypeScript union type (`PropsA | PropsB`) rather than making both optional and relying on runtime checks. Flag components where two props are documented as "use one or the other" but the interface permits both simultaneously.

.gitar/review/radix-ui.md

@@ -0,0 +1,4 @@
+# Radix UI
+
+- **Namespace imports**: Radix primitives must always be imported as a namespace (`import * as RadixDialog from '@radix-ui/react-dialog'`), never as named imports. This makes Radix usage visually distinct from internal components throughout the file. Flag any `import { Root, Trigger } from '@radix-ui/react-*'` style imports.
+- **`style*` factory pattern for Radix composition**: When a Radix primitive needs styling, define a `styleXxx()` factory function that returns a styled component, then call it with the Radix component as the argument (e.g., `const ModalOverlay = styleModalOverlay(RadixDialog.Overlay)`). Do not wrap Radix components directly with `styled()` inline — this avoids double-wrapping and keeps style definitions composable. Flag direct `styled(RadixDialog.SomeComponent)` calls that bypass the factory pattern.

.gitar/review/stories-and-figma.md

@@ -0,0 +1,5 @@
+# Stories & Figma Connect
+
+- **Stories file**: Every new component must have a corresponding `stories/{category}/{ComponentName}-stories.tsx` file. Flag missing story files. When an existing component gains a new prop, variety, or behaviour, the matching story must be updated or a new story added to cover it — flag changes to component files with no corresponding change to the stories file.
+- **Controls coverage**: Stories must expose all meaningful props as Storybook controls. Flag stories that omit enum props from `argTypes`.
+- **Figma Connect file**: If a Figma design exists for the component, a `.figma.tsx` file should be present in `figma-connect/`. Flag missing Figma Connect files when the component clearly maps to a Figma component (this is a soft flag — confirm with the author).

.gitar/review/testing.md

@@ -0,0 +1,8 @@
+# Testing
+
+- **No snapshot tests**: Snapshot testing must be avoided. Flag any `toMatchSnapshot()` or `toMatchInlineSnapshot()` usage in component tests.
+- **Behavioral testing with RTL**: Tests must assert on user-visible behavior, not implementation details. Flag tests that query by component internals, CSS class names, or implementation-specific attributes.
+- **Keyboard navigation coverage**: For every interactive element introduced, verify that at least one test explicitly exercises keyboard interaction (e.g., `userEvent.keyboard`, `fireEvent.keyDown`).
+- **Coverage gate**: Low coverage on new code is a merge blocker. Flag new components that have no test file at all.
+- **Test file location**: Tests must live in `src/components/{component-folder}/__tests__/{ComponentName}-test.tsx`.
+- **Inline `setup*`/`render*` helpers**: Test files must define a local helper function (`setupFoo()` or `renderFoo()`) that wraps `render()` and returns `{ user, ...rest }`. Raw `render()` calls scattered across individual test cases are not permitted — they make tests harder to refactor and bypass the consistent `userEvent.setup()` wiring. Flag test files that call `render()` directly in `it()` blocks without a shared helper.

.gitignore

@@ -57,6 +57,12 @@ build-reports/
 dist/
 .env/
 
-# SonarQube LLM code context files
+# ---- LLM code context files
 .sonar-code-context/
 .serena/
+.claude/hooks/
+.mcp.json
+CLAUDE-personal.md
+CLAUDE.local.md
+AGENT-personal.md
+AGENT.local.md