Wes/ct 2457/mx UI ad rwith review skill

.claude/skills/adr-review/SKILL.md

@@ -0,0 +1,126 @@
+---
+name: adr-review
+description: Review a pull/merge request's new code against the connect-widget Architecture Decision Records (ADRs). Use when asked to "ADR review", "review this PR against the ADRs", "check ADR compliance", or before approving a PR/MR in the connect-widget (GitHub) or the sibling GitLab repo that follows the same standards. Reviews styling (prefer raw MXUI/MUI, theme variables, CSS Modules), testing (Vitest/MSW/Cypress), folder structure (screaming architecture), PR size, and undocumented architecture choices.
+---
+
+# ADR Review
+
+Review the **new code** in a pull request (GitHub) or merge request (GitLab) against
+the connect-widget Architecture Decision Records and report any violations.
+
+The connect-widget ADRs are the single **canonical** source of truth. Two repos are
+expected to follow them: `connect-widget` (GitHub) and a sibling GitLab repo. This
+skill can review either — it always evaluates against the connect-widget ADRs.
+
+## Scope: what to review
+
+Review **only the code the PR adds or changes** (the diff), not the whole repo. ADR
+0002 states that a PR is judged on whether its *new* code adheres to the ADRs; you
+are not auditing pre-existing code except where the PR modifies it. When a PR edits a
+line that was already non-conforming, note that conforming it would be ideal but is
+not blocking unless the PR is making that area worse.
+
+## Step 1 — Load the canonical ADRs (always do this first)
+
+The ADRs evolve, so read them live rather than relying on this skill's summary. Load
+them from the connect-widget repo, trying these sources in order until one works:
+
+1. If `architectureDecisionRecords/` exists in the current working directory (you are
+   in the connect-widget repo), read every `*.md` file in it.
+2. If the env var `CONNECT_WIDGET_ADR_PATH` is set, read the `*.md` files there.
+3. If a local connect-widget checkout is known, read its `architectureDecisionRecords/`.
+4. Fall back to fetching them from GitHub (works from any repo, e.g. the GitLab one):
+   ```bash
+   gh api repos/mxenabled/connect-widget/contents/architectureDecisionRecords \
+     --jq '.[] | select(.name|endswith(".md")) | .name' \
+   | while read -r f; do
+       echo "===== $f =====";
+       gh api "repos/mxenabled/connect-widget/contents/architectureDecisionRecords/$f" \
+         --jq '.content' | base64 --decode;
+     done
+   ```
+
+Read `reference/adr-checklist.md` (next to this file) for the distilled, checkable
+rules. The live ADR files win if they ever disagree with the checklist — if you spot
+drift, mention it.
+
+## Step 2 — Determine the target and get the diff
+
+Detect the platform from the git remote (`git remote -v`): `github.com` → GitHub,
+`gitlab` in the host → GitLab.
+
+Figure out what the user wants reviewed, in this priority:
+
+- **Explicit PR/MR number or URL** in the request → fetch that.
+  - GitHub: `gh pr diff <number>` and `gh pr view <number> --json title,body,files,baseRefName,additions,deletions`
+  - GitLab (if `glab` is installed): `glab mr diff <number>` and `glab mr view <number>`
+  - GitLab (no `glab`): tell the user glab isn't installed and fall back to the local
+    diff below, or ask them to check out the MR branch.
+- **A branch/PR is open for the current branch** → `gh pr view --json ...` + `gh pr diff`.
+- **Otherwise review the current branch** against its base:
+  ```bash
+  base=$(gh pr view --json baseRefName -q .baseRefName 2>/dev/null \
+         || git symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/dev/null | sed 's@^origin/@@' \
+         || echo master)
+  git fetch -q origin "$base" 2>/dev/null || true
+  git diff "origin/$base...HEAD" --stat
+  git diff "origin/$base...HEAD"
+  ```
+
+Also gather the list of changed files (`--name-status`) and the added-lines only
+(`git diff ... --unified=0`) so you can cite precise `file:line` locations.
+
+If you cannot obtain a diff, stop and tell the user what's missing (e.g. wrong repo,
+private MR needing glab auth) rather than reviewing nothing.
+
+## Step 3 — Review the diff against each ADR
+
+Go through the checklist in `reference/adr-checklist.md`. For each added/changed hunk,
+check every applicable ADR. Only flag things you can point to in the diff. Prefer
+being specific and actionable over exhaustive nitpicking.
+
+Assign each finding a severity:
+- **Blocking** — clearly violates an ADR's decision (would fail review per ADR 0002).
+- **Should fix** — likely violation or strongly discouraged pattern; confirm intent.
+- **Consider** — judgment call, style, or a heads-up (e.g. PR getting large).
+
+For anything ambiguous (folder-structure judgment calls, "is this new code or a hotfix"),
+say why it's ambiguous rather than asserting a violation.
+
+## Step 4 — Report the findings in chat
+
+Print a structured report. Do **not** post to the PR/MR unless the user later asks.
+
+Format:
+
+```
+# ADR Review — <PR title / branch> (<N files, +X/-Y>)
+
+**Verdict:** <Conforms ✅ | Changes needed ⚠️ | Blocking issues ❌>
+
+## Blocking
+- **[ADR 0001 Styling]** `src/Foo/Foo.tsx:42` — Uses `sx` prop for styling.
+  → Move to a CSS Module; `sx`/`xs` are only allowed for breakpoint-specific code.
+
+## Should fix
+- ...
+
+## Consider
+- ...
+
+## Notes
+- <ADR drift, hotfix exceptions, or "no test file added for new component", etc.>
+```
+
+If everything conforms, say so plainly and list what you checked so the user has
+confidence the review was real. If nothing in the diff is in scope for a given ADR
+(e.g. no styling changes), note that you checked and it didn't apply.
+
+## Notes & exceptions
+
+- **Hotfix exception (ADR 0002):** urgent production hotfixes may bypass the ADRs but
+  must be followed by a conforming PR. If the PR looks like a hotfix, flag violations
+  as "acceptable only if this is an urgent hotfix — file a follow-up ticket."
+- **Legacy code:** the repo is mid-migration (e.g. `@kyper/*` → MXUI). Editing legacy
+  files doesn't require rewriting them, but new code must conform.
+- Keep the review grounded in the *diff* — never invent violations you can't cite.

.claude/skills/adr-review/reference/adr-checklist.md

@@ -0,0 +1,119 @@
+# ADR Compliance Checklist
+
+Distilled, checkable rules from the connect-widget ADRs. The **live ADR markdown files
+are authoritative** — if this checklist disagrees with them, follow the ADRs and note
+the drift. Each rule lists what to look for in a diff and how confidently it can be
+flagged from static inspection.
+
+Cite the ADR number in every finding (e.g. `[ADR 0001 Styling]`).
+
+---
+
+## ADR 0001 — Styling our HTML
+
+Decision: prefer **raw MXUI/MUI components**; custom styling is rare and must be
+designer-validated; when custom styling is warranted, style with **CSS Modules** and
+use **MUI `Stack`** for spacing/layout.
+
+Check added/changed `.tsx`/`.jsx`/`.css` code:
+
+- **Prefer raw MXUI/MUI components.** Reach for the design system's components as-is
+  before writing any custom styling — they carry the design system's styling by
+  default. Flag new custom-styled elements (CSS Modules, wrappers, overrides) that
+  reimplement something an existing MXUI/MUI component already provides.
+- **Custom styling should be rare and designer-validated.** Custom styling should be
+  the exception, not the norm, and should be validated with a designer before being
+  implemented. If a design doesn't fit the design system, that deviation should be
+  confirmed as intentional with a designer. When a PR adds non-trivial custom styling,
+  flag it (usually "Consider"/"Should fix") with a note to confirm designer sign-off —
+  especially when it visibly deviates from the design system.
+- **Prefer MUI theme variables over hard-coded values.** When a raw component can't be
+  used, prefer MUI theme variables (e.g. `--mui-palette-*` CSS variables, visible in
+  Chrome dev tools' Styles panel on a rendered MXUI component) over hard-coded colors,
+  spacing, and other magic values. Flag hard-coded hex/rgb colors or pixel values that
+  a theme variable would cover.
+- **CSS Modules required.** New stylesheets must be `*.module.css` and imported as a
+  module (`import styles from './Foo.module.css'`). Flag new plain `.css`/global CSS
+  files, or Tailwind / other global CSS-framework classes, or styled-components.
+- **No `sx` prop for styling.** Flag `sx={...}` on MUI/MXUI components. *Exception:*
+  `xs` is allowed **only** for breakpoint-specific code (MUI doesn't expose breakpoints
+  as CSS variables). Ordinary styling via `sx`/`xs` → move to a CSS Module.
+- **Spacing between elements → MUI `<Stack spacing={n}>`.** Flag margins/padding added
+  purely to space sibling elements when a `Stack` would be idiomatic. (Judgment call —
+  mark as "Consider" unless obvious.)
+- **On a `Stack`, don't use `gap` or `flexDirection` props.** Use `spacing` and
+  `direction` instead. Flag `<Stack ... gap=` and `<Stack ... flexDirection=`. Other
+  flexbox props directly on `Stack` are fine.
+
+Related lint (not an ADR, but reinforces intent): `.eslintrc.cjs` restricts some
+`@kyper/*` and `@mui/material/TextField` imports (use `src/privacy/input`). New
+`@kyper/*` usage is discouraged (migrate to MXUI) per project docs.
+
+---
+
+## ADR 0002 — Document architecture decisions
+
+Decision: new code must adhere to the ADRs; significant technical choices need an ADR.
+
+- **PR-blocking rule:** new code that violates any ADR should not be approved (unless
+  it's an urgent hotfix, which must be followed by a conforming PR).
+- **New library / major pattern without an ADR.** If the diff adds a dependency to
+  `package.json` or introduces a notably new architectural pattern (new state lib, new
+  styling approach, new test framework, etc.), check whether a supporting ADR exists.
+  If not, flag: "introduces <X>; ADR 0002 expects significant choices to be documented."
+- **Non-conforming code being modified:** ADR 0002 says to conform it if feasible, or
+  at minimum file a ticket and add tests covering the new code. Note this when a PR
+  touches non-conforming areas without doing either.
+
+---
+
+## ADR 0003 — Automated testing (frontend)
+
+Decision: **Vitest** (unit/integration), **MSW** (API mocking), **Cypress** (e2e).
+Prefer integration tests; mock as little as possible; render real components.
+
+- **New code should have tests.** Flag new components/hooks/util modules added without
+  a corresponding `*.test.ts(x)` (or `*.cy.ts` for e2e) in the same PR. Colocated test
+  next to source is expected (see ADR 0004).
+- **Prefer integration over heavy mocking.** Flag heavy use of `vi.mock(...)` to stub
+  out real components/modules — the ADR prefers rendering real components so context
+  and side-effects are wired. Mark as "Should fix" / "Consider" with a note.
+- **Use MSW for API mocking.** Flag tests that mock `fetch`/`axios` directly (e.g.
+  `vi.fn()` on the network, `global.fetch = ...`) instead of MSW handlers.
+- **Right tool for the layer:** many edge cases belong in Vitest integration tests
+  (with MSW), not Cypress. e2e is for verifying frontend↔backend/API wiring.
+
+---
+
+## ADR 0004 — Folder structure (screaming architecture)
+
+Decision: organize by **domain**, not by framework/technical type. Keep files that are
+used together in close proximity; move code to `shared/` only once actually shared.
+
+- **New files organized by domain.** A new feature's component, `api.ts`, and tests
+  should live together in a domain folder (e.g. `src/Institutions/Institution/...`),
+  not scattered across generic technical folders. Flag new files dropped into generic
+  buckets (`components/`, `hooks/`, `utils/`, `redux/`) purely by file type when a
+  domain folder would be clearer. (Judgment call — the existing repo predates this ADR,
+  so weigh against surrounding structure; mark most as "Consider"/"Should fix".)
+- **Colocation.** Tests and `api.ts` live next to the code they cover, not in a
+  separate mirror tree. Flag new tests placed far from their subject.
+- **`shared/` is for genuinely shared code.** Flag brand-new code placed directly in
+  `shared/` that only one domain uses (premature sharing).
+
+Because the current repo is mid-migration, treat structure findings as guidance for
+*new* domains/files rather than demanding relocation of existing ones.
+
+---
+
+## ADR 0005 — Small pull requests
+
+Decision: strive for small, focused PRs that serve a single purpose.
+
+- **Size heuristic.** Consider flagging when the diff is large or unfocused, e.g.
+  roughly >~400 changed lines or >~15–20 files of production code, OR the PR clearly
+  bundles unrelated concerns (e.g. a refactor + a feature + a dependency bump).
+- Mechanical/generated changes (lockfiles, snapshots, i18n) don't count against size
+  the same way — note them separately.
+- This is almost always a **"Consider"** (advisory), not blocking. Frame it as "could
+  this be split?" and point to the distinct concerns you see.

architectureDecisionRecords/0001Styling.md

@@ -18,7 +18,13 @@ We also need to work with both CSS modules and MXUI. MXUI uses [MUI](https://mui
 
 ## Decision
 
-We will use CSS modules to style our html.
+We should use the raw MXUI and MUI components as much as possible. These components come with the design system's styling baked in, so leaning on them keeps us consistent by default.
+
+Custom styling should be rare. Before implementing custom styling, validate it with a designer. If a design doesn't fit within the confines of the design system, confirm with the designer that the deviation from the design system was intentional.
+
+If we can't use the raw MUI component, then we should use a MUI theme variable when possible. These theme variables can be viewed by looking in Chrome dev tools under the Styles panel where an MXUI component is being rendered.
+
+When custom styling is warranted, we will use CSS modules to style our html.
 
 When we need to add spacing between two elements, we should use a MUI [Stack](https://mui.com/material-ui/react-stack/) with a `spacing` prop.