diff --git a/README.md b/README.md index 86ecf54..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, 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,9 +281,9 @@ 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 @@ -294,7 +294,10 @@ Runs a comprehensive **pre-PR validation** with auto-fix capabilities. Adapts al 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. --- @@ -418,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 → 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 70fed93..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, - and protected file check. 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,7 +155,7 @@ go test ./... Report test results but don't block on test failures unless the user asks. -## Check 6: Protected Files Check +## Check 7: Protected Files Check ```bash git diff --name-only origin/main...HEAD @@ -178,7 +193,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 +205,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 @@ -205,29 +220,207 @@ List: 5. **Frontend changes** (Angular only) 6. **Helm chart changes** (Go repos with `charts/`) -## Results Report +--- + +# Phase 2: Code Review (Angular repos only) + +**Skip this entire phase for Go repos or if `--skip-review` was passed.** + +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). + +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. + +## Review Check 1: Raw HTML Form Elements + +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. + +**Severity:** BLOCKER — reviewers will always flag this. + +## Review Check 2: Dead Code + +For **TypeScript-only** items, 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 +- **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 + +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. + +**Severity:** BLOCKER for unused providers/imports and unbound outputs. DISCUSS for unused methods (may be used externally). + +## Review Check 3: 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. + +## Review Check 4: 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. + +## 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: + +- **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. + +## 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. -**Start with a one-line plain-language verdict** before any details: +## Review Check 8: 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. + +## Review Check 9: 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 + +## Review Check 10: 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 + +## Review Check 11: 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 + +## Review Check 12: 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 + +## Review Check 13: 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. + +## Review Check 14: 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 + +## 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 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. + +--- + +# Results Report + +**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) -✓ 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/ @@ -239,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: @@ -270,13 +502,15 @@ Report failures in plain language, explaining what each means: ## Scope Boundaries **This skill DOES:** -- Run format, lint, build checks +- 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`)