From f1922c216989318e539563ad12d410d97ad7b80c Mon Sep 17 00:00:00 2001 From: Manish Dixit Date: Mon, 23 Mar 2026 23:59:41 +0100 Subject: [PATCH 1/2] feat(lfx-preflight): add 15 code review guard checks for Angular repos MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fold review-guard checks into /lfx-preflight per reviewer feedback on PR #16. Single workflow instead of a separate skill — contributors run one command. Adds Check 6 (Code Review Guard) with 15 sub-checks split into: - Auto-fix (4): raw HTML wrappers, dead imports, type safety, signal patterns - Advisory (11): component size, loading states, error handling, API alignment, PR description, accessibility, design tokens, N+1 patterns, template completeness, stale data, visitor gating All checks are Angular-only, gated behind repo-type detection. Skipped for Go repos. Bug fixes from PR #16 review: - Removed incorrect allowSignalWrites rule for form patchValue/setValue - Fixed naming: uses /lfx-preflight and /lfx-coordinator consistently LFXV2-1312 Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Manish Dixit --- README.md | 11 ++- lfx-preflight/SKILL.md | 216 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 216 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 86ecf54..13f2c00 100644 --- a/README.md +++ b/README.md @@ -130,7 +130,7 @@ The skills form a layered system where each skill has a clear responsibility and | `/lfx-backend-builder` | Generates Express.js proxy endpoints, Go microservice code, shared types. Encodes three-file pattern, logging, Goa DSL, NATS messaging | Code gen | Bash, Read, **Write, Edit**, Glob, Grep, AskUserQuestion | | `/lfx-ui-builder` | Generates Angular 20 components, services, drawers, pagination UI, styling. Encodes signal patterns, PrimeNG wrappers | Code gen | Bash, Read, **Write, Edit**, Glob, Grep, AskUserQuestion | | `/lfx-product-architect` | Answers "where should this go?", traces data flows, makes placement decisions, explains design patterns | Read-only | Bash, Read, Glob, Grep, AskUserQuestion | -| `/lfx-preflight` | Pre-PR validation — auto-fixes formatting & license headers, runs lint, build, checks protected files, offers PR creation | Validate + fix | Bash, Read, **Write, Edit**, Glob, Grep, AskUserQuestion | +| `/lfx-preflight` | Pre-PR validation — auto-fixes formatting, license headers, and common reviewer blockers (15 checks for Angular), runs lint, build, checks protected files, offers PR creation | Validate + fix | Bash, Read, **Write, Edit**, Glob, Grep, AskUserQuestion | | `/lfx-pr-catchup` | Morning PR dashboard — unresolved comments, status changes, stale PRs, approved-but-not-merged across all your open PRs | Read-only | Bash, Read, Glob, Grep, AskUserQuestion | | `/lfx-setup` | Environment setup — prerequisites, clone, install, env vars, dev server. Adapts to Angular or Go repos | Interactive guide | Bash, Read, Glob, Grep, AskUserQuestion | | `/lfx-test-journey` | Combine branches from multiple repos into worktrees for journey testing | Interactive | Bash, Read, Write, Edit, Glob, Grep, AskUserQuestion | @@ -290,9 +290,10 @@ Runs a comprehensive **pre-PR validation** with auto-fix capabilities. Adapts al 4. **Linting** — `yarn lint` (Angular) or `go vet ./...` (Go), auto-fixes import order/unused imports 5. **Build verification** — `yarn build` (Angular) or `go build ./...` (Go), fixes simple issues 6. **Tests** — runs if test files exist for modified code (doesn't block on failures) -7. **Protected files check** — flags changes to infrastructure files (`server.ts`, middleware, `angular.json`, `gen/`, `charts/`, etc.) -8. **Commit verification** — conventions, signoff, JIRA ticket -9. **Change summary** — categorized list of all new and modified files +7. **Code review guard** (Angular only) — 15 sub-checks for common reviewer blockers, split into auto-fix (raw HTML wrappers, dead imports, type safety, signal patterns) and advisory (loading states, error handling, accessibility, design tokens, API alignment, N+1 patterns, template completeness, stale data, visitor gating, component size, PR description) +8. **Protected files check** — flags changes to infrastructure files (`server.ts`, middleware, `angular.json`, `gen/`, `charts/`, etc.) +9. **Commit verification** — conventions, signoff, JIRA ticket +10. **Change summary** — categorized list of all new and modified files **Modes:** Auto-fix (default) or report-only ("dry run"). Offers to commit auto-fixes and create PR when all checks pass. @@ -418,7 +419,7 @@ An **interactive setup guide** that walks through environment configuration step ### Validate before submitting a PR ``` -/lfx-preflight → license headers → format → lint → build → protected files → PR +/lfx-preflight → license headers → format → lint → build → review guard (Angular) → protected files → PR ``` ### Set up a new developer environment diff --git a/lfx-preflight/SKILL.md b/lfx-preflight/SKILL.md index 70fed93..662a9d3 100644 --- a/lfx-preflight/SKILL.md +++ b/lfx-preflight/SKILL.md @@ -2,8 +2,8 @@ name: lfx-preflight description: > Pre-PR validation for any LFX repo — license headers, format, lint, build, - and protected file check. Adapts to repo type (Angular or Go). Use before - submitting any PR. + protected file check, and 15 code review guard checks for Angular repos. + Adapts to repo type (Angular or Go). Use before submitting any PR. allowed-tools: Bash, Read, Write, Edit, Glob, Grep, AskUserQuestion --- @@ -140,7 +140,209 @@ go test ./... Report test results but don't block on test failures unless the user asks. -## Check 6: Protected Files Check +## Check 6: Code Review Guard (Angular repos only) + +**Skip this entire check for Go repos.** + +Scope all sub-checks to **changed files only** (`git diff --name-only origin/main...HEAD`). These checks catch the patterns most commonly flagged by reviewers across 20+ LFX PRs. They are split into auto-fix and advisory categories. + +### Auto-Fix Sub-Checks + +These can be fixed automatically in auto-fix mode, similar to formatting and license headers. + +#### 6a. Raw HTML Form Elements (MOST COMMON blocker) + +Search **changed `.html` files** for raw form elements that must use LFX wrappers: + +| Raw Element | Required Wrapper | +| --- | --- | +| `` from PrimeNG | + +**Exceptions:** Elements inside comments, or `` are acceptable. + +**Note:** LFX wrappers require `FormGroup` + `FormControl` — `ngModel` is not supported. + +**Auto-fix:** Replace raw elements with LFX wrapper equivalents, preserving attributes. + +**Severity:** BLOCKER + +#### 6b. Dead Imports and Unused Providers + +Search **all changed `.ts` files** for: + +- **Unused imports** — imported symbols not referenced in the file body +- **Unused providers** — `providers: [...]` entries in component metadata where the service is never injected via `inject()` or constructor +- **Unbound component outputs** — when a template uses a child component (e.g., ``), check if that component emits outputs (e.g., `viewVote`, `rowClick`, `refresh`) that the parent template doesn't bind. Missing output bindings mean user interactions silently do nothing. + +**Auto-fix:** Remove unused imports and providers. Flag unbound outputs for manual review. + +**Severity:** BLOCKER + +#### 6c. Type Safety + +Search **changed `.html` templates** and **`.ts` files** for: + +- **Non-null assertions (`!`)** — patterns like `data()!.field` or `item!.property` in templates. These cause runtime crashes when the value is null/undefined. Use `?.` and `@if (data(); as d)` guards instead. +- **Falsy `||` vs nullish `??`** — using `||` where `??` is needed. `value || null` treats `0`, `""`, and `false` as falsy — hiding valid zero counts (e.g., `total_members || null` hides `0` members). Use `??` to only coalesce on `null`/`undefined`. + +**Auto-fix:** Replace `!` with `?.` or `@if` guards where safe. Replace `||` with `??` for null-coalescing contexts. Flag ambiguous cases for manual review. + +**Severity:** BLOCKER for `!` assertions. DISCUSS for `||` vs `??`. + +#### 6d. Signal Pattern Compliance + +Search **changed `*.component.ts` and `*.service.ts` files** for: + +- **`BehaviorSubject` for simple state** — should use `signal()` instead. `BehaviorSubject` is only appropriate for complex async streams. +- **`cdr.detectChanges()` or `ChangeDetectorRef`** — not needed in zoneless Angular 20. The framework handles change detection. +- **`model()` for internal state** — `model()` creates a two-way bindable input/output on the component's public API. For internal-only state (e.g., dialog visibility, drawer toggles not exposed to parents), use `signal()` instead. Only use `model()` when the parent component needs two-way binding (e.g., `[(visible)]="childVisible"`). +- **Signals not initialized inline** — per `component-organization.md`, simple `WritableSignal`s must be initialized directly (e.g., `loading = signal(false)`), not in the constructor. + +**Auto-fix:** Replace `BehaviorSubject` with `signal()` for simple state. Remove `ChangeDetectorRef` injections and `detectChanges()` calls. Replace `model()` with `signal()` for internal-only state. Flag complex cases for manual review. + +**Severity:** BLOCKER for `BehaviorSubject` misuse. DISCUSS for `ChangeDetectorRef` (may be legacy code) and `model()` misuse. + +### Advisory Sub-Checks (report only) + +These require human judgment and are reported as discussion items, not auto-fixed. + +#### 6e. Component Responsibility + +Search **changed `*.component.ts` files** for service injection count (count `inject()` calls and constructor injections): + +- **4+ service injections** → Flag for discussion. This often means the component is doing too much. +- **Multiple independent edit workflows** in a single component (e.g., separate forms that don't share state) → Suggest extracting sub-components. + +**Severity:** DISCUSS — guideline, not a hard rule. + +#### 6f. Loading States + +Search **changed `.html` templates** and **`.ts` files** for: + +- **Stats or counts rendered without loading check** — interpolations like `{{ count() }}` or `{{ stats().total }}` without a surrounding `@if (loading())` guard. These show `0` during loading instead of a placeholder. +- **Missing loading branch** — components that fetch data but have no `@if (loading())` / `@else` pattern. +- **Content that jumps** — `@for` loops rendering data without a loading skeleton before data arrives. +- **Loading not reset on re-fetch** — `loading` signal set to `false` after a fetch completes, but never set back to `true` when a new fetch starts (e.g., inside `switchMap` when input changes). Fix: set `loading.set(true)` at the start of each `switchMap` callback. + +Every data display that starts empty and populates asynchronously needs an explicit loading branch showing `—`, ``, or equivalent. + +**Severity:** BLOCKER for showing `0` during load. DISCUSS for missing re-fetch reset. + +#### 6g. Error Handling + +Search **changed `.ts` files** for: + +- **Silent `catchError`** — `catchError(() => of([]))` or `catchError(() => EMPTY)` without any logging before the fallback. Every `catchError` should log via `logger` service or `console.error` at minimum. +- **Duplicate/layered error handling** — when a service method already has `catchError` that returns a default (e.g., `of([])`), a component-level `catchError` on the same stream is unreachable dead code. Handle errors in one place — either the service or the component, not both. +- **Inconsistent fallback values** — mixing `EMPTY` and `of([])` in the same service. Pick one pattern. +- **Removed error logging** — check `git diff` for removed `console.error` or `logger.error` calls that weren't replaced. + +**Severity:** BLOCKER for silent or unreachable `catchError`. DISCUSS for inconsistent fallbacks. + +#### 6h. Upstream API Alignment + +Search **changed `.ts` files** for API calls and verify: + +- **Parameter names match upstream** — known divergences: + - Meetings API uses `limit` for pagination + - Votes/Surveys APIs use `page_size` for pagination + - Don't mix these up +- **No invented fields** — if the code references a field in an API response, verify it exists in the upstream contract. +- **No UI for non-existent backend fields** — form fields or display elements bound to data that the API doesn't actually return. + +If you cannot verify the upstream contract from the local codebase, flag for manual verification. + +**Severity:** BLOCKER for clearly wrong parameter names. DISCUSS for fields needing upstream verification. + +#### 6i. PR Description Completeness + +Check the **git log and diff** for changes that need explicit documentation in the PR description: + +- **Removed UI elements** — deleted components, removed buttons/fields/sections from templates. +- **Permission check changes** — modifications to FGA checks, role guards, or auth logic. +- **Error handling behavior changes** — changed fallback values, modified retry logic, altered error messages. + +**Severity:** DISCUSS + +#### 6j. Accessibility + +Search **changed `.html` templates** for: + +- **Missing `aria-pressed` on toggle buttons** — button groups acting as toggles must have `[attr.aria-pressed]="isActive()"`. +- **Nested interactive elements** — a clickable `
` containing an `` or ``. +- **Focusable elements behind overlay/blur masks** — use `[attr.tabindex]="-1"`, `inert`, or conditionally render elements. +- **Missing `aria-label` on icon-only buttons**. + +**Severity:** DISCUSS + +#### 6k. Design Token Compliance + +Search **changed `.html` templates** for hardcoded Tailwind color classes that should use LFX design tokens: + +- **Hardcoded colors** — `bg-blue-50`, `text-gray-300`, `border-blue-100`, etc. Check `tailwind.config.js` for the custom LFX color palette. Raw Tailwind defaults are not design tokens. + +**Severity:** DISCUSS + +#### 6l. N+1 API Patterns + +Search **changed `.ts` files** for per-item API calls inside loops: + +- **Per-item fetches** — `.map(item => this.http.get('/api/' + item.id))` or `forkJoin(items.map(...))` where a batch endpoint exists. +- **Backend too:** In Express controllers, `await` inside `for`/`forEach`/`.map()` loops calling `microserviceProxy.proxyRequest()`. + +**Severity:** DISCUSS + +#### 6m. Template/Config Completeness + +Search **changed `.html` templates and `.component.ts` files** for: + +- **Missing `@switch` cases** — if a component defines tabs/routes/modes in a config array, every entry must have a corresponding `@case` in the template. A tab in config without a matching case renders blank content. +- **`activeTab` not constrained to visible set** — if tabs are conditionally visible, ensure `activeTab` resets to a valid tab when the visible set changes. +- **Partial feature wiring** — form controls, outputs, or config entries added but not fully connected. + +**Severity:** BLOCKER for missing switch cases. DISCUSS for partial wiring. + +#### 6n. Stale Data During Navigation + +Search **changed `*.component.ts` files** for: + +- **One-time initialization that should react to changes** — `if (!this.data())` guards that only load data on first render, not when route params change. +- **Early returns that skip state reset** — guard clauses that exit before resetting `loading` or `saving` signals, leaving the UI stuck. +- **`track $index` in `@for` loops** — causes unnecessary DOM churn when items reorder. Prefer `track item.uid` or a stable identifier. + +**Severity:** DISCUSS + +#### 6o. Visitor/Permission Gating + +Search **changed `.html` templates** for: + +- **Content visible during role loading** — `@if (!isVisitor())` evaluates to `true` while `myRoleLoading()` is still `true` (because `isVisitor()` defaults to `false`). Fix: add `!myRoleLoading()` to the guard. +- **Visitor blur bypass** — blur overlays that don't prevent keyboard/screen-reader access (see 6j). +- **Permission changes not documented** — if the diff adds/removes/changes `canEdit()`, `isVisitor()`, `hasPMOAccess()` checks, flag for PR description. + +**Severity:** BLOCKER for content flashing during role loading. DISCUSS for blur bypass. + +### Check 6 Results + +Report Check 6 results grouped by severity: + +```text +Review guard (Angular): + Auto-fixed: + - Replaced 2 raw elements with wrappers + - Removed 3 unused imports + Blockers: + ✗ 6f. committee-votes.component.ts — loading not reset in switchMap + ✗ 6m. committee-view.component.html — missing @case ('settings') in @switch + Discussion: + ⚠ 6e. overview.component.ts — 5 service injections (consider splitting) + ⚠ 6k. committee-surveys.component.html — hardcoded bg-blue-50 (use design token) +``` + +## Check 7: Protected Files Check ```bash git diff --name-only origin/main...HEAD @@ -178,7 +380,7 @@ git diff --name-only origin/main...HEAD - `go.mod` / `go.sum` (dependency changes need review) - `Makefile` (build system changes) -## Check 7: Commit Verification +## Check 8: Commit Verification ```bash git status @@ -190,7 +392,7 @@ git log --format="%h %s%n%b" origin/main...HEAD - **`--signoff` on all commits?** - **JIRA ticket referenced?** -## Check 8: Change Summary +## Check 9: Change Summary ```bash git diff --stat origin/main...HEAD @@ -225,6 +427,7 @@ Detailed checks: ✓ Linting — No errors ✓ Build — Succeeded ✓ Tests — N/A (no test files for changed code) +✓ Review guard — 2 auto-fixed, 0 blockers, 1 discussion item (see below) ✓ Protected files — None modified ✓ Commits — Conventions followed, signed off ═══════════════════════════════════════════ @@ -271,7 +474,8 @@ Report failures in plain language, explaining what each means: **This skill DOES:** - Run format, lint, build checks -- Auto-fix formatting and license headers +- Auto-fix formatting, license headers, raw HTML wrappers, dead imports, type safety, and signal patterns +- Run 15 code review guard checks for Angular repos (common reviewer blocker patterns) - Report protected file changes - Verify commit conventions - Offer to create PR after passing From 83dbf7b1a8843e4d5d10fa5a2cd0b1d3782614c7 Mon Sep 17 00:00:00 2001 From: Manish Dixit Date: Tue, 24 Mar 2026 15:05:59 +0100 Subject: [PATCH 2/2] refactor(lfx-preflight): split into two phases, make all review checks report-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per Nirav's feedback on PR #17: Phase 1: Validation (auto-fix) — license, format, lint, build, protected files, commits. Runs for all repo types. Phase 2: Code Review (report-only, Angular only) — 15 pattern checks. None auto-fix. Runs last so it's the final thing before submitting. Changes: - All 15 review checks are now report-only (no auto-fix) - Added --skip-review flag to skip Phase 2 during dev - Fixed scoping: unbound outputs search .html not .ts - Fixed scoping: ! assertions report-only in .ts, blocker in templates - Fixed scoping: ChangeDetectorRef is advisory, not auto-removed - New two-phase output format per Nirav's spec LFXV2-1312 Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Manish Dixit --- README.md | 21 ++- lfx-preflight/SKILL.md | 362 ++++++++++++++++++++++------------------- 2 files changed, 208 insertions(+), 175 deletions(-) diff --git a/README.md b/README.md index 13f2c00..d1c284c 100644 --- a/README.md +++ b/README.md @@ -130,7 +130,7 @@ The skills form a layered system where each skill has a clear responsibility and | `/lfx-backend-builder` | Generates Express.js proxy endpoints, Go microservice code, shared types. Encodes three-file pattern, logging, Goa DSL, NATS messaging | Code gen | Bash, Read, **Write, Edit**, Glob, Grep, AskUserQuestion | | `/lfx-ui-builder` | Generates Angular 20 components, services, drawers, pagination UI, styling. Encodes signal patterns, PrimeNG wrappers | Code gen | Bash, Read, **Write, Edit**, Glob, Grep, AskUserQuestion | | `/lfx-product-architect` | Answers "where should this go?", traces data flows, makes placement decisions, explains design patterns | Read-only | Bash, Read, Glob, Grep, AskUserQuestion | -| `/lfx-preflight` | Pre-PR validation — auto-fixes formatting, license headers, and common reviewer blockers (15 checks for Angular), runs lint, build, checks protected files, offers PR creation | Validate + fix | Bash, Read, **Write, Edit**, Glob, Grep, AskUserQuestion | +| `/lfx-preflight` | Pre-PR validation — Phase 1 auto-fixes (format, license, lint, build), Phase 2 code review (15 report-only checks for Angular). Pass `--skip-review` to skip Phase 2 | Validate + review | Bash, Read, **Write, Edit**, Glob, Grep, AskUserQuestion | | `/lfx-pr-catchup` | Morning PR dashboard — unresolved comments, status changes, stale PRs, approved-but-not-merged across all your open PRs | Read-only | Bash, Read, Glob, Grep, AskUserQuestion | | `/lfx-setup` | Environment setup — prerequisites, clone, install, env vars, dev server. Adapts to Angular or Go repos | Interactive guide | Bash, Read, Glob, Grep, AskUserQuestion | | `/lfx-test-journey` | Combine branches from multiple repos into worktrees for journey testing | Interactive | Bash, Read, Write, Edit, Glob, Grep, AskUserQuestion | @@ -281,21 +281,23 @@ A **read-only** advisory skill that answers architectural questions without gene ### `/lfx-preflight` -Runs a comprehensive **pre-PR validation** with auto-fix capabilities. Adapts all checks to the repo type. +Runs a two-phase **pre-PR validation**. Adapts all checks to the repo type. -**Checks (in order):** +**Phase 1: Validation (auto-fix):** 1. **Working tree status** — uncommitted changes, commits ahead of main, JIRA references, `--signoff` 2. **License headers** — verifies and auto-fixes missing headers on `.ts`, `.html`, `.scss`, `.go` files 3. **Formatting** — `yarn format` (Angular) or `gofmt -w .` (Go), reports which files changed 4. **Linting** — `yarn lint` (Angular) or `go vet ./...` (Go), auto-fixes import order/unused imports 5. **Build verification** — `yarn build` (Angular) or `go build ./...` (Go), fixes simple issues 6. **Tests** — runs if test files exist for modified code (doesn't block on failures) -7. **Code review guard** (Angular only) — 15 sub-checks for common reviewer blockers, split into auto-fix (raw HTML wrappers, dead imports, type safety, signal patterns) and advisory (loading states, error handling, accessibility, design tokens, API alignment, N+1 patterns, template completeness, stale data, visitor gating, component size, PR description) -8. **Protected files check** — flags changes to infrastructure files (`server.ts`, middleware, `angular.json`, `gen/`, `charts/`, etc.) -9. **Commit verification** — conventions, signoff, JIRA ticket -10. **Change summary** — categorized list of all new and modified files +7. **Protected files check** — flags changes to infrastructure files (`server.ts`, middleware, `angular.json`, `gen/`, `charts/`, etc.) +8. **Commit verification** — conventions, signoff, JIRA ticket +9. **Change summary** — categorized list of all new and modified files -**Modes:** Auto-fix (default) or report-only ("dry run"). Offers to commit auto-fixes and create PR when all checks pass. +**Phase 2: Code Review (Angular only, report-only):** +15 pattern checks for common reviewer blockers — raw HTML wrappers, dead code, component size, loading states, type safety, error handling, signal patterns, API alignment, PR description, accessibility, design tokens, N+1 patterns, template completeness, stale data, visitor gating. All report-only; none auto-fix. + +**Flags:** `--skip-review` skips Phase 2. "report only" / "dry run" makes Phase 1 report-only too. --- @@ -419,7 +421,8 @@ An **interactive setup guide** that walks through environment configuration step ### Validate before submitting a PR ``` -/lfx-preflight → license headers → format → lint → build → review guard (Angular) → protected files → PR +/lfx-preflight → Phase 1 (license, format, lint, build, protected files) → Phase 2 (15 code review checks, Angular only) → PR +/lfx-preflight --skip-review → Phase 1 only (useful during dev) ``` ### Set up a new developer environment diff --git a/lfx-preflight/SKILL.md b/lfx-preflight/SKILL.md index 662a9d3..f5d1fbb 100644 --- a/lfx-preflight/SKILL.md +++ b/lfx-preflight/SKILL.md @@ -1,9 +1,9 @@ --- name: lfx-preflight description: > - Pre-PR validation for any LFX repo — license headers, format, lint, build, - protected file check, and 15 code review guard checks for Angular repos. - Adapts to repo type (Angular or Go). Use before submitting any PR. + Pre-PR validation for any LFX repo — Phase 1 auto-fixes (license, format, lint, build), + Phase 2 code review (15 report-only checks for Angular). Adapts to repo type. + Use before submitting any PR. Pass --skip-review to skip Phase 2. allowed-tools: Bash, Read, Write, Edit, Glob, Grep, AskUserQuestion --- @@ -15,7 +15,16 @@ allowed-tools: Bash, Read, Write, Edit, Glob, Grep, AskUserQuestion You are running a comprehensive validation before the contributor submits a pull request. Adapt checks based on the repo type. -**Mode:** By default, auto-fix issues where possible (formatting, license headers). If the user says "report only" or "dry run", just report without fixing. +**Two phases:** + +- **Phase 1: Validation** — mechanical checks with auto-fix (all repo types) +- **Phase 2: Code Review** — 15 report-only pattern checks (Angular repos only) + +**Modes:** + +- **Default:** Run both phases. Phase 1 auto-fixes where possible (formatting, license headers). Phase 2 is always report-only. +- **`--skip-review`:** Run Phase 1 only, skip Phase 2. Useful during development when you just want to validate build/lint. +- **"report only" / "dry run":** Phase 1 reports without fixing. Phase 2 is unchanged (always report-only). ## Repo Type Detection @@ -27,7 +36,13 @@ elif [ -f go.mod ]; then fi ``` -## Check 0: Working Tree Status +--- + +# Phase 1: Validation + +Mechanical checks that can be auto-fixed. Runs for all repo types. + +## Check 1: Working Tree Status ```bash git status @@ -42,7 +57,7 @@ git log --format="%h %s%n%b" origin/main...HEAD - **Commit messages missing JIRA ticket?** — Flag commits without `LFXV2-` references. - **Commits missing `--signoff`?** — Flag any commits without `Signed-off-by:` lines. -## Check 1: License Headers +## Check 2: License Headers **Angular repos:** ```bash @@ -59,7 +74,7 @@ Every source file (`.ts`, `.html`, `.scss`) must have the license header. **Go repos:** Check for license headers in `.go` files. The standard Go license header format varies by repo — check existing files for the pattern. -## Check 2: Formatting +## Check 3: Formatting **Angular repos:** ```bash @@ -75,7 +90,7 @@ gofmt -l . gofmt -w . ``` -## Check 3: Linting +## Check 4: Linting **Angular repos:** ```bash @@ -96,12 +111,12 @@ golangci-lint run ./... ### Re-validation -If fixes were applied in Checks 1-3, re-run lint to confirm: +If fixes were applied in Checks 2-4, re-run lint to confirm: **Angular:** `yarn lint` **Go:** `go vet ./...` -## Check 4: Build Verification +## Check 5: Build Verification **Angular repos:** ```bash @@ -122,7 +137,7 @@ go build ./... 3. If it's a simple fix (missing import, typo), fix it in auto-fix mode 4. If it's a structural issue, report it with context -## Check 5: Tests (if available) +## Check 6: Tests (if available) **Angular repos:** ```bash @@ -140,76 +155,112 @@ go test ./... Report test results but don't block on test failures unless the user asks. -## Check 6: Code Review Guard (Angular repos only) +## Check 7: Protected Files Check -**Skip this entire check for Go repos.** +```bash +git diff --name-only origin/main...HEAD +``` -Scope all sub-checks to **changed files only** (`git diff --name-only origin/main...HEAD`). These checks catch the patterns most commonly flagged by reviewers across 20+ LFX PRs. They are split into auto-fix and advisory categories. +**Angular repos — flag changes to:** -### Auto-Fix Sub-Checks +- `apps/lfx-one/src/server/server.ts` +- `apps/lfx-one/src/server/server-logger.ts` +- `apps/lfx-one/src/server/middleware/*` +- `apps/lfx-one/src/server/services/logger.service.ts` +- `apps/lfx-one/src/server/services/microservice-proxy.service.ts` +- `apps/lfx-one/src/server/services/nats.service.ts` +- `apps/lfx-one/src/server/services/snowflake.service.ts` +- `apps/lfx-one/src/server/services/supabase.service.ts` +- `apps/lfx-one/src/server/services/ai.service.ts` +- `apps/lfx-one/src/server/services/project.service.ts` +- `apps/lfx-one/src/server/services/etag.service.ts` +- `apps/lfx-one/src/server/helpers/error-serializer.ts` +- `apps/lfx-one/src/app/app.routes.ts` +- `.husky/*` +- `eslint.config.*` +- `.prettierrc*` +- `turbo.json` +- `apps/lfx-one/angular.json` +- `CLAUDE.md` +- `check-headers.sh` +- `package.json` / `*/package.json` +- `yarn.lock` -These can be fixed automatically in auto-fix mode, similar to formatting and license headers. +**Go repos — flag changes to:** -#### 6a. Raw HTML Form Elements (MOST COMMON blocker) +- `gen/` (should only change via `make apigen`) +- `charts/` (deployment config — review carefully) +- `go.mod` / `go.sum` (dependency changes need review) +- `Makefile` (build system changes) -Search **changed `.html` files** for raw form elements that must use LFX wrappers: +## Check 8: Commit Verification -| Raw Element | Required Wrapper | -| --- | --- | -| `` from PrimeNG | +```bash +git status +git log --format="%h %s%n%b" origin/main...HEAD +``` -**Exceptions:** Elements inside comments, or `` are acceptable. +- **All changes committed?** — If auto-fixes created uncommitted changes, prompt to commit them +- **Commit messages follow conventions?** — `type(scope): description` format +- **`--signoff` on all commits?** +- **JIRA ticket referenced?** -**Note:** LFX wrappers require `FormGroup` + `FormControl` — `ngModel` is not supported. +## Check 9: Change Summary -**Auto-fix:** Replace raw elements with LFX wrapper equivalents, preserving attributes. +```bash +git diff --stat origin/main...HEAD +``` -**Severity:** BLOCKER +List: -#### 6b. Dead Imports and Unused Providers +1. **New files created** — with their purpose +2. **Modified files** — with what changed +3. **Shared package changes** (Angular) or **Domain model changes** (Go) +4. **Backend changes** — controllers/services/routes (Angular) or Goa design/service (Go) +5. **Frontend changes** (Angular only) +6. **Helm chart changes** (Go repos with `charts/`) -Search **all changed `.ts` files** for: +--- -- **Unused imports** — imported symbols not referenced in the file body -- **Unused providers** — `providers: [...]` entries in component metadata where the service is never injected via `inject()` or constructor -- **Unbound component outputs** — when a template uses a child component (e.g., ``), check if that component emits outputs (e.g., `viewVote`, `rowClick`, `refresh`) that the parent template doesn't bind. Missing output bindings mean user interactions silently do nothing. +# Phase 2: Code Review (Angular repos only) -**Auto-fix:** Remove unused imports and providers. Flag unbound outputs for manual review. +**Skip this entire phase for Go repos or if `--skip-review` was passed.** -**Severity:** BLOCKER +These checks are **always report-only** — they never auto-fix. The transformations are not safe to do mechanically (wrapper replacements need FormGroup wiring, `!` in `.ts` can be definite assignment, `BehaviorSubject` → `signal` needs cross-file changes, `ChangeDetectorRef` removal isn't always safe). -#### 6c. Type Safety +Scope all checks to **changed files only** (`git diff --name-only origin/main...HEAD`). These catch the patterns most commonly flagged by reviewers across 20+ LFX PRs. -Search **changed `.html` templates** and **`.ts` files** for: +## Review Check 1: Raw HTML Form Elements -- **Non-null assertions (`!`)** — patterns like `data()!.field` or `item!.property` in templates. These cause runtime crashes when the value is null/undefined. Use `?.` and `@if (data(); as d)` guards instead. -- **Falsy `||` vs nullish `??`** — using `||` where `??` is needed. `value || null` treats `0`, `""`, and `false` as falsy — hiding valid zero counts (e.g., `total_members || null` hides `0` members). Use `??` to only coalesce on `null`/`undefined`. +Search **changed `.html` files** for raw form elements that must use LFX wrappers: -**Auto-fix:** Replace `!` with `?.` or `@if` guards where safe. Replace `||` with `??` for null-coalescing contexts. Flag ambiguous cases for manual review. +| Raw Element | Required Wrapper | +| --- | --- | +| `` from PrimeNG | -**Severity:** BLOCKER for `!` assertions. DISCUSS for `||` vs `??`. +**Exceptions:** Elements inside comments, or `` are acceptable. -#### 6d. Signal Pattern Compliance +**Note:** LFX wrappers require `FormGroup` + `FormControl` — `ngModel` is not supported. -Search **changed `*.component.ts` and `*.service.ts` files** for: +**Severity:** BLOCKER — reviewers will always flag this. -- **`BehaviorSubject` for simple state** — should use `signal()` instead. `BehaviorSubject` is only appropriate for complex async streams. -- **`cdr.detectChanges()` or `ChangeDetectorRef`** — not needed in zoneless Angular 20. The framework handles change detection. -- **`model()` for internal state** — `model()` creates a two-way bindable input/output on the component's public API. For internal-only state (e.g., dialog visibility, drawer toggles not exposed to parents), use `signal()` instead. Only use `model()` when the parent component needs two-way binding (e.g., `[(visible)]="childVisible"`). -- **Signals not initialized inline** — per `component-organization.md`, simple `WritableSignal`s must be initialized directly (e.g., `loading = signal(false)`), not in the constructor. +## Review Check 2: Dead Code -**Auto-fix:** Replace `BehaviorSubject` with `signal()` for simple state. Remove `ChangeDetectorRef` injections and `detectChanges()` calls. Replace `model()` with `signal()` for internal-only state. Flag complex cases for manual review. +For **TypeScript-only** items, search **all changed `.ts` files** for: -**Severity:** BLOCKER for `BehaviorSubject` misuse. DISCUSS for `ChangeDetectorRef` (may be legacy code) and `model()` misuse. +- **Unused imports** — imported symbols not referenced in the file body +- **Unused providers** — `providers: [...]` entries in component metadata where the service is never injected via `inject()` or constructor +- **Unused methods** — private methods not called anywhere in the file; public methods in services not called from any changed file +- **Unused signals** — signals declared but never read (called) in the template or class -### Advisory Sub-Checks (report only) +For **unbound component outputs**, search **changed `.html` templates** (and, as needed, the referenced child component's `.ts` definition to see its `@Output()`s or `output()`s) for cases where a template uses a child component (e.g., ``) but does not bind its emitted outputs (e.g., `viewVote`, `rowClick`, `refresh`). Missing output bindings mean user interactions silently do nothing. -These require human judgment and are reported as discussion items, not auto-fixed. +**Severity:** BLOCKER for unused providers/imports and unbound outputs. DISCUSS for unused methods (may be used externally). -#### 6e. Component Responsibility +## Review Check 3: Component Responsibility Search **changed `*.component.ts` files** for service injection count (count `inject()` calls and constructor injections): @@ -218,7 +269,7 @@ Search **changed `*.component.ts` files** for service injection count (count `in **Severity:** DISCUSS — guideline, not a hard rule. -#### 6f. Loading States +## Review Check 4: Loading States Search **changed `.html` templates** and **`.ts` files** for: @@ -231,7 +282,20 @@ Every data display that starts empty and populates asynchronously needs an expli **Severity:** BLOCKER for showing `0` during load. DISCUSS for missing re-fetch reset. -#### 6g. Error Handling +## Review Check 5: Type Safety + +Search **changed `.html` templates** for: + +- **Non-null assertions (`!`)** — patterns like `data()!.field` or `item!.property` in templates. These cause runtime crashes when the value is null/undefined. Use `?.` and `@if (data(); as d)` guards instead. + +Search **changed `.ts` files** for: + +- **Non-null assertions (`!`)** — in `.ts` files, `!` is also used for definite assignment (`foo!: T`) and may already be runtime-guarded. Report for manual review only — do not suggest automatic replacement. +- **Falsy `||` vs nullish `??`** — using `||` where `??` is needed. `value || null` treats `0`, `""`, and `false` as falsy — hiding valid zero counts (e.g., `total_members || null` hides `0` members). Use `??` to only coalesce on `null`/`undefined`. + +**Severity:** BLOCKER for `!` assertions in templates. DISCUSS for `!` in `.ts` and `||` vs `??`. + +## Review Check 6: Error Handling Search **changed `.ts` files** for: @@ -242,7 +306,18 @@ Search **changed `.ts` files** for: **Severity:** BLOCKER for silent or unreachable `catchError`. DISCUSS for inconsistent fallbacks. -#### 6h. Upstream API Alignment +## Review Check 7: Signal Pattern Compliance + +Search **changed `*.component.ts` and `*.service.ts` files** for: + +- **`BehaviorSubject` for simple state** — should use `signal()` instead. `BehaviorSubject` is only appropriate for complex async streams that need RxJS operators. +- **`cdr.detectChanges()` or `ChangeDetectorRef`** — often not required in zoneless Angular 20 when using Angular's reactive primitives (signals, `AsyncPipe`, `toSignal`, etc.), but may still be needed for integrations with non-Angular event sources or advanced `OnPush` patterns. Flag for manual review. +- **`model()` for internal state** — `model()` creates a two-way bindable input/output on the component's public API. For internal-only state (e.g., dialog visibility, drawer toggles not exposed to parents), use `signal()` instead. Only use `model()` when the parent component needs two-way binding (e.g., `[(visible)]="childVisible"`). +- **Signals not initialized inline** — per `component-organization.md`, simple `WritableSignal`s must be initialized directly (e.g., `loading = signal(false)`), not in the constructor. + +**Severity:** BLOCKER for `BehaviorSubject` misuse. DISCUSS for `ChangeDetectorRef` and `model()` usage. + +## Review Check 8: Upstream API Alignment Search **changed `.ts` files** for API calls and verify: @@ -257,7 +332,7 @@ If you cannot verify the upstream contract from the local codebase, flag for man **Severity:** BLOCKER for clearly wrong parameter names. DISCUSS for fields needing upstream verification. -#### 6i. PR Description Completeness +## Review Check 9: PR Description Completeness Check the **git log and diff** for changes that need explicit documentation in the PR description: @@ -267,7 +342,7 @@ Check the **git log and diff** for changes that need explicit documentation in t **Severity:** DISCUSS -#### 6j. Accessibility +## Review Check 10: Accessibility Search **changed `.html` templates** for: @@ -278,7 +353,7 @@ Search **changed `.html` templates** for: **Severity:** DISCUSS -#### 6k. Design Token Compliance +## Review Check 11: Design Token Compliance Search **changed `.html` templates** for hardcoded Tailwind color classes that should use LFX design tokens: @@ -286,7 +361,7 @@ Search **changed `.html` templates** for hardcoded Tailwind color classes that s **Severity:** DISCUSS -#### 6l. N+1 API Patterns +## Review Check 12: N+1 API Patterns Search **changed `.ts` files** for per-item API calls inside loops: @@ -295,7 +370,7 @@ Search **changed `.ts` files** for per-item API calls inside loops: **Severity:** DISCUSS -#### 6m. Template/Config Completeness +## Review Check 13: Template/Config Completeness Search **changed `.html` templates and `.component.ts` files** for: @@ -305,7 +380,7 @@ Search **changed `.html` templates and `.component.ts` files** for: **Severity:** BLOCKER for missing switch cases. DISCUSS for partial wiring. -#### 6n. Stale Data During Navigation +## Review Check 14: Stale Data During Navigation Search **changed `*.component.ts` files** for: @@ -315,122 +390,37 @@ Search **changed `*.component.ts` files** for: **Severity:** DISCUSS -#### 6o. Visitor/Permission Gating +## Review Check 15: Visitor/Permission Gating Search **changed `.html` templates** for: - **Content visible during role loading** — `@if (!isVisitor())` evaluates to `true` while `myRoleLoading()` is still `true` (because `isVisitor()` defaults to `false`). Fix: add `!myRoleLoading()` to the guard. -- **Visitor blur bypass** — blur overlays that don't prevent keyboard/screen-reader access (see 6j). +- **Visitor blur bypass** — blur overlays that don't prevent keyboard/screen-reader access (see Review Check 10). - **Permission changes not documented** — if the diff adds/removes/changes `canEdit()`, `isVisitor()`, `hasPMOAccess()` checks, flag for PR description. **Severity:** BLOCKER for content flashing during role loading. DISCUSS for blur bypass. -### Check 6 Results - -Report Check 6 results grouped by severity: - -```text -Review guard (Angular): - Auto-fixed: - - Replaced 2 raw elements with wrappers - - Removed 3 unused imports - Blockers: - ✗ 6f. committee-votes.component.ts — loading not reset in switchMap - ✗ 6m. committee-view.component.html — missing @case ('settings') in @switch - Discussion: - ⚠ 6e. overview.component.ts — 5 service injections (consider splitting) - ⚠ 6k. committee-surveys.component.html — hardcoded bg-blue-50 (use design token) -``` - -## Check 7: Protected Files Check - -```bash -git diff --name-only origin/main...HEAD -``` - -**Angular repos — flag changes to:** - -- `apps/lfx-one/src/server/server.ts` -- `apps/lfx-one/src/server/server-logger.ts` -- `apps/lfx-one/src/server/middleware/*` -- `apps/lfx-one/src/server/services/logger.service.ts` -- `apps/lfx-one/src/server/services/microservice-proxy.service.ts` -- `apps/lfx-one/src/server/services/nats.service.ts` -- `apps/lfx-one/src/server/services/snowflake.service.ts` -- `apps/lfx-one/src/server/services/supabase.service.ts` -- `apps/lfx-one/src/server/services/ai.service.ts` -- `apps/lfx-one/src/server/services/project.service.ts` -- `apps/lfx-one/src/server/services/etag.service.ts` -- `apps/lfx-one/src/server/helpers/error-serializer.ts` -- `apps/lfx-one/src/app/app.routes.ts` -- `.husky/*` -- `eslint.config.*` -- `.prettierrc*` -- `turbo.json` -- `apps/lfx-one/angular.json` -- `CLAUDE.md` -- `check-headers.sh` -- `package.json` / `*/package.json` -- `yarn.lock` - -**Go repos — flag changes to:** - -- `gen/` (should only change via `make apigen`) -- `charts/` (deployment config — review carefully) -- `go.mod` / `go.sum` (dependency changes need review) -- `Makefile` (build system changes) - -## Check 8: Commit Verification - -```bash -git status -git log --format="%h %s%n%b" origin/main...HEAD -``` - -- **All changes committed?** — If auto-fixes created uncommitted changes, prompt to commit them -- **Commit messages follow conventions?** — `type(scope): description` format -- **`--signoff` on all commits?** -- **JIRA ticket referenced?** - -## Check 9: Change Summary - -```bash -git diff --stat origin/main...HEAD -``` - -List: - -1. **New files created** — with their purpose -2. **Modified files** — with what changed -3. **Shared package changes** (Angular) or **Domain model changes** (Go) -4. **Backend changes** — controllers/services/routes (Angular) or Goa design/service (Go) -5. **Frontend changes** (Angular only) -6. **Helm chart changes** (Go repos with `charts/`) +--- -## Results Report +# Results Report -**Start with a one-line plain-language verdict** before any details: +**Output the two-phase report:** ```text ═══════════════════════════════════════════ PREFLIGHT RESULTS ═══════════════════════════════════════════ -YOUR CHANGES LOOK GOOD AND ARE READY FOR REVIEW! -(or: FOUND 2 ISSUES THAT NEED ATTENTION — see below) - -───────────────────────────────────────── -Detailed checks: -✓ Working tree — Clean, N commits ahead of main -✓ License headers — All files have headers (2 auto-fixed) -✓ Formatting — Applied (3 files reformatted) -✓ Linting — No errors -✓ Build — Succeeded -✓ Tests — N/A (no test files for changed code) -✓ Review guard — 2 auto-fixed, 0 blockers, 1 discussion item (see below) -✓ Protected files — None modified -✓ Commits — Conventions followed, signed off -═══════════════════════════════════════════ +PHASE 1: VALIDATION +═══════════════════ +✓ Working tree — Clean, N commits ahead of main +✓ License headers — All files have headers (2 auto-fixed) +✓ Formatting — Applied (3 files reformatted) +✓ Linting — No errors +✓ Build — Succeeded +✓ Tests — N/A (no test files for changed code) +✓ Protected files — None modified +✓ Commits — Conventions followed, signed off Changes summary: - 2 files modified in packages/shared/ @@ -442,11 +432,50 @@ Auto-fixes applied: - Added license header to member-form.component.html - Reformatted 3 files with prettier -READY FOR PR ✓ +PHASE 2: CODE REVIEW (Angular) +══════════════════════════════ + 1. ✓ Raw HTML elements — No raw inputs/selects/textareas found + 2. ⚠ Dead code — 2 unused imports in settings.service.ts + 3. ✓ Component size — All components under 4 injections + 4. ✓ Loading states — All data displays have loading guards + 5. ✓ Type safety — No non-null assertions or || vs ?? issues + 6. ✓ Error handling — All catchError blocks have logging + 7. ✓ Signal patterns — No BehaviorSubject or ChangeDetectorRef issues + 8. ✓ API alignment — Parameters match upstream contracts + 9. ✓ PR description — Behavioral changes documented +10. ⚠ Accessibility — Toggle buttons missing aria-pressed (discuss) +11. ✓ Design tokens — No hardcoded color classes +12. ✓ N+1 patterns — No per-item API calls in loops +13. ✓ Template completeness — All config entries have matching template cases +14. ✓ Stale data — Components re-fetch on input changes +15. ✓ Visitor gating — Permission guards include loading checks + +═══════════════════════════════════════════ +REVIEW READY (2 discussion items) ═══════════════════════════════════════════ ``` -### If Fixes Created Uncommitted Changes +**Legend:** + +- `✓` — Pass. No issues found. +- `⚠` — Discuss. Not a blocker, but should be considered. +- `✗` — Blocker. Must be fixed before submitting the PR. + +**Final verdict line:** + +- All `✓` → `REVIEW READY` +- Has `⚠` only → `REVIEW READY (N discussion items)` +- Has any `✗` in Phase 1 or Phase 2 → `NOT READY — N blockers must be fixed` + +### If Blockers Are Found + +For each blocker, provide: + +1. **File and line** where the issue occurs +2. **What's wrong** in plain language +3. **How to fix it** with a concrete example + +### If Phase 1 Auto-Fixes Created Uncommitted Changes After auto-fixing, check for uncommitted changes: @@ -473,14 +502,15 @@ Report failures in plain language, explaining what each means: ## Scope Boundaries **This skill DOES:** -- Run format, lint, build checks -- Auto-fix formatting, license headers, raw HTML wrappers, dead imports, type safety, and signal patterns -- Run 15 code review guard checks for Angular repos (common reviewer blocker patterns) +- Run format, lint, build checks (Phase 1, auto-fix) +- Auto-fix formatting and license headers +- Run 15 code review pattern checks for Angular repos (Phase 2, report-only) - Report protected file changes - Verify commit conventions - Offer to create PR after passing **This skill does NOT:** +- Auto-fix Phase 2 code review findings (report-only — contributor decides) - Generate new code (use `/lfx-backend-builder` or `/lfx-ui-builder`) - Make architectural decisions (use `/lfx-product-architect`) - Research upstream APIs (use `/lfx-research`)