--- name: ui-review 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).' --- > Codex compatibility note: > > - Invoke repository skills with `$skill-name` in Codex; this mirrored copy rewrites legacy Claude `/skill-name` references. > - Task tracker mandate: BEFORE executing any workflow or skill step, create/update task tracking for all steps and keep it synchronized as progress changes. > - User-question prompts mean to ask the user directly in Codex. > - Ignore Claude-specific mode-switch instructions when they appear. > - Strict execution contract: when a user explicitly invokes a skill, execute that skill protocol as written. > - Subagent authorization: when a skill is user-invoked or AI-detected and its protocol requires subagents, that skill activation authorizes use of the required `spawn_agent` subagent(s) for that task. > - Do not skip, reorder, or merge protocol steps unless the user explicitly approves the deviation first. > - For workflow skills, execute each listed child-skill step explicitly and report step-by-step evidence. > - If a required step/tool cannot run in this environment, stop and ask the user before adapting. ## Codex Project-Reference Loading (No Hooks) Codex uses static project-reference loading instead of runtime-injected project docs. When coding, planning, debugging, testing, or reviewing, open project docs explicitly using this routing. **Always read:** - `docs/project-config.json` (project-specific paths, commands, modules, and workflow/test settings) - `docs/project-reference/docs-index-reference.md` (routes to the full `docs/project-reference/*` catalog) - `docs/project-reference/lessons.md` (always-on guardrails and anti-patterns) **Missing/stale context route:** If `docs/project-config.json`, the docs index, `lessons.md`, `CLAUDE.md`, `AGENTS.md`, or any task-required reference doc is missing or stale, auto-run `$project-init` or the narrow setup route (`$project-config`, `$docs-init`, `$scan-all`, `$scan --target=`, `$claude-md-init`) before ordinary project-specific work. If Codex mirrors or `AGENTS.md` are missing/stale, ask the user to run `$sync-codex`; do not auto-run it. **Situation-based docs:** - Project structure/architecture/tech-stack/deployment/setup (any layer — backend, frontend, or infra): `project-structure-reference.md` - Backend/CQRS/API/domain/entity changes: `backend-patterns-reference.md`, `domain-entities-reference.md` - Frontend/UI/styling/design-system: `frontend-patterns-reference.md`, `scss-styling-guide.md`, `design-system/README.md` - Spec authoring, `docs/specs/` pathing, or TC format: `feature-spec-reference.md`, `spec-system-reference.md`, `spec-principles.md` - Behavior/public-contract changes or spec-test-code sync: `workflow-spec-test-code-cycle-reference.md` plus the spec docs above - Derived spec indexes/ERDs/reimplementation guides: `spec-system-reference.md` and source Feature Specs under `docs/specs/` - Integration test implementation/review: `integration-test-reference.md` - E2E test implementation/review: `e2e-test-reference.md` - Code review/audit work: `code-review-rules.md` plus domain docs above based on changed files Do not read all docs blindly. Start from `docs-index-reference.md`, then open only relevant files for the task. > **[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. ## 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 $ARGUMENTS ## 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 --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.-