feat(lfx-preflight): add 15 code review guard checks for Angular repos

[manishdixitlfx]

Summary

• Fold review-guard checks into /lfx-preflight instead of a separate skill (per @niravpatel27 feedback on feat(lfx-review-guard): add self-review skill for common reviewer blockers #16)
• Add Check 6 (Code Review Guard) with 15 sub-checks for Angular repos
• Update README to reflect the expanded preflight capability

Why not a separate skill?

Per Nirav's comment on PR #16:

1. Single workflow — contributors already know code → /lfx-preflight → PR
2. Auto-fix capability — preflight has Write/Edit tools for mechanical fixes
3. No overlap confusion — one pre-PR gate, not two

15 Sub-Checks (Check 6)

Auto-fix (4):

• 6a. Raw HTML form elements → LFX wrappers
• 6b. Dead imports and unused providers
• 6c. Type safety (! assertions, || vs ??)
• 6d. Signal patterns (BehaviorSubject, ChangeDetectorRef, model() misuse)

Advisory / report only (11):

• 6e. Component responsibility (4+ injections)
• 6f. Loading states (missing guards, no reset on re-fetch)
• 6g. Error handling (silent/duplicate catchError)
• 6h. Upstream API alignment
• 6i. PR description completeness
• 6j. Accessibility (aria-pressed, nested interactives, overlay focus)
• 6k. Design token compliance
• 6l. N+1 API patterns
• 6m. Template/config completeness (missing switch cases)
• 6n. Stale data during navigation
• 6o. Visitor/permission gating

All gated behind REPO_TYPE=angular — skipped for Go repos.

Bug fixes from PR #16 review

• Removed incorrect allowSignalWrites rule for form patchValue()/setValue() (that flag is for signal writes, not imperative form updates)
• Fixed naming: /preflight → /lfx-preflight, /develop → /lfx-coordinator

JIRA

LFXV2-1312

Test plan

• Run /lfx-preflight in an Angular repo — verify Check 6 review guard sub-checks appear
• Run /lfx-preflight in a Go repo — verify Check 6 is skipped
• Verify README renders correctly on GitHub
• Verify no references to /lfx-review-guard remain anywhere

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR expands /lfx-preflight to include an Angular-only “Code Review Guard” (15 sub-checks) and updates documentation to reflect the new preflight workflow and renumbered checks.

Changes:

• Added “Check 6: Code Review Guard” (Angular-only) with 15 sub-checks, including auto-fix and advisory categories.
• Renumbered subsequent checks in lfx-preflight/SKILL.md and updated the sample results output accordingly.
• Updated README.md skill listing and /lfx-preflight step breakdown to include the new guard step.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
lfx-preflight/SKILL.md	Adds Check 6 review-guard spec, defines sub-check behaviors/severity, and renumbers downstream checks.
README.md	Updates /lfx-preflight description and step list to include the Angular review guard.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In Check 6b, the “unbound component outputs” item is described as a template-level issue, but this section currently scopes the search to changed .ts files only. That makes the check easy to mis-apply (and likely miss real cases). Consider scoping this sub-check to changed .html files (and/or explicitly say it requires reading the referenced child component’s definition to know what outputs exist).

Suggested change

	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., `<lfx-votes-table>`), 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.
	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
	For **unbound component outputs**, search **changed `.html` templates** (and, as needed, the referenced child component’s `.ts` definition to see its `@Output()`s) for cases where a template uses a child component (e.g., `<lfx-votes-table>`) but does not bind its emitted outputs (e.g., `viewVote`, `rowClick`, `refresh`). Missing output bindings mean user interactions silently do nothing.

[Copilot]

Check 6c scopes non-null assertion detection to changed .ts files as well as templates, and proposes auto-fixing !. In TypeScript, ! is also used for definite assignment (foo!: T) and for cases that are already runtime-guarded; auto-rewriting in .ts can be incorrect and change semantics/types. Suggest limiting the auto-fix to template expressions (or to very specific expression patterns) and treating .ts non-null assertions as report-only unless you can prove a safe transformation.

Suggested change

	- **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 `??`.
	- **Non-null assertions (`!`)** — patterns like `data()!.field` or `item!.property` in templates. These cause runtime crashes when the value is null/undefined. In **templates**, prefer `?.` and `@if (data(); as d)` guards instead. In **`.ts` files**, treat `!` as a report-only signal and require manual review, since `!` is also used for definite assignment (`foo!: T`) and may already be runtime-guarded.
	- **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:** In **templates**, replace `!` with `?.` or `@if` guards where the transformation is clearly safe. In **`.ts` files**, do **not** auto-rewrite `!` by default; only suggest changes (or apply them) when a safe transformation can be proven. Replace `||` with `??` for null-coalescing contexts. Flag ambiguous cases for manual review.
	**Severity:** BLOCKER for unsafe `!` assertions in templates. REPORT/DISCUSS for `!` in `.ts` (manual review only). DISCUSS for `||` vs `??`.

[Copilot]

Check 6d states ChangeDetectorRef/detectChanges() are “not needed in zoneless Angular 20” and then lists removing them as an auto-fix. This isn’t generally safe: ChangeDetectorRef may still be required for integrations outside Angular’s reactive primitives (e.g., non-Angular event sources, manual markForCheck in OnPush views, etc.). Recommend changing this sub-check to advisory-only and not auto-removing ChangeDetectorRef/detectChanges() unless there is a very strict, provable pattern match showing it’s unused and redundant.

Suggested change

	- **`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.
	- **`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 usages for manual review rather than auto-removal.
	- **`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. Replace `model()` with `signal()` for internal-only state. For `ChangeDetectorRef` injections and `detectChanges()` calls, only flag for manual review; do not auto-remove except where they are trivially provable as unused/dead code.
	**Severity:** BLOCKER for `BehaviorSubject` misuse. DISCUSS (advisory-only, no auto-fix) for `ChangeDetectorRef` usage and `model()` misuse.

[Copilot]

The auto-fix guidance in 6d (e.g., replacing BehaviorSubject with signal() / replacing model() with signal()) can require non-trivial refactors across .ts + template usage and may be unsafe to do mechanically. Consider tightening the auto-fix scope to very specific, easily-verifiable patterns (and otherwise report-only) to avoid introducing subtle runtime behavior changes during preflight.

Suggested change

**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.

**Auto-fix:** Only apply auto-fixes for trivially safe, local patterns that can be mechanically verified (e.g., a private field used solely as internal state in the same `.ts` file, not referenced in templates, not exposed via inputs/outputs, and not used with RxJS operators). In those narrow cases, you MAY replace a `BehaviorSubject` or `model()` with a `signal()` and remove now-unused `ChangeDetectorRef` injections / `detectChanges()` calls. For any other or ambiguous usage, DO NOT auto-fix — report the finding for manual review only.

[niravpatel27]

@manishdixitlfx

Split into two phases

/lfx-preflight
Phase 1: Validation (auto-fix) — license, format, lint, build, protected files, commits
Phase 2: Code Review (report-only, Angular only) — the 15 checks

Add --skip-review flag so contributors can skip Phase 2 during dev and only run it before submitting.

All 15 checks should be report-only

Don't auto-fix any of the pattern checks. The transformations aren't safe to do mechanically:

• Raw HTML → LFX wrappers needs FormGroup wiring
• ! in .ts can be definite assignment
• BehaviorSubject → signal needs cross-file changes
• ChangeDetectorRef removal isn't always safe

Fix scoping issues

• "Unbound component outputs" (6b) — search .html not .ts
• ! assertions (6c) — report-only in .ts, auto-fix only in templates
• ChangeDetectorRef (6d) — advisory, don't auto-remove

Phase 2 runs last

Put the review report after all mechanical checks so it's the last thing the user sees before submitting.

Output format

PHASE 1: VALIDATION
═══════════════════
✓ License ✓ Format ✓ Lint ✓ Build ✓ Protected files ✓ Commits

PHASE 2: CODE REVIEW (Angular)
══════════════════════════════

1. ✓ Raw HTML — clean
2. ⚠ Dead code — 2 unused imports in settings.service.ts
3. ✓ Component size — ok
   ...
   REVIEW READY (1 discussion item)