code-review

name	code-review
description	Architectural and quality review of one or more files. Verifies coding standards, SOLID, testability, mobile anti-patterns, and ADR compliance. Use after /dev-story and before /story-done.
argument-hint	[file-or-directory-path]
user-invocable	true
allowed-tools	Read, Glob, Grep, Bash, Task
agent	lead-developer
model	sonnet

Code Review

Read-only quality pass over a recent change. Confirms the diff respects the project's coding rules, the governing ADR, and mobile-specific health checks (cold start, frame budget, memory, battery, deep-link surface).

---

Phase 1: Load Targets

Read the target file(s) in full. If a directory is given, glob it for source files (*.ts, *.tsx, *.js, *.dart, *.swift, *.kt, *.kts, *.java, *.m, *.mm).

Read CLAUDE.md and .claude/docs/coding-standards.md to refresh the active project rules. Read .claude/docs/technical-preferences.md to capture frame budget, cold-start budget, memory ceiling, and forbidden patterns.

---

Phase 2: Identify Framework Specialists

Open the ## Framework Specialists section of technical-preferences.md. Note:

• Primary specialist (architecture and lifecycle calls).
• Language/UI specialist (RN screens, SwiftUI views, Compose composables, Flutter widgets).
• Native modules / platform channels specialist (bridges, JNI, Obj-C++).
• State management specialist (Redux, Riverpod, MVI, Combine, etc.).

If the section is [TO BE CONFIGURED], skip the framework-specialist phase.

---

Phase 3: ADR Compliance

Search the changed files (and any matching story file) for ADR references: patterns like ADR-NNN, docs/architecture/ADR-, or Governing ADR:. If none are found, log: "No ADR references found — ADR compliance check skipped."

For each referenced ADR, read it and pull out the Decision and Consequences sections. Classify each deviation:

• BLOCKING — the code uses a pattern explicitly rejected by the ADR.
• DRIFT — the code diverges from the chosen approach without using a forbidden pattern.
• MINOR — small textual difference that does not change architecture.

---

Phase 4: Standards Compliance

For each target file, score these checks:

• All public APIs (exports, public methods, public protocols) carry doc comments.
• No method exceeds 40 lines (excluding data declarations).
• Cyclomatic complexity stays under 10 per method.
• Dependencies are injected; no module-scope singletons holding mutable app state.
• Tunable values come from config/remote config, not magic numbers.
• Modules expose interfaces (TS interface, Swift protocol, Kotlin interface, abstract Dart class) rather than concrete implementations.
• No hardcoded user-facing strings — all strings flow through the localization system.

Cite line numbers for every failure.

---

Phase 5: Architecture and SOLID

Architecture:

• Dependency direction is correct (UI -> domain -> data, never reverse).
• No circular module imports.
• UI does not own domain state directly; state passes through a state container.
• Cross-feature communication goes through events / streams / navigation, not direct calls.
• New code matches the patterns already in the codebase — flag novelty.

SOLID:

• Single Responsibility: each class/screen/hook has one reason to change.
• Open/Closed: extension via composition, not edits to closed code.
• Liskov Substitution: subtypes honour parent contracts.
• Interface Segregation: no fat protocols/interfaces.
• Dependency Inversion: depend on abstractions, not concretes.

---

Phase 6: Mobile-Specific Concerns

• Cold-start budget respected — no synchronous heavy work in app entry, AppDelegate.didFinishLaunching, MainActivity.onCreate, or React Native App.tsx top-level.
• No allocations in animation callbacks, gesture handlers, or list cell builders.
• List rendering uses virtualization (FlatList/SectionList, LazyColumn, LazyVStack, ListView.builder).
• Image loads use a cache layer (FastImage, Coil, SDWebImage, cached_network_image).
• Network calls have explicit timeout, retry, and offline handling.
• Background work uses platform-correct APIs (WorkManager, BGTaskScheduler).
• Deep links and universal links validate input before navigating.
• Sensitive data lands in Keychain / Keystore, never AsyncStorage, SharedPreferences, NSUserDefaults, or unencrypted SQLite.
• Battery-affecting features (location, camera, BLE) release resources on screen exit.
• No string-concatenated SQL; use parameterized queries.
• Memory: no retain cycles in closures (Swift [weak self], Dart avoid capturing State, RN avoid capturing component refs).

---

Phase 7: Specialist Reviews (in parallel)

Spawn every applicable specialist via Task at the same time — do not chain.

Framework specialists

Decide per file:

• React Native screens / hooks / RN-specific code -> RN specialist.
• Flutter widgets / Dart code -> Flutter specialist.
• SwiftUI / UIKit / Swift code -> iOS specialist (UI variant if SwiftUI).
• Compose / Views / Kotlin code -> Android specialist (Compose variant if Compose).
• Native module / platform channel / JNI code -> native modules specialist.
• Cross-cutting (build config, app entry, lifecycle) -> primary specialist.

Testability review

For Logic and Integration stories, also spawn qa-tester in parallel. Pass:

• The diff under review.
• The story's ## QA Test Cases section.
• The story's Acceptance Criteria.

Ask qa-tester to verify:

• Test seams exist — public hooks, exported functions, dependency injection points.
• Each QA test case maps to a code path that is reachable in test mode.
• No AC is hidden behind a hardcoded value or non-injectable singleton.
• No new edge cases are introduced without a corresponding test.
• Observable side effects (analytics events, navigation, persistence) have at least an interaction test.

For Visual/Feel and UI stories: qa-tester instead checks that the manual verification steps in QA Test Cases are physically reachable in the build (e.g., the state transition the tester needs to observe is reachable).

Wait for all specialists to return before composing output.

---

Phase 8: Render the Review

## Code Review: [target]

### Framework Specialist Findings: [N/A | CLEAN | ISSUES]
[per-specialist output]

### Testability: [N/A | TESTABLE | GAPS | BLOCKING]
[qa-tester output — list test seams missing or untestable ACs]

### ADR Compliance: [NONE FOUND | COMPLIANT | DRIFT | BLOCKING]
[ADR-by-ADR result and severity]

### Standards Compliance: [X/7 passing]
[failures with file:line]

### Architecture: [CLEAN | MINOR | VIOLATIONS]
[specific concerns]

### SOLID: [COMPLIANT | ISSUES]
[specific violations]

### Mobile-Specific Concerns
[cold start, frame budget, battery, secure storage, deep links, etc.]

### Positive Observations
[Always include — what the change did well]

### Required Changes
[Must-fix before approval. Any BLOCKING ADR finding lands here.]

### Suggestions
[Nice-to-have improvements]

### Verdict: [APPROVED | APPROVED WITH SUGGESTIONS | CHANGES REQUIRED]

This skill writes nothing to disk.

---

Quality Gates / PASS-FAIL

• APPROVED — every required check passed and no BLOCKING items remain.
• APPROVED WITH SUGGESTIONS — required checks passed; nice-to-haves remain.
• CHANGES REQUIRED — at least one BLOCKING ADR violation, standards failure, mobile anti-pattern, or testability gap is present.

---

Examples

Example 1 — A new RN list screen: Reads src/screens/inbox/InboxScreen.tsx, finds ADR-022 (list virtualization policy), spawns RN specialist + qa-tester, flags an inline .map() over 2000+ items as a frame-budget violation, returns CHANGES REQUIRED.

Example 2 — A pricing config edit: Single file assets/data/pricing.json. Standards check passes; no ADR referenced. qa-tester confirms a smoke check exists for the price flow. Verdict: APPROVED.

---

Next Steps

• APPROVED -> run /story-done [story-path].
• CHANGES REQUIRED -> fix and re-run /code-review.
• BLOCKING ADR violation -> run /architecture-decision to capture the correct approach before re-implementing.