---
name: ui-review
version: 2.0.0
description: '[Code Quality] Use when reviewing UI/frontend changes for long-content overflow, responsive multi-screen layout (flex-wrap / row-to-column on small devices), flex-vs-fixed sizing, z-index discipline, SCSS/BEM styling quality, and async UI states & feedback (loading indicator, error surface, empty state).'
execution-mode: subagent
context-budget: high
---

<!-- PROMPT-ENHANCE:STEP-TASK-ANCHOR:START -->

> **[BLOCKING]** Execute skill steps in declared order. NEVER skip, reorder, or merge steps without explicit user approval.
> **[BLOCKING]** Before each step or sub-skill call, update task tracking: set `in_progress` when step starts, set `completed` when step ends.
> **[BLOCKING]** Every completed/skipped step MUST include brief evidence or explicit skip reason.
> **[BLOCKING]** If Task tools are unavailable, create and maintain an equivalent step-by-step plan tracker with the same status transitions.

<!-- PROMPT-ENHANCE:STEP-TASK-ANCHOR:END -->

## Quick Summary

**Goal:** Validate UI/frontend changes for the six visual-quality dimensions that break in production but slip past correctness review — long-content overflow & truncation, responsive multi-screen layout (flex-wrap / row-to-column so it stays usable on small devices), flex-grow vs fixed sizing, z-index scale discipline, SCSS/CSS/BEM styling quality, and async UI states & feedback (loading indicator, user-visible error surface, empty state, in-flight disable) — so frontend/UI changes survive real content, responsive layouts, layering, styling conventions, and the loading/error/empty branches before handoff.

**Default scope:** All uncommitted frontend changes (staged + unstaged) matching the frontend path and file-extension patterns declared by the project configuration/docs index. Override: specify files, directories, components, or full frontend codebase.

> **CONDITIONAL — SKIP when no frontend files in scope.** In workflow context this skill is SKIPPED when the diff has no files matching the project frontend path/extension patterns. If invoked standalone with no frontend changes → announce `"No frontend changes detected — ui-review skipped"` and report clean.

> **ROUTING BOUNDARY (read before starting):**
>
> - **`ui-review` (this skill)** — the project UI review gate. Purpose: find issues, assign severity, give project-specific fix guidance citing real reuse targets (sourced from the project reference docs). It complements `architecture-review`. In `workflow-review-changes` it runs in TWO places by design (keep both): (a) INTERNALLY as `changes-review`'s UI dimension (step 1), AND (b) as a DEDICATED conditional parallel-batch member (step 9, `ui-ux-designer` sub-agent). Both fire only when frontend files are in scope.
> - **`web-design-guidelines`** — a generic, standalone accessibility / UX checklist. Cross-read it for a11y depth (WCAG, focus order, ARIA, contrast); do NOT duplicate its content here.
> - **`ui-ux-designer`** — specialized UI/UX, accessibility, responsive layout, and design-token review/authoring sub-agent when the local agent catalog provides it.

> **MANDATORY MUST ATTENTION** Plan tasks to READ UI rules BEFORE reviewing:
>
> 1. Project styling rules doc — BEM convention, mixins, variables, responsive patterns **(READ FIRST — primary styling rules source; resolve via project config/docs index)**
> 2. Project design-system/token doc — design tokens, especially the **Z-Index & Layering** section
> 3. Project frontend architecture/patterns doc — base component classes, state/store, API service, lifecycle teardown rules shared with `architecture-review`
> 4. Project code-review rules doc — anti-patterns and conventions
>
> Not found → search: "scss styling", "design tokens", "frontend patterns". Rules come from docs — NOT general knowledge.

**Workflow:**

1. **Phase 0: Load UI Rules** — Resolve and read the four project UI/styling rule docs above
2. **Phase 1: Determine Scope** — Changed frontend files (default) or user-specified scope
3. **Phase 2: Blast Radius** — Run graph trace if graph.db exists
4. **Phase 3: UI Category Review** — Check each file against all 6 applicable categories
5. **Phase 4: Finalize** — Generate compliance report with PASS/BLOCKED/WARN verdicts
6. **Fix Loop: Validate → Fix → Full UI Re-Review** — validate findings first; after validated fixes, rerun the full UI review using the local sub-agent selection guide only when that protocol calls for agents

**Key Rules:**

- Write findings to `plans/reports/ui-review-{date}-{slug}.md`
- BLOCKED = must fix before merge | WARN = review and decide | PASS = compliant
- Every violation needs `file:line` proof + grep 3+ counterexamples before flagging
- Review is read-only until `/why-review --validate-findings` confirms findings; fixes may happen only in the validated fix loop or downstream plan/feature-implement, and every fix restarts a full UI review from Phase 0 with brand-new tasks.

## Your Mission

<task>
$ARGUMENTS
</task>

## First Principle — Easy to Change

> **The success metric of every coding decision is _future change cost_.**
> DRY, SRP, abstraction, design patterns, naming, layering, tests — every
> technique exists to serve one goal: **making the next change cheaper**.

When evaluating styling, a layout, a token, or a component, ask:
**does this make the next change cheaper or more expensive?**

- Reject "best practices" that raise change cost (hardcoded values forcing
  per-file edits, hand-rolled overflow handling, raw breakpoints,
  copy-pasted truncation CSS).
- Name the real enemies in findings: **magic values, duplicated styling
  knowledge, fixed sizing that fights the viewport, cross-system token
  mixing, z-index escalation wars**.
- A simpler layout that survives 320px and a 200-char value beats a
  pixel-perfect fixed layout that breaks on either.

Apply this lens **before** invoking any specific rule, pattern, or checklist
below — if a downstream rule would raise change cost, this principle wins.

---

## Review Mindset (NON-NEGOTIABLE)

Skeptical. Every claim needs traced proof, confidence >80%.

- NEVER flag a styling violation without reading the actual SCSS/template + tracing the rendered element
- Every finding MUST include `file:line` evidence
- Before flagging a pattern violation: grep 3+ existing examples — codebase convention wins
- Question: "Is this actually a violation, or an established exception (icon dimensions, fixed brand assets, genuinely fixed UI)?"

## Phase 0: Load UI Rules (MANDATORY FIRST) (MUST ATTENTION)

> **MUST ATTENTION:** Read project UI docs BEFORE reviewing. Rules come from docs, not general knowledge.

- read the project styling rules doc — extract BEM convention, mixin names, variable names, responsive breakpoint mixins, nesting limits
- read the project design-system/token doc — extract design tokens and the **Z-Index & Layering** section
- read the project frontend architecture/patterns doc — extract base component classes, store/effect patterns, API service base, lifecycle teardown pattern
- read the project code-review rules doc — extract frontend anti-patterns and review rules directly

> **CROSS-SYSTEM WARNING (carry through every category):** Do NOT mix token systems with incompatible root-size, namespace, or layer assumptions in one file. When flagging a fix, recommend whichever token system the file already imports/uses; never introduce another system unless the project docs explicitly require migration.

## Phase 1: Determine Scope

**Default (no override):** Review all uncommitted frontend changes.

```bash
git status          # List changed files
git diff            # Staged + unstaged changes
git diff --cached   # Staged only
```

- Collect file list to review
- Filter to files matching the project frontend path/extension patterns resolved from project config/docs
- If ZERO frontend files match → announce `"No frontend changes detected — ui-review skipped"` and report clean (honor the CONDITIONAL skip)

## Phase 2: Blast Radius (if graph.db exists)

- If `.code-graph/graph.db` exists: run graph trace on key changed component files
- Record: impacted file count, shared-component fan-out (a changed shared-library component affects every consumer app), risk level
- Prioritize review by highest-impact files first (shared library components > app-local components)
- Graph unavailable: note "Graph not available — skipping blast radius" and proceed

For each changed component/style file with downstream impact:

```bash
python .claude/scripts/code_graph trace <changed-file> --direction both --json
```

Use `--node-mode file` first (10-30x less noise), then `--node-mode function` for detail. Flag shared-component consumers impacted by a styling or layout change.

## Phase 3: UI Category Review

Create report: `plans/reports/ui-review-{date}-{slug}.md`

For EACH file in scope, evaluate against ALL applicable categories. Skip categories not applicable to the file type (e.g., a pure `.ts` store file skips overflow/sizing/z-index but still hits Category 5's architecture checks and Category 6's loading/error/empty-state wiring).

> **Apply the `Think:` reasoning prompt before each category — derive violations, do NOT recite checklists.**

---

### Category 1: Long-Content Overflow & Truncation — Severity: WARN (HIGH when a flex child truncates with no `min-width: 0`)

**Think:** Does every text container survive a 200-char value? Single-line or multi-line? Can the user still read the full value when it is truncated?

**Detection signals:**

- Hand-rolled `text-overflow: ellipsis` (with `white-space: nowrap` / `overflow: hidden` re-declared by hand) instead of the project mixin/directive
- A flex child that truncates but has **NO `min-width: 0`** — the flex-overflow trap: a flex item's default `min-width: auto` refuses to shrink below content width, so ellipsis never triggers
- Truncated text with **NO tooltip / `title`** to reveal the full value

**DECISION RULE the reviewer enforces:**

- Single-line labels / table cells / chips → ellipsis **+ tooltip-on-overflow**
- Multi-line prose / descriptions → wrap or `-webkit-line-clamp`

**Project fix guidance** (cite real reuse targets from the resolved styling rules doc):

- Prefer the project's documented overflow/ellipsis directive, component, or utility. It must expose the full value only when the element actually overflows and must handle the flex `min-width` trap.
- OR the project's documented truncate/text-ellipsis mixins or utility classes from the styling rules doc.
- Multi-line: use the project-documented clamp pattern.

**GOOD vs BAD:** a utility/token-driven truncation that exposes the full value on overflow (tooltip/`title`) is correct; a hand-rolled substring / width-math truncation, or truncated text with no tooltip, is the anti-pattern. Cite the styling rules doc for the project's reuse targets.

---

### Category 2: Responsive Multi-Screen via Flex — Severity: WARN (BLOCKED only when content is broken on a small screen — clipped, cut off, or unreachable)

**Think:** Is this usable at 320 / 768 / 1024 / 1440? Does the layout flex, wrap, and **reflow from row to column** on small screens? If it genuinely cannot reflow, does it at least stay _usable_ via a fixed `min-width`/`min-height` + `overflow: auto` scroll — or does content get clipped, cut off, or pushed out of reach?

**Small-screen usability — the minimum bar (canonical):**

- **Preferred:** reflow — `flex-wrap: wrap`, a `flex-direction: row → column` switch at the breakpoint, or a responsive grid that collapses to a single column.
- **Acceptable fallback:** when a layout genuinely can't reflow (dense tables, canvases, diagrams, wide data grids), a fixed `min-width`/`min-height` with `overflow: auto` is fine — **scrolling is OK, not a defect.** Do NOT flag a component BLOCKED merely because it scrolls.
- **Hard minimum (BLOCKED if violated):** on a small screen the UI is _usable_ and nothing is **broken** — no clipped, cut-off, or unreachable content/controls (e.g. a submit button pushed off-screen with no scroll path, text overlapping, a fixed row that overflows AND hides content instead of scrolling).
- **Escalation:** if making a surface small-screen-usable would need a refactor too large for the current change, STOP and **confirm scope with the user** before proceeding — note it as a finding, do not silently rewrite the layout.

**Detection signals:**

- Raw `@media (max-width: NNNpx)` / `(min-width: NNNpx)` in component SCSS that bypasses the breakpoint mixins
- A multi-child flex `row` with **NO `flex-wrap`**, **NO `flex-direction: column`** override, AND **no `overflow: auto`** escape → content is trapped and clips on a phone (BLOCKED); the same row WITH a scroll escape is at most WARN (prefer reflow)
- Multi-column CSS grid with fixed column counts / fixed track widths and no single-column collapse and no scroll container at small breakpoints
- Non-flex fixed layouts (absolute positioning, fixed px widths on containers) that cannot reflow AND cannot scroll → content lost off-screen
- Content clipped, overlapping, or a control pushed out of reach below ~360px with no scroll path (BLOCKED). Horizontal scroll _by itself_, with all content still reachable, is the acceptable fallback — not a finding.

**Project fix guidance:**

- Preferred: the project documented responsive-flex mixins/utilities; add `flex-wrap: wrap` and/or the documented `row → column` breakpoint switch so the layout stacks vertically on small screens
- Fallback (when reflow isn't feasible): give the container a sensible `min-width`/`min-height` and `overflow: auto` so everything stays reachable by scroll
- Breakpoints from the project breakpoint tokens — NEVER inline pixel breakpoints
- Large layout refactor required → surface it as a finding and confirm with the user; don't bundle a big responsive rewrite into an unrelated change

**Anti-patterns:** raw `@media` pixel breakpoints that bypass the project's breakpoint mixins; a `flex-direction: row` with no wrap, no column fallback, AND no scroll escape so content clips; and non-flex fixed grids that neither reflow nor scroll. Cite the styling rules doc for documented offenders; grep the changeset for the same patterns.

---

### Category 3: Flex-Grow vs Fixed Width/Height (prefer min/max) — Severity: WARN

**Think:** Must this size be fixed, or can it grow/shrink with content + viewport?

**Detection signals:**

- Fixed `width:` / `height:` in px (≥ ~3 digits) on containers / cards / forms / dialogs

**Project fix guidance:**

- Prefer `flex: 1` / `flex-grow` + `max-width` / `min-width` caps, and `min-width: 0` on truncating flex children
- The project flex-container mixin/utility documented in the styling rules doc
- Reserve fixed px for icons / borders / genuinely fixed UI

**Anti-patterns:** large fixed `width:` / `height:` in px on containers / cards / forms / dialogs (e.g. `width: 964px`, `height: 772px`) that should flex with content + viewport. Cite the styling rules doc for documented offenders.

---

### Category 4: Z-Index Scale Discipline — Severity: BLOCKED (HARD GATE)

**Think:** Which layer does this surface belong to (base / raised / dropdown / sticky / modal / toast)?

**Detection signals:**

- Raw numeric `z-index` (literal value instead of a token)
- **ANY `z-index` with `!important` → BLOCKED** (an escalation war that the next dev will only beat with a bigger literal)

**Project fix guidance — use tokens:**

- The project documented z-index layer tokens
- Existing legacy/framework token systems only when the current file already uses that system
- The semantic layer variables declared canonical by the project design-system doc

Cross-reference the project design-system **Z-Index & Layering** map. The chosen token MUST match the surface's semantic layer.

**GOOD vs BAD:** a `z-index` set from a semantic token / layer scale is correct; a raw literal (e.g. `z-index: 99999`, `z-index: 10000`) or `z-index: N !important` is the anti-pattern. Cite the design-system and styling docs for the project's token scale and documented offenders.

---

### Category 5: SCSS/CSS Best Practices & BEM — Severity: WARN (BLOCKED on `!important`, chained BEM modifiers)

**Think:** Does this stylesheet read cleanly, follow BEM, and use tokens — or does it hardcode, over-nest, and chain modifiers?

> **SCSS/BEM rules (canonical):** BEM classes on ALL template elements (`block__element--modifier`). No magic numbers — use variables / design tokens. Max 3 nesting levels.

**Detection signals:**

- Nesting > 3 levels
- Hardcoded hex colors (should use CSS vars / design tokens)
- `px` where `rem` is expected
- Chained BEM modifiers (`.block__element.--modifier`) — `.--mod` MUST be a separate class, NEVER chained → BLOCKED
- Template elements missing BEM classes
- `!important` → BLOCKED

Apply fixes per the resolved project styling rules doc.

**Frontend architecture checks (OWNED JOINTLY WITH `architecture-review` Category 8 — reference, do not drift):**

> These checks are lifted from `architecture-review` Category 8 so the two skills stay synchronized. They are **owned jointly**; when one changes, update both. They apply to the `.ts` files in scope:

- Components MUST extend the project-documented base component/form/store component classes (BLOCKED)
- State MUST use the project-documented store/effect pattern — NEVER ad hoc local state when the project provides a canonical store pattern (BLOCKED)
- API services MUST extend the project-documented API service base — NEVER raw HTTP clients when the project provides a service abstraction (BLOCKED)
- All subscriptions MUST use the project's auto-teardown operator — NEVER manual unsubscribe (BLOCKED)
- All template elements MUST have BEM classes (WARN)
- Logic in lowest layer: Model > Service > Component (WARN)

---

### Category 6: Async UI States & Feedback — Loading / Error / Empty / Disabled — Severity: BLOCKED (HARD GATE when an async fetch or mutation renders NO loading indicator OR NO user-visible error surface)

> **This is the fundamental UI-resilience gate.** A screen whose happy path renders perfectly but shows a frozen blank while loading, fails silently on error, or shows nothing when empty is a broken user experience. Every async or interactive surface MUST answer three questions for the user: _what do I see while it's working, when it fails, and when there's nothing?_

**Think:** For EACH async operation the file performs (data fetch, form submit, mutation, navigation-triggered load, subscription), trace what the user actually sees in the in-flight, failure, and zero-result branches — not just the success branch. If any of those three branches renders a blank/frozen screen or nothing at all, that is the finding.

**The essential states every async/interactive surface MUST handle** (canonical vocabulary — shared verbatim with `design-spec` "Component States Checklist": Default / Loading / Disabled / Error / Empty / Success):

- **Loading** — every in-flight async request MUST render a loading indicator (spinner / skeleton / progress bar), NEVER a frozen or blank screen. Lists/tables → skeleton rows; buttons → in-button spinner + disabled.
- **Error** — every operation that can fail (network, validation, server error) MUST surface a human-readable error to the user WITH a recovery/retry path where sensible. NEVER a silent failure, a swallowed `catch`, or a raw stack trace / raw error object rendered to the user.
- **Empty** — every collection / list / table view MUST render a meaningful empty state (message + optional CTA) when it has zero items, NOT a blank area.
- **Disabled / in-flight guard** — submit/action controls MUST disable (or show busy) while their request is in-flight, to prevent double-submission.
- **Success** — a completed mutation SHOULD give confirmation feedback (toast / inline / visibly updated data).

**Detection signals:**

- An async call (fetch