From 31e7ee93ec0f5869f29344fe4b1a780c683f2bec Mon Sep 17 00:00:00 2001 From: Zhongwei Li Date: Sat, 29 Nov 2025 18:02:45 +0800 Subject: [PATCH] Initial commit --- .claude-plugin/plugin.json | 11 + README.md | 3 + plugin.lock.json | 89 ++ skills/component-designing/SKILL.md | 437 ++++++ skills/component-designing/reference.md | 811 +++++++++++ skills/documentation/SKILL.md | 557 ++++++++ skills/documentation/reference.md | 514 +++++++ skills/linter-driven-development/SKILL.md | 226 +++ skills/linter-driven-development/reference.md | 443 ++++++ skills/pre-commit-review/SKILL.md | 467 +++++++ skills/pre-commit-review/reference.md | 610 +++++++++ skills/refactoring/SKILL.md | 576 ++++++++ skills/refactoring/reference.md | 1210 +++++++++++++++++ skills/testing/SKILL.md | 529 +++++++ skills/testing/reference.md | 1012 ++++++++++++++ 15 files changed, 7495 insertions(+) create mode 100644 .claude-plugin/plugin.json create mode 100644 README.md create mode 100644 plugin.lock.json create mode 100644 skills/component-designing/SKILL.md create mode 100644 skills/component-designing/reference.md create mode 100644 skills/documentation/SKILL.md create mode 100644 skills/documentation/reference.md create mode 100644 skills/linter-driven-development/SKILL.md create mode 100644 skills/linter-driven-development/reference.md create mode 100644 skills/pre-commit-review/SKILL.md create mode 100644 skills/pre-commit-review/reference.md create mode 100644 skills/refactoring/SKILL.md create mode 100644 skills/refactoring/reference.md create mode 100644 skills/testing/SKILL.md create mode 100644 skills/testing/reference.md diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json new file mode 100644 index 0000000..9e3280d --- /dev/null +++ b/.claude-plugin/plugin.json @@ -0,0 +1,11 @@ +{ + "name": "ts-react-linter-driven-development", + "description": "Linter-driven development workflow for TypeScript + React with six specialized skills: component design, testing, refactoring, review, and documentation", + "version": "1.0.0", + "author": { + "name": "Dan Mordechay" + }, + "skills": [ + "./skills" + ] +} \ No newline at end of file diff --git a/README.md b/README.md new file mode 100644 index 0000000..a70932e --- /dev/null +++ b/README.md @@ -0,0 +1,3 @@ +# ts-react-linter-driven-development + +Linter-driven development workflow for TypeScript + React with six specialized skills: component design, testing, refactoring, review, and documentation diff --git a/plugin.lock.json b/plugin.lock.json new file mode 100644 index 0000000..c8d1b4e --- /dev/null +++ b/plugin.lock.json @@ -0,0 +1,89 @@ +{ + "$schema": "internal://schemas/plugin.lock.v1.json", + "pluginId": "gh:buzzdan/ai-coding-rules:ts-react-linter-driven-development", + "normalized": { + "repo": null, + "ref": "refs/tags/v20251128.0", + "commit": "bcf345fb510ba01153ec24639e5ca9cd9f72a2e1", + "treeHash": "7068f1fa7bd710d383ee171ac127a7217efb736143c2bc1d48b76430412c82bb", + "generatedAt": "2025-11-28T10:14:26.933214Z", + "toolVersion": "publish_plugins.py@0.2.0" + }, + "origin": { + "remote": "git@github.com:zhongweili/42plugin-data.git", + "branch": "master", + "commit": "aa1497ed0949fd50e99e70d6324a29c5b34f9390", + "repoRoot": "/Users/zhongweili/projects/openmind/42plugin-data" + }, + "manifest": { + "name": "ts-react-linter-driven-development", + "description": "Linter-driven development workflow for TypeScript + React with six specialized skills: component design, testing, refactoring, review, and documentation", + "version": "1.0.0" + }, + "content": { + "files": [ + { + "path": "README.md", + "sha256": "88c714ded41a20d0e885f68506be74c09a432b3c48903d3e68df71a45da5086d" + }, + { + "path": ".claude-plugin/plugin.json", + "sha256": "5c67acffb1f3fa9a6a78424d60b2074800b61f83eaca547a1e40bc521e3846a5" + }, + { + "path": "skills/component-designing/reference.md", + "sha256": "b104004e174e3c3cb6d629b35abf0725d775f26a3d2672b1367c667791e76a61" + }, + { + "path": "skills/component-designing/SKILL.md", + "sha256": "613edc2801972461e5a8ae9d08436f65d392c09f4d8dacd138cd8020ee2c3a31" + }, + { + "path": "skills/documentation/reference.md", + "sha256": "8b14ee0b70d4c97c6e47f9cba96419063412c10c1eca2c3b45350a27881c85a8" + }, + { + "path": "skills/documentation/SKILL.md", + "sha256": "ed77ae906b3a70afb162a041c9a2fb87cbeede1bf264de0877845b87c2300975" + }, + { + "path": "skills/pre-commit-review/reference.md", + "sha256": "ac754b5270d4b547140a3b5d0ba7e1133bccb5a07433b4b9475bcf1e4c33f1fb" + }, + { + "path": "skills/pre-commit-review/SKILL.md", + "sha256": "58a1220802d33cb653bd42870d8a125c6e57b9fac0229f8301191209485a83a2" + }, + { + "path": "skills/testing/reference.md", + "sha256": "28f5c25f988e908b7881b27103fd1ca78798b2bdb13d9d44be2043e7643929a8" + }, + { + "path": "skills/testing/SKILL.md", + "sha256": "d8843b9559be114a12d67ad3e955ae0055cfa908d948d99cf052c494e297d853" + }, + { + "path": "skills/refactoring/reference.md", + "sha256": "a22b0f6dd4d500ce8761c916decf15b42fddefb3a84a90d8a23862fb0b2e15c5" + }, + { + "path": "skills/refactoring/SKILL.md", + "sha256": "c972635223c54c681a96b2be78c5fb37031351d920b7d0d8b8dab35babeb6b29" + }, + { + "path": "skills/linter-driven-development/reference.md", + "sha256": "dc5b9fd11dddcce002f896352186b6b3285349623b8f2c86e2dcff76370e971c" + }, + { + "path": "skills/linter-driven-development/SKILL.md", + "sha256": "87a2d97e6078cb06d9c1014d0d7fa6b5c873d22580e1bc74cddd214a4df95ec1" + } + ], + "dirSha256": "7068f1fa7bd710d383ee171ac127a7217efb736143c2bc1d48b76430412c82bb" + }, + "security": { + "scannedAt": null, + "scannerVersion": null, + "flags": [] + } +} \ No newline at end of file diff --git a/skills/component-designing/SKILL.md b/skills/component-designing/SKILL.md new file mode 100644 index 0000000..e95a19e --- /dev/null +++ b/skills/component-designing/SKILL.md @@ -0,0 +1,437 @@ +--- +name: component-designing +description: Component and type design for TypeScript + React code. Use when planning new features, designing components and custom hooks, preventing primitive obsession, or when refactoring reveals need for new abstractions. Focuses on feature-based architecture and type safety. +--- + +# Component Designing + +Component and type design for TypeScript + React applications. +Use when planning new features or identifying need for new abstractions during refactoring. + +## When to Use +- Planning a new feature (before writing code) +- Refactoring reveals need for new components/hooks +- Linter failures suggest better abstractions +- When you need to think through component architecture +- Designing state management approach + +## Purpose +Design clean, well-composed components and types that: +- Prevent primitive obsession (use branded types, Zod schemas) +- Ensure type safety with TypeScript +- Follow component composition patterns +- Implement feature-based architecture +- Create reusable custom hooks + +## Workflow + +### 0. Architecture Pattern Analysis (FIRST STEP) + +**Default: Always use feature-based architecture** (group by feature, not technical layer). + +Scan codebase structure: +- **Feature-based**: `src/features/auth/{LoginForm,useAuth,types,AuthContext}.tsx` ✅ +- **Technical layers**: `src/{components,hooks,contexts}/auth.tsx` ⚠️ + +**Decision Flow**: +1. **Pure feature-based** → Continue pattern, implement as `src/features/[new-feature]/` +2. **Pure technical layers** → Propose: Start migration with `docs/architecture/feature-based-migration.md`, implement new feature as first feature slice +3. **Mixed (migrating)** → Check for migration docs, continue pattern as feature-based + +**Always ask user approval with options:** +- Option A: Feature-based (recommended for cohesion/maintainability) +- Option B: Match existing pattern (if time-constrained) +- Acknowledge: Time pressure, team decisions, consistency needs are valid + +**If migration needed**, create/update `docs/architecture/feature-based-migration.md`: +```markdown +# Feature-Based Architecture Migration Plan +## Current State: [technical-layers/mixed] +## Target: Feature-based structure in src/features/[feature]/ +## Strategy: New features feature-based, migrate existing incrementally +## Progress: [x] [new-feature] (this PR), [ ] existing features +``` + +See reference.md section #2 for detailed patterns. + +--- + +### 1. Understand Domain + +- What is the problem domain? +- What are the main UI concepts/interactions? +- What state needs to be managed? +- What are the user flows? +- How does this fit into existing architecture? + +### 2. Identify Core Abstractions + +Ask for each concept: +- Is this currently a primitive (string, number, boolean)? +- Does it have validation rules? +- Is it a UI concept (component)? +- Is it reusable logic (custom hook)? +- Is it shared state (context)? +- Does it need type safety (branded type)? + +### 3. Design Self-Validating Types + +For primitives with validation (Email, UserId, Port): + +**Option A: Zod Schemas (Recommended)** +```typescript +import { z } from 'zod' + +// Schema definition with validation +export const EmailSchema = z.string().email().min(1) +export const UserIdSchema = z.string().uuid() + +// Extract type from schema +export type Email = z.infer +export type UserId = z.infer + +// Validation function +export function validateEmail(value: unknown): Email { + return EmailSchema.parse(value) // Throws on invalid +} +``` + +**Option B: Branded Types (TypeScript)** +```typescript +// Brand for nominal typing +declare const __brand: unique symbol +type Brand = T & { [__brand]: TBrand } + +export type Email = Brand +export type UserId = Brand + +// Validating constructor +export function createEmail(value: string): Email { + if (!value.match(/^[^\s@]+@[^\s@]+\.[^\s@]+$/)) { + throw new Error('Invalid email format') + } + return value as Email +} + +export function createUserId(value: string): UserId { + if (!value || value.length === 0) { + throw new Error('UserId cannot be empty') + } + return value as UserId +} +``` + +**When to use which:** +- Zod: Form validation, API parsing, runtime validation +- Branded types: Type safety without runtime overhead + +### 4. Design Component Structure + +**Component Types:** + +**A. Presentational Components (Pure UI)** +- No state management +- Props-driven +- Reusable across features +- 100% testable + +```typescript +interface ButtonProps { + label: string + onClick: () => void + variant?: 'primary' | 'secondary' + disabled?: boolean +} + +export function Button({ label, onClick, variant = 'primary', disabled = false }: ButtonProps) { + return ( + + ) +} +``` + +**B. Container Components (Logic + State)** +- Manage state +- Handle side effects +- Coordinate data fetching +- Compose presentational components + +```typescript +export function LoginContainer() { + const { login, isLoading, error } = useAuth() + const [email, setEmail] = useState('') + const [password, setPassword] = useState('') + + const handleSubmit = async () => { + try { + const validEmail = EmailSchema.parse(email) + await login(validEmail, password) + } catch (error) { + // Handle error + } + } + + return ( + + ) +} +``` + +### 5. Design Custom Hooks + +Extract reusable logic into custom hooks: + +```typescript +// Single responsibility: Form state management +export function useFormState(initialValues: T) { + const [values, setValues] = useState(initialValues) + const [errors, setErrors] = useState>>({}) + + const setValue = (key: K, value: T[K]) => { + setValues(prev => ({ ...prev, [key]: value })) + setErrors(prev => ({ ...prev, [key]: undefined })) + } + + const reset = () => { + setValues(initialValues) + setErrors({}) + } + + return { values, errors, setValue, setErrors, reset } +} + +// Single responsibility: Data fetching +export function useUsers() { + const [users, setUsers] = useState([]) + const [isLoading, setIsLoading] = useState(false) + const [error, setError] = useState(null) + + useEffect(() => { + const fetchUsers = async () => { + setIsLoading(true) + try { + const data = await api.getUsers() + setUsers(data) + } catch (err) { + setError(err as Error) + } finally { + setIsLoading(false) + } + } + fetchUsers() + }, []) + + return { users, isLoading, error } +} +``` + +### 6. Design Context for Shared State + +When state is needed across 3+ component levels: + +```typescript +interface AuthContextValue { + user: User | null + login: (email: Email, password: string) => Promise + logout: () => Promise + isAuthenticated: boolean +} + +const AuthContext = createContext(null) + +export function AuthProvider({ children }: { children: ReactNode }) { + const [user, setUser] = useState(null) + + const login = async (email: Email, password: string) => { + const user = await api.login(email, password) + setUser(user) + } + + const logout = async () => { + await api.logout() + setUser(null) + } + + const value = useMemo( + () => ({ user, login, logout, isAuthenticated: !!user }), + [user] + ) + + return {children} +} + +export function useAuth() { + const context = useContext(AuthContext) + if (!context) { + throw new Error('useAuth must be used within AuthProvider') + } + return context +} +``` + +### 7. Plan Feature Structure + +**Feature-based structure (Recommended)**: +``` +src/features/auth/ +├── components/ +│ ├── LoginForm.tsx # Presentational +│ ├── LoginForm.test.tsx +│ ├── RegisterForm.tsx +│ └── RegisterForm.test.tsx +├── hooks/ +│ ├── useAuth.ts # Custom hook +│ ├── useAuth.test.ts +│ ├── useFormValidation.ts +│ └── useFormValidation.test.ts +├── context/ +│ ├── AuthContext.tsx # Shared state +│ └── AuthContext.test.tsx +├── types.ts # Email, UserId, etc. +├── api.ts # API calls +└── index.ts # Public exports +``` + +**Bad structure (Technical layers)**: +``` +src/ +├── components/LoginForm.tsx +├── hooks/useAuth.ts +├── contexts/AuthContext.tsx +└── types/auth.ts +``` + +### 8. Review Against Principles + +Check design against (see reference.md): +- [ ] No primitive obsession (use Zod/branded types) +- [ ] Feature-based architecture +- [ ] Component composition over prop drilling +- [ ] Custom hooks for reusable logic +- [ ] Context only when needed (3+ levels) +- [ ] Clear separation: presentational vs container +- [ ] Props interfaces well-defined + +## Output Format + +After design phase: + +``` +🎨 DESIGN PLAN + +Feature: User Authentication + +Core Domain Types: +✅ Email (Zod schema) - RFC 5322 validation, used in login/register +✅ UserId (branded type) - Non-empty string, prevents invalid IDs +✅ User (interface) - { id: UserId, email: Email, name: string } + +Components: +✅ LoginForm (Presentational) + Props: { email, password, onSubmit, isLoading, error } + Responsibility: UI only, no state + +✅ LoginContainer (Container) + Responsibility: State management, form handling, validation + Uses: LoginForm, useAuth hook + +✅ RegisterForm (Presentational) + Props: { formData, onSubmit, isLoading, errors } + Responsibility: UI only, no state + +Custom Hooks: +✅ useAuth + Returns: { user, login, logout, isAuthenticated, isLoading } + Responsibility: Auth operations and state + +✅ useFormValidation + Returns: { values, errors, setValue, validate, reset } + Responsibility: Form state and validation logic + +Context: +✅ AuthContext + Provides: { user, login, logout, isAuthenticated } + Used by: Protected routes, user menu, profile pages + Reason: Auth state needed across entire app + +Feature Structure: +📁 src/features/auth/ + ├── components/ + │ ├── LoginForm.tsx + │ ├── LoginForm.test.tsx + │ ├── RegisterForm.tsx + │ └── RegisterForm.test.tsx + ├── hooks/ + │ ├── useAuth.ts + │ ├── useAuth.test.ts + │ ├── useFormValidation.ts + │ └── useFormValidation.test.ts + ├── context/ + │ ├── AuthContext.tsx + │ └── AuthContext.test.tsx + ├── types.ts + ├── api.ts + └── index.ts + +Design Decisions: +- Email and UserId as validated types prevent runtime errors +- Zod for Email (form validation), branded type for UserId (type safety) +- LoginForm is presentational for reusability and testability +- useAuth hook encapsulates auth logic for reuse across components +- AuthContext provides auth state to avoid prop drilling +- Feature-based structure keeps all auth code together + +Integration Points: +- Consumed by: App routes, protected route wrapper, user menu +- Depends on: API client, token storage +- Events: User login/logout events for analytics + +Next Steps: +1. Create types with validation (Zod schemas + branded types) +2. Write tests for types and hooks (Jest + RTL) +3. Implement presentational components (LoginForm) +4. Implement container components (LoginContainer) +5. Add context provider (AuthContext) +6. Integration tests for full flows + +Ready to implement? Use @testing skill for test structure. +``` + +## Key Principles + +See reference.md for detailed principles: +- Primitive obsession prevention (Zod schemas, branded types) +- Component composition patterns +- Feature-based architecture +- Custom hooks for reusable logic +- Context for shared state (use sparingly) +- Props interfaces and type safety + +## Pre-Code Review Questions + +Before writing code, ask: +- Can logic be moved into custom hooks? +- Is this component presentational or container? +- Should state be local or context? +- Have I avoided primitive obsession? +- Is validation in the right place? +- Does this follow feature-based architecture? +- Are components small and focused? + +Only after satisfactory answers, proceed to implementation. + +See reference.md for complete design principles and examples. diff --git a/skills/component-designing/reference.md b/skills/component-designing/reference.md new file mode 100644 index 0000000..f305874 --- /dev/null +++ b/skills/component-designing/reference.md @@ -0,0 +1,811 @@ +# Component Designing Reference (TypeScript + React) + +## Overview + +This reference provides detailed principles for designing clean, maintainable React components and TypeScript types. Focus on composition, type safety, and feature-based architecture. + +## 1. Primitive Obsession Prevention + +### Problem + +Using primitive types (string, number, boolean) for domain concepts loses: +- Type safety +- Validation guarantees +- Domain meaning +- Refactoring safety + +**Bad example:** +```typescript +interface User { + id: string // What if empty? What if not UUID? + email: string // What if invalid format? + age: number // What if negative? What if 999? +} + +function getUser(id: string): User { + // No guarantee id is valid + return api.fetchUser(id) +} +``` + +### Solution A: Zod Schemas (Runtime Validation) + +**When to use**: Form validation, API parsing, user input + +```typescript +import { z } from 'zod' + +// Define schemas with validation +export const EmailSchema = z + .string() + .email('Invalid email format') + .min(1, 'Email is required') + +export const UserIdSchema = z + .string() + .uuid('UserId must be a valid UUID') + +export const AgeSchema = z + .number() + .int() + .min(0, 'Age cannot be negative') + .max(150, 'Age must be realistic') + +// Extract types from schemas +export type Email = z.infer +export type UserId = z.infer +export type Age = z.infer + +// User schema composition +export const UserSchema = z.object({ + id: UserIdSchema, + email: EmailSchema, + age: AgeSchema, + name: z.string().min(1) +}) + +export type User = z.infer + +// Validation functions +export function parseUser(data: unknown): User { + return UserSchema.parse(data) // Throws ZodError on invalid +} + +export function validateUser(data: unknown): { success: true; data: User } | { success: false; error: z.ZodError } { + return UserSchema.safeParse(data) +} +``` + +**Usage in components:** +```typescript +function UserForm() { + const [email, setEmail] = useState('') + const [error, setError] = useState('') + + const handleSubmit = () => { + try { + const validEmail = EmailSchema.parse(email) + // validEmail is now Email type, guaranteed valid + await api.createUser(validEmail) + } catch (err) { + if (err instanceof z.ZodError) { + setError(err.errors[0].message) + } + } + } +} +``` + +### Solution B: Branded Types (Compile-Time Safety) + +**When to use**: Type safety without runtime overhead, internal APIs + +```typescript +// Branding technique +declare const __brand: unique symbol +type Brand = T & { [__brand]: TBrand } + +// Define branded types +export type Email = Brand +export type UserId = Brand +export type Age = Brand + +// Validating constructors +export function createEmail(value: string): Email { + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/ + if (!emailRegex.test(value)) { + throw new Error(`Invalid email: ${value}`) + } + return value as Email +} + +export function createUserId(value: string): UserId { + if (!value || value.trim().length === 0) { + throw new Error('UserId cannot be empty') + } + return value as UserId +} + +export function createAge(value: number): Age { + if (value < 0 || value > 150) { + throw new Error(`Invalid age: ${value}`) + } + return value as Age +} + +// User interface +export interface User { + id: UserId + email: Email + age: Age + name: string +} +``` + +**Type safety:** +```typescript +// Compile-time error: Type 'string' is not assignable to type 'Email' +const email: Email = "test@example.com" // ❌ + +// Must use constructor +const email = createEmail("test@example.com") // ✅ + +// Cannot mix branded types +const userId: UserId = createEmail("test@example.com") // ❌ Compile error +``` + +### Which to Choose? + +| Feature | Zod Schemas | Branded Types | +|---------|-------------|---------------| +| Runtime validation | ✅ Yes | ❌ No | +| Compile-time safety | ✅ Yes | ✅ Yes | +| Bundle size impact | ⚠️ Adds Zod | ✅ None | +| Form validation | ✅ Excellent | ⚠️ Manual | +| API parsing | ✅ Excellent | ⚠️ Manual | +| Internal APIs | ✅ Good | ✅ Excellent | +| Error messages | ✅ Built-in | ⚠️ Manual | + +**Recommendation**: Use both! +- Zod for boundaries (forms, API) +- Branded types for internal type safety + +## 2. Feature-Based Architecture + +### Principle + +Group code by **feature** (what it does), not by **technical layer** (how it works). + +### Pattern Comparison + +**❌ Bad: Technical Layers (Horizontal Slicing)** +``` +src/ +├── components/ +│ ├── LoginForm.tsx +│ ├── RegisterForm.tsx +│ ├── UserProfile.tsx +│ └── UserList.tsx +├── hooks/ +│ ├── useAuth.ts +│ ├── useUsers.ts +│ └── useProfile.ts +├── contexts/ +│ ├── AuthContext.tsx +│ └── UserContext.tsx +├── types/ +│ ├── auth.ts +│ └── user.ts +└── api/ + ├── auth.ts + └── users.ts +``` + +**Problems:** +- Related code spread across directories +- Hard to find all code for a feature +- Changes require touching multiple folders +- Tight coupling between features +- Difficult to extract or delete features + +**✅ Good: Feature-Based (Vertical Slicing)** +``` +src/ +├── features/ +│ ├── auth/ +│ │ ├── components/ +│ │ │ ├── LoginForm.tsx +│ │ │ ├── LoginForm.test.tsx +│ │ │ ├── RegisterForm.tsx +│ │ │ └── RegisterForm.test.tsx +│ │ ├── hooks/ +│ │ │ ├── useAuth.ts +│ │ │ ├── useAuth.test.ts +│ │ │ ├── useLogin.ts +│ │ │ └── useRegister.ts +│ │ ├── context/ +│ │ │ ├── AuthContext.tsx +│ │ │ └── AuthContext.test.tsx +│ │ ├── types.ts +│ │ ├── api.ts +│ │ ├── utils.ts +│ │ └── index.ts # Public exports +│ │ +│ └── users/ +│ ├── components/ +│ │ ├── UserProfile.tsx +│ │ ├── UserProfile.test.tsx +│ │ ├── UserList.tsx +│ │ └── UserList.test.tsx +│ ├── hooks/ +│ │ ├── useUsers.ts +│ │ ├── useUserProfile.ts +│ │ └── useUserSearch.ts +│ ├── types.ts +│ ├── api.ts +│ └── index.ts +│ +└── shared/ # Truly shared code only + ├── components/ # Shared UI components (Button, Input) + ├── hooks/ # Shared hooks (useDebounce, useLocalStorage) + ├── utils/ # Shared utilities (date formatting, string helpers) + └── types/ # Shared types (ApiResponse, Pagination) +``` + +**Benefits:** +- All code for a feature in one place +- Easy to understand feature scope +- Easy to extract or delete features +- Clear dependencies between features +- Tests colocated with implementation + +### Feature Index Files + +Each feature should export its public API through `index.ts`: + +```typescript +// src/features/auth/index.ts +export { LoginForm, RegisterForm } from './components/LoginForm' +export { useAuth } from './hooks/useAuth' +export { AuthProvider, useAuthContext } from './context/AuthContext' +export type { User, Email, UserId } from './types' + +// Private implementation details stay unexported +// - Internal hooks +// - Utility functions +// - Implementation components +``` + +**Usage from other features:** +```typescript +// ✅ Good: Import from feature's public API +import { LoginForm, useAuth, type User } from '@/features/auth' + +// ❌ Bad: Import internal implementation +import { LoginForm } from '@/features/auth/components/LoginForm' +import { useAuth } from '@/features/auth/hooks/useAuth' +``` + +## 3. Component Composition Patterns + +### A. Presentational vs Container Components + +**Presentational Components (Pure UI)**: +- No state management (or only local UI state) +- Props-driven behavior +- Reusable across features +- Easy to test (just props) +- No side effects + +```typescript +// ✅ Good: Presentational +interface UserCardProps { + user: User + onEdit: (id: UserId) => void + onDelete: (id: UserId) => void +} + +export function UserCard({ user, onEdit, onDelete }: UserCardProps) { + return ( +
+

{user.name}

+

{user.email}

+ + +
+ ) +} + +// Test: Just pass props, assert rendering +test('UserCard renders user info', () => { + const user = createMockUser() + render() + expect(screen.getByText(user.name)).toBeInTheDocument() +}) +``` + +**Container Components (Logic + Orchestration)**: +- Manage state +- Handle side effects +- Data fetching +- Compose presentational components + +```typescript +// ✅ Good: Container +export function UserCardContainer({ userId }: { userId: UserId }) { + const { user, isLoading, error } = useUser(userId) + const { deleteUser } = useUsers() + const navigate = useNavigate() + + if (isLoading) return + if (error) return + if (!user) return + + const handleEdit = (id: UserId) => { + navigate(`/users/${id}/edit`) + } + + const handleDelete = async (id: UserId) => { + if (confirm('Delete user?')) { + await deleteUser(id) + } + } + + return +} +``` + +### B. Compound Components Pattern + +For components with multiple related parts: + +```typescript +// ✅ Good: Compound components with context +interface CardContextValue { + isExpanded: boolean + toggle: () => void +} + +const CardContext = createContext(null) + +function useCardContext() { + const context = useContext(CardContext) + if (!context) throw new Error('Must be used within Card') + return context +} + +// Main component +export function Card({ children }: { children: ReactNode }) { + const [isExpanded, setIsExpanded] = useState(false) + const toggle = () => setIsExpanded(prev => !prev) + + return ( + +
{children}
+
+ ) +} + +// Sub-components +Card.Header = function CardHeader({ children }: { children: ReactNode }) { + return
{children}
+} + +Card.Toggle = function CardToggle({ children }: { children: ReactNode }) { + const { toggle } = useCardContext() + return +} + +Card.Body = function CardBody({ children }: { children: ReactNode }) { + const { isExpanded } = useCardContext() + if (!isExpanded) return null + return
{children}
+} + +// Usage + + +

Title

+ Expand +
+ +

Content here

+
+
+``` + +### C. Render Props Pattern (Advanced) + +For maximum flexibility: + +```typescript +interface DataLoaderProps { + fetchData: () => Promise + children: (data: { + data: T | null + isLoading: boolean + error: Error | null + }) => ReactNode +} + +function DataLoader({ fetchData, children }: DataLoaderProps) { + const [data, setData] = useState(null) + const [isLoading, setIsLoading] = useState(false) + const [error, setError] = useState(null) + + useEffect(() => { + setIsLoading(true) + fetchData() + .then(setData) + .catch(setError) + .finally(() => setIsLoading(false)) + }, [fetchData]) + + return <>{children({ data, isLoading, error })} +} + +// Usage + api.getUsers()}> + {({ data, isLoading, error }) => { + if (isLoading) return + if (error) return + return + }} + +``` + +## 4. Custom Hooks Design + +### Single Responsibility Principle + +Each hook should do **one thing** well. + +**❌ Bad: God Hook** +```typescript +function useUser(userId: UserId) { + // Too many responsibilities! + const [user, setUser] = useState(null) + const [posts, setPosts] = useState([]) + const [friends, setFriends] = useState([]) + const [settings, setSettings] = useState(null) + const [isLoading, setIsLoading] = useState(false) + const [error, setError] = useState(null) + + useEffect(() => { + // Fetch everything... + }, [userId]) + + const updateProfile = async (data: Partial) => { /* ... */ } + const addPost = async (post: Post) => { /* ... */ } + const addFriend = async (friendId: UserId) => { /* ... */ } + const updateSettings = async (settings: Settings) => { /* ... */ } + + return { + user, posts, friends, settings, + isLoading, error, + updateProfile, addPost, addFriend, updateSettings + } +} +``` + +**✅ Good: Focused Hooks** +```typescript +// Fetch user data +function useUser(userId: UserId) { + const [user, setUser] = useState(null) + const [isLoading, setIsLoading] = useState(false) + const [error, setError] = useState(null) + + useEffect(() => { + const fetchUser = async () => { + setIsLoading(true) + try { + const data = await api.getUser(userId) + setUser(data) + } catch (err) { + setError(err as Error) + } finally { + setIsLoading(false) + } + } + fetchUser() + }, [userId]) + + return { user, isLoading, error } +} + +// Fetch user posts +function useUserPosts(userId: UserId) { + const [posts, setPosts] = useState([]) + const [isLoading, setIsLoading] = useState(false) + + useEffect(() => { + api.getUserPosts(userId).then(setPosts) + }, [userId]) + + return { posts, isLoading } +} + +// Update user profile +function useUpdateUser() { + const [isUpdating, setIsUpdating] = useState(false) + + const updateUser = async (userId: UserId, data: Partial) => { + setIsUpdating(true) + try { + await api.updateUser(userId, data) + } finally { + setIsUpdating(false) + } + } + + return { updateUser, isUpdating } +} + +// Compose in component +function UserProfile({ userId }: { userId: UserId }) { + const { user, isLoading, error } = useUser(userId) + const { posts } = useUserPosts(userId) + const { updateUser, isUpdating } = useUpdateUser() + + // ... +} +``` + +### Hook Naming Conventions + +- `use` prefix (React requirement) +- Verb-noun pattern: `useFormState`, `useUserData`, `useClickOutside` +- Boolean returns: `useIsAuthenticated`, `useHasPermission` + +## 5. Context Design + +### When to Use Context + +**Use context when:** +- State needed in 3+ component levels +- Truly global state (auth, theme, i18n) +- Avoiding prop drilling is worth complexity + +**Don't use context for:** +- State needed in 1-2 levels (use props) +- Frequently changing state (causes rerenders) +- Can be solved with component composition + +### Context Best Practices + +**1. Separate context and provider:** +```typescript +// Context value type +interface ThemeContextValue { + theme: 'light' | 'dark' + setTheme: (theme: 'light' | 'dark') => void +} + +// Context with null default +const ThemeContext = createContext(null) + +// Custom hook with error handling +export function useTheme() { + const context = useContext(ThemeContext) + if (!context) { + throw new Error('useTheme must be used within ThemeProvider') + } + return context +} + +// Provider component +export function ThemeProvider({ children }: { children: ReactNode }) { + const [theme, setTheme] = useState<'light' | 'dark'>('light') + + // Memoize value to prevent unnecessary rerenders + const value = useMemo(() => ({ theme, setTheme }), [theme]) + + return {children} +} +``` + +**2. Split contexts by concern:** +```typescript +// ❌ Bad: One context for everything +interface AppContextValue { + user: User | null + theme: 'light' | 'dark' + language: string + notifications: Notification[] + // ... everything else +} + +// ✅ Good: Separate contexts + + + + + + + + + +``` + +**3. Optimize performance:** +```typescript +// Split frequently and rarely changing state +interface UserContextValue { + user: User | null // Changes rarely +} + +interface UserActionsContextValue { + updateProfile: (data: Partial) => Promise // Never changes + logout: () => Promise // Never changes +} + +// Two contexts to prevent unnecessary rerenders +const UserContext = createContext(null) +const UserActionsContext = createContext(null) + +export function UserProvider({ children }: { children: ReactNode }) { + const [user, setUser] = useState(null) + + // Actions memoized once + const actions = useMemo( + () => ({ + updateProfile: async (data: Partial) => { + const updated = await api.updateProfile(data) + setUser(updated) + }, + logout: async () => { + await api.logout() + setUser(null) + } + }), + [] + ) + + return ( + + + {children} + + + ) +} + +// Components only rerender when they need to +export function UserName() { + const { user } = useUserContext() // Rerenders when user changes + return {user?.name} +} + +export function LogoutButton() { + const { logout } = useUserActions() // Never rerenders! + return +} +``` + +## 6. Props Interface Design + +### Props Naming + +**Boolean props:** +```typescript +// ✅ Good: Use is/has/can/should prefix +interface ButtonProps { + isLoading: boolean + isDisabled: boolean + hasIcon: boolean + canSubmit: boolean +} + +// ❌ Bad: No prefix +interface ButtonProps { + loading: boolean + disabled: boolean + icon: boolean +} +``` + +**Event handlers:** +```typescript +// ✅ Good: Use on prefix +interface FormProps { + onSubmit: (data: FormData) => void + onChange: (field: string, value: string) => void + onError: (error: Error) => void +} +``` + +### Props Destructuring + +**Always destructure in function signature:** +```typescript +// ✅ Good: Clear, self-documenting +export function Button({ label, onClick, isDisabled = false }: ButtonProps) { + return +} + +// ❌ Bad: Props object drilling +export function Button(props: ButtonProps) { + return +} +``` + +## 7. Type Safety Patterns + +### Discriminated Unions + +For state machines or variants: + +```typescript +// ✅ Good: Discriminated union +type AsyncState = + | { status: 'idle' } + | { status: 'loading' } + | { status: 'success'; data: T } + | { status: 'error'; error: Error } + +function DataComponent() { + const [state, setState] = useState>({ status: 'idle' }) + + // TypeScript narrows types based on status + if (state.status === 'loading') { + return + } + + if (state.status === 'error') { + // state.error is available (type narrowing) + return + } + + if (state.status === 'success') { + // state.data is available (type narrowing) + return + } + + return +} +``` + +### Utility Types + +```typescript +// Make all fields optional +type PartialUser = Partial + +// Pick specific fields +type UserPreview = Pick + +// Omit specific fields +type UserWithoutPassword = Omit + +// Make fields required +type RequiredUser = Required> + +// Make fields readonly +type ImmutableUser = Readonly +``` + +## Summary + +### Key Principles + +1. **Prevent Primitive Obsession**: Use Zod schemas and branded types +2. **Feature-Based Architecture**: Group by feature, not technical layer +3. **Component Composition**: Presentational vs container, compound components +4. **Single Responsibility Hooks**: Each hook does one thing +5. **Context Sparingly**: Only for 3+ levels, optimize for performance +6. **Type Safety**: Use discriminated unions, utility types, branded types +7. **Clear Props**: Descriptive names, always destructure +8. **Colocate Tests**: Tests next to implementation + +### Design Checklist + +Before writing code: +- [ ] Architecture pattern decided (feature-based) +- [ ] Primitives identified and types designed (Zod/branded) +- [ ] Component responsibilities clear (presentational vs container) +- [ ] Custom hooks extracted (single responsibility) +- [ ] Context usage justified (3+ levels, truly shared) +- [ ] Props interfaces defined (clear, typed) +- [ ] File structure planned (colocated tests) +- [ ] Integration points identified diff --git a/skills/documentation/SKILL.md b/skills/documentation/SKILL.md new file mode 100644 index 0000000..cdc0416 --- /dev/null +++ b/skills/documentation/SKILL.md @@ -0,0 +1,557 @@ +--- +name: documentation +description: Generate comprehensive feature documentation including Storybook stories, JSDoc comments, and feature guides. Use after completing a feature (may span multiple commits). Creates documentation for humans and AI to understand features, usage patterns, and design decisions. +--- + +# Documentation (TypeScript + React) + +Generate comprehensive documentation for features, components, and hooks. + +## When to Use +- After completing a feature (may span multiple commits) +- When a component/hook needs usage documentation +- When design decisions need recording +- For public/shared components and hooks + +## Documentation Types + +### 1. Storybook Stories (Component Documentation) +**Purpose**: Visual documentation of component usage and variants + +**Creates**: `.stories.tsx` files alongside components + +### 2. JSDoc Comments (Code Documentation) +**Purpose**: Inline documentation for types, props, complex functions + +**Location**: In source files (`.ts`, `.tsx`) + +### 3. Feature Documentation (Architectural Documentation) +**Purpose**: WHY decisions were made, HOW feature works, WHAT to extend + +**Creates**: `docs/features/[feature-name].md` + +## Workflow + +### 1. Identify Documentation Needs + +Ask: +- Is this a reusable component? → Storybook story +- Is this a custom hook? → JSDoc + usage example +- Is this a complete feature? → Feature documentation +- Are types/props complex? → JSDoc comments + +### 2. Create Storybook Stories + +**For each component**, create stories showing: +- Default state +- All variants/props +- Interactive states +- Edge cases (loading, error, empty) + +**Example**: +```typescript +// src/components/Button/Button.stories.tsx +import type { Meta, StoryObj } from '@storybook/react' +import { Button } from './Button' + +const meta: Meta = { + title: 'Components/Button', + component: Button, + parameters: { + layout: 'centered' + }, + tags: ['autodocs'], + argTypes: { + variant: { + control: 'select', + options: ['primary', 'secondary', 'danger'] + } + } +} + +export default meta +type Story = StoryObj + +// Default story +export const Primary: Story = { + args: { + label: 'Button', + variant: 'primary', + onClick: () => alert('clicked') + } +} + +// Variants +export const Secondary: Story = { + args: { + ...Primary.args, + variant: 'secondary' + } +} + +export const Danger: Story = { + args: { + ...Primary.args, + variant: 'danger' + } +} + +// States +export const Disabled: Story = { + args: { + ...Primary.args, + isDisabled: true + } +} + +export const Loading: Story = { + args: { + ...Primary.args, + isLoading: true + } +} + +// Interactive example +export const WithIcon: Story = { + args: { + ...Primary.args, + icon: + } +} +``` + +### 3. Add JSDoc Comments + +**For public types and interfaces**: +```typescript +/** + * Props for the Button component. + * + * @example + * ```tsx + * + * + * ) + * } + * ``` + */ +export function useFormValidation( + schema: ZodSchema, + initialValues: T, + onSubmit: (data: T) => Promise +): UseFormValidationReturn { + // Implementation +} +``` + +### Documenting Types + +```typescript +/** + * Branded type for user IDs. + * + * Prevents accidentally passing any string as a user ID. + * Must be created through `createUserId` validation function. + * + * @example + * ```typescript + * // ❌ Error: Type 'string' is not assignable to type 'UserId' + * const id: UserId = "some-id" + * + * // ✅ Must use constructor + * const id = createUserId("uuid-here") + * + * // ✅ Type-safe function parameters + * function getUser(id: UserId): User { + * // id is guaranteed to be validated + * } + * ``` + */ +export type UserId = Brand + +/** + * Creates a validated UserId. + * + * @param value - String to validate as user ID + * @returns Branded UserId type + * @throws {Error} If value is empty or invalid format + * + * @example + * ```typescript + * try { + * const id = createUserId("550e8400-e29b-41d4-a716-446655440000") + * // id is now UserId type + * } catch (error) { + * console.error("Invalid user ID") + * } + * ``` + */ +export function createUserId(value: string): UserId { + if (!value || !isValidUUID(value)) { + throw new Error(`Invalid user ID: ${value}`) + } + return value as UserId +} +``` + +## Storybook Templates + +### Basic Component Story + +```typescript +import type { Meta, StoryObj } from '@storybook/react' +import { Button } from './Button' + +const meta: Meta = { + title: 'Components/Button', + component: Button, + parameters: { + layout: 'centered', + docs: { + description: { + component: 'A versatile button component with multiple variants and states.' + } + } + }, + tags: ['autodocs'], + argTypes: { + variant: { + control: 'select', + options: ['primary', 'secondary', 'danger'], + description: 'Visual style variant' + }, + isDisabled: { + control: 'boolean', + description: 'Disables the button' + } + } +} + +export default meta +type Story = StoryObj + +export const Default: Story = { + args: { + label: 'Button', + variant: 'primary', + onClick: () => console.log('clicked') + } +} + +export const AllVariants: Story = { + render: () => ( +
+
+ ) +} +``` + +### Form Component Story + +```typescript +import type { Meta, StoryObj } from '@storybook/react' +import { userEvent, within, expect } from '@storybook/test' +import { LoginForm } from './LoginForm' + +const meta: Meta = { + title: 'Features/Auth/LoginForm', + component: LoginForm, + parameters: { + layout: 'centered' + } +} + +export default meta +type Story = StoryObj + +export const Default: Story = {} + +// Interactive story with testing +export const FilledForm: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement) + + // Fill form + await userEvent.type(canvas.getByLabelText(/email/i), 'test@example.com') + await userEvent.type(canvas.getByLabelText(/password/i), 'password123') + + // Click submit + await userEvent.click(canvas.getByRole('button', { name: /log in/i })) + + // Assert loading state appears + await expect(canvas.getByText(/logging in/i)).toBeInTheDocument() + } +} + +export const WithErrors: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement) + + // Submit without filling + await userEvent.click(canvas.getByRole('button', { name: /log in/i })) + + // Assert errors appear + await expect(canvas.getByText(/email is required/i)).toBeInTheDocument() + } +} +``` + +## Feature Documentation Template + +Complete template in SKILL.md. Key sections: + +### Executive Summary +```markdown +# Feature: [Name] + +## TL;DR +One-paragraph summary of what this feature does and why it matters. + +## Quick Start +```typescript +// Minimal example showing feature in action +``` + +``` + +### Problem & Solution +```markdown +## Problem +What pain point does this solve? Be specific. + +Example: "Users couldn't authenticate because..." + +## Solution +How does this feature solve it? High-level approach. + +Example: "Implemented OAuth2 flow with JWT tokens..." +``` + +### Architecture +```markdown +## Architecture + +### Component Tree +``` +AuthProvider +└── LoginContainer + ├── LoginForm (presentational) + ├── PasswordInput (presentational) + └── ErrorDisplay (presentational) +``` + +### Data Flow +```mermaid +graph LR + User[User Input] --> Form[LoginForm] + Form --> Hook[useAuth] + Hook --> API[Auth API] + API --> Context[AuthContext] + Context --> App[App State] +``` + +### File Structure +``` +src/features/auth/ +├── components/ +│ ├── LoginForm.tsx # Main form component +│ ├── LoginForm.test.tsx # Tests +│ └── LoginForm.stories.tsx # Storybook +├── hooks/ +│ ├── useAuth.ts # Auth logic +│ └── useAuth.test.ts # Hook tests +├── context/ +│ └── AuthContext.tsx # Shared auth state +├── types.ts # Email, UserId types +├── api.ts # API client +└── index.ts # Public exports +``` +``` + +### Design Decisions +```markdown +## Key Design Decisions + +### 1. Context for Auth State + +**Decision**: Use React Context for auth state instead of prop drilling + +**Rationale**: +- Auth state needed in 10+ components (nav, profile, settings, routes) +- Prop drilling through 4+ levels would be unmaintainable +- Context provides clean API and prevents coupling + +**Alternatives Considered**: +- Redux: Overkill for single feature state +- Zustand: Added dependency, context sufficient +- Prop drilling: Would couple many components + +**Trade-offs**: +- ✅ Gained: Clean API, decoupled components, easy testing +- ❌ Lost: Some component isolation, potential rerender issues +- ⚖️ Mitigation: Split context into state and actions to minimize rerenders +``` + +### Usage Examples +```markdown +## Usage + +### Basic Usage +```typescript +// Most common use case (90% of usage) +function ProtectedPage() { + const { user, logout } = useAuth() + + if (!user) return + + return ( +
+

Welcome {user.name}

+ +
+ ) +} +``` + +### Advanced Usage +```typescript +// Complex scenario or edge case +function AdminDashboard() { + const { user, isLoading, error, refreshToken } = useAuth() + + // Handle token refresh + useEffect(() => { + const interval = setInterval(refreshToken, 14 * 60 * 1000) // 14 min + return () => clearInterval(interval) + }, [refreshToken]) + + // ... rest of component +} +``` +``` + +## Documentation Checklist + +Before considering documentation complete: + +### Storybook Stories +- [ ] Story file created for each component +- [ ] Default story shows typical usage +- [ ] All prop variants documented +- [ ] Interactive states shown (loading, error, disabled) +- [ ] Accessibility checks pass (a11y addon) +- [ ] Controls configured for props +- [ ] Component description added + +### JSDoc Comments +- [ ] All public types documented +- [ ] All custom hooks documented +- [ ] Complex functions documented +- [ ] Examples included and working +- [ ] Parameters documented with types +- [ ] Return values documented + +### Feature Documentation +- [ ] Problem/solution described +- [ ] Architecture explained +- [ ] Design decisions documented (WHY) +- [ ] Usage examples provided (basic + advanced) +- [ ] API reference complete +- [ ] Testing strategy documented +- [ ] Accessibility features listed +- [ ] Troubleshooting guide included +- [ ] Related features linked + +## Documentation Maintenance + +### When Code Changes + +| Change | Update Needed | +|--------|--------------| +| New prop added | Update Storybook story, JSDoc, examples | +| Prop removed | Update all documentation, mark as breaking | +| New variant | Add Storybook story, update docs | +| API change | Update JSDoc, examples, feature docs | +| New hook | Create JSDoc, add examples | +| Refactor (no API change) | May update architecture docs | +| Bug fix | Update troubleshooting if relevant | +| Design decision changed | Update design decisions section | + +### Regular Reviews + +- **Quarterly**: Review all feature docs for accuracy +- **On major releases**: Update all examples to latest API +- **When onboarding**: Test docs with new team members + +## Tools and Automation + +### Storybook Addons + +```javascript +// .storybook/main.js +module.exports = { + addons: [ + '@storybook/addon-essentials', // Docs, controls, actions, etc. + '@storybook/addon-a11y', // Accessibility checks + '@storybook/addon-interactions', // Interactive testing + '@storybook/addon-links' // Navigate between stories + ] +} +``` + +### TypeDoc Configuration + +```json +// typedoc.json +{ + "entryPoints": ["src/index.ts"], + "out": "docs/api", + "exclude": ["**/*.test.ts", "**/*.stories.tsx"], + "excludePrivate": true, + "excludeProtected": true, + "readme": "README.md" +} +``` + +## Summary + +### Key Principles + +1. **Document WHY**: Decisions and trade-offs +2. **Show Code**: Working examples over prose +3. **Keep Updated**: Docs with code changes +4. **Test Examples**: Storybook stories compile and run +5. **Multiple Audiences**: Humans and AI both need context +6. **Focus on Usage**: Not implementation details +7. **Colocate**: Docs near code they document + +### Documentation Types + +- **JSDoc**: Inline code documentation +- **Storybook**: Visual component examples +- **Feature Docs**: Architecture and decisions + +### Quality Indicators + +Good documentation: +- Has working code examples +- Explains WHY, not just WHAT +- Shows common AND edge cases +- Is kept up to date +- Helps both debugging and extending + +Bad documentation: +- Out of date with code +- Only describes WHAT code does +- No examples or broken examples +- Implementation details instead of usage +- Written once, never updated diff --git a/skills/linter-driven-development/SKILL.md b/skills/linter-driven-development/SKILL.md new file mode 100644 index 0000000..bc1b58c --- /dev/null +++ b/skills/linter-driven-development/SKILL.md @@ -0,0 +1,226 @@ +--- +name: linter-driven-development +description: META ORCHESTRATOR for complete implementation workflow - design, test, lint, refactor, review, commit. Use for any code change that should result in a commit (features, bug fixes, refactors). Ensures clean code with tests, linting passes, and design validation. +--- + +# Linter-Driven Development Workflow (TypeScript + React) + +META ORCHESTRATOR for implementation workflow: design → test → lint → refactor → review → commit. +Use for any commit: features, bug fixes, refactors. + +## When to Use +- Implementing any code change that should result in a commit +- Need automatic workflow management with quality gates +- Want to ensure: clean code + tests + linting + design validation + accessibility + +## Workflow Phases + +### Phase 1: Design (if needed) +- If new components/types/major changes needed → invoke @component-designing skill +- Output: Component design plan with types, hooks, and structure + +### Phase 2: Implementation +- Follow @testing skill principles (Jest + React Testing Library) +- Write tests + implementation in parallel (not necessarily test-first) +- Aim for 100% coverage on new leaf components/hooks (pure logic with no external dependencies) + - Leaf types: Pure logic (can compose other leaf types), no API/DB/file system access + - Orchestrating types: Coordinate leaf types and external systems, need integration tests +- Test from user perspective (public API only) + +### Phase 3: Linter Loop +Run quality checks in this order: +1. **Type Check**: `npm run typecheck` (TypeScript compiler) +2. **Lint Check**: `npm run lintcheck` (ESLint validation) +3. **Format Check**: `npm run formatcheck` (Prettier validation) +4. **Style Check**: `npm run stylecheck` (Stylelint for SCSS) + +If any failures detected: +- Run auto-fixes: + - `npm run lint` (ESLint --fix) + - `npm run format` (Prettier --write) + - `npm run stylefix` (Stylelint --fix) +- Re-run quality checks +- If still failing (complexity, design issues): + - Interpret failures (cognitive complexity, cyclomatic complexity, etc.) + - Invoke @refactoring skill to fix (use storifying, extract functions/hooks, early returns) + - Re-run checks +- Repeat until all checks pass clean + +**Alternative**: If project has combined commands: +- Check: `npm run checkall` or `npm run check` +- Fix: `npm run fix` + +### Phase 4: Pre-Commit Design Review (ADVISORY) +- Invoke @pre-commit-review skill +- Review validates design principles (not code correctness) +- Includes accessibility checks (ARIA, semantic HTML, keyboard nav) +- Categorized findings: Design Debt / Readability Debt / Polish Opportunities +- If issues found in broader file context, flag for potential refactor +- **User decides**: commit as-is, apply fixes, or expand scope + +### Phase 5: Commit Ready +- Type checking passes ✅ +- ESLint passes ✅ +- Prettier passes ✅ +- Stylelint passes ✅ +- Tests pass with target coverage ✅ +- Design review complete (advisory) ✅ +- Present summary + commit message suggestion + +## Output Format + +``` +📋 COMMIT READINESS SUMMARY + +✅ Type Check: Passed (0 errors) +✅ ESLint: Passed (0 issues) +✅ Prettier: Passed (all files formatted) +✅ Stylelint: Passed (0 style issues) +✅ Tests: 92% coverage (3 leaf hooks at 100%, 1 orchestrating component, 18 test cases) +⚠️ Design Review: 3 findings (see below) + +🎯 COMMIT SCOPE +Modified: +- src/features/auth/LoginForm.tsx (+65, -20 lines) +- src/features/auth/useAuth.ts (+30, -5 lines) + +Added: +- src/features/auth/types.ts (new: UserId, Email types) +- src/features/auth/AuthContext.tsx (new context provider) + +Tests: +- src/features/auth/LoginForm.test.tsx (+95 lines) +- src/features/auth/useAuth.test.ts (new) +- src/features/auth/types.test.ts (new) + +⚠️ DESIGN REVIEW FINDINGS + +🔴 DESIGN DEBT (Recommended to fix): +- src/features/auth/LoginForm.tsx:45 - Primitive obsession detected + Current: function validateEmail(email: string): boolean + Better: Use Zod schema or branded Email type with validation + Why: Type safety, validation guarantee, prevents invalid emails + Fix: Use @component-designing to create self-validating Email type + +- src/features/auth/useAuth.ts:78 - Prop drilling detected + Auth state passed through 3+ component levels + Why: Tight coupling, hard to maintain + Fix: Extract AuthContext or use composition pattern + +🟡 READABILITY DEBT (Consider fixing): +- src/features/auth/LoginForm.tsx:120 - Mixed abstraction levels + Component mixes validation logic with UI rendering + Why: Harder to understand and test independently + Fix: Use @refactoring to extract custom hooks (useValidation) + +- src/features/auth/LoginForm.tsx:88 - Cognitive complexity: 18 (max: 15) + Nested conditionals for form validation + Why: Hard to understand logic flow + Fix: Use @refactoring to extract validation functions or use Zod + +🟢 POLISH OPPORTUNITIES: +- src/features/auth/types.ts:12 - Missing JSDoc comments + Public types should have documentation +- src/features/auth/LoginForm.tsx:45 - Consider semantic HTML + Use
with proper ARIA labels for better accessibility +- src/features/auth/useAuth.ts:34 - Missing error boundaries + Consider wrapping async operations with error handling + +📝 BROADER CONTEXT: +While reviewing LoginForm.tsx, noticed similar validation patterns in +RegisterForm.tsx and ProfileForm.tsx (src/features/user/). Consider +extracting a shared validation hook or creating branded types for common +fields (Email, Username, Password) used across the application. + +💡 SUGGESTED COMMIT MESSAGE +Add self-validating Email and UserId types to auth feature + +- Introduce Email type with RFC 5322 validation using Zod +- Introduce UserId branded type for type safety +- Refactor LoginForm to use validated types +- Extract useAuth hook for auth state management +- Add AuthContext to eliminate prop drilling +- Achieve 92% test coverage with React Testing Library + +Follows component composition principles and reduces primitive obsession. + +──────────────────────────────────────── + +Would you like to: +1. Commit as-is (ignore design findings) +2. Fix design debt only (🔴), then commit +3. Fix design + readability debt (🔴 + 🟡), then commit +4. Fix all findings (🔴 🟡 🟢), then commit +5. Refactor broader scope (address validation patterns across features), then commit +``` + +## Complexity Thresholds (SonarJS) + +These metrics trigger @refactoring when exceeded: +- **Cognitive Complexity**: max 15 +- **Cyclomatic Complexity**: max 10 +- **Expression Complexity**: max 5 +- **Function Length**: max 200 lines +- **File Length**: max 600 lines +- **Nesting Level**: max 4 + +## Workflow Control + +**Sequential Phases**: Each phase depends on previous phase completion +- Design must complete before implementation +- Implementation must complete before linting +- Linting must pass before review +- Review must complete before commit + +**Iterative Linting**: Phase 3 loops until clean +**Advisory Review**: Phase 4 never blocks, always asks user + +## Integration with Other Skills + +This orchestrator **invokes** other skills automatically: +- @component-designing (Phase 1, if needed) +- @testing (Phase 2, principles applied) +- @refactoring (Phase 3, when linter fails on complexity) +- @pre-commit-review (Phase 4, always) + +After committing, consider: +- If feature complete → invoke @documentation skill +- If more work needed → run this workflow again for next commit + +## Common Linter Failures and Resolutions + +### TypeScript Errors (npm run typecheck) +- Type mismatches → Fix types or add proper type guards +- Missing types → Add explicit types or interfaces +- Cannot fix automatically → Manual intervention required + +### ESLint Failures (npm run lintcheck) +**Auto-fixable**: +- Import sorting (simple-import-sort) +- Unused imports (unused-imports) +- Formatting issues covered by Prettier +- Simple style violations + +**Requires refactoring** (invoke @refactoring): +- Cognitive/cyclomatic complexity +- Max lines per function +- Expression complexity +- Nested control flow +- React hooks violations +- Component design issues + +### Prettier Failures (npm run formatcheck) +- Always auto-fixable with `npm run format` +- No manual intervention needed + +### Stylelint Failures (npm run stylecheck) +- Most auto-fixable with `npm run stylefix` +- Class naming violations may require manual fixes + +## Best Practices + +1. **Run checks frequently** during development +2. **Fix one complexity issue at a time** (don't batch refactoring) +3. **Trust the advisory review** (design debt causes future pain) +4. **Test after each refactoring** (ensure behavior unchanged) +5. **Commit frequently** (small, focused commits) diff --git a/skills/linter-driven-development/reference.md b/skills/linter-driven-development/reference.md new file mode 100644 index 0000000..0a18186 --- /dev/null +++ b/skills/linter-driven-development/reference.md @@ -0,0 +1,443 @@ +# Linter-Driven Development Reference (TypeScript + React) + +## Overview + +The linter-driven development workflow ensures code quality through automated tooling and design validation. This orchestrator manages the complete lifecycle from design to commit-ready code. + +## Quality Tool Stack + +### 1. TypeScript Compiler (`tsc`) +**Command**: `npm run typecheck` +**Purpose**: Type safety validation +**Can auto-fix**: No +**Failure resolution**: Manual type fixes or refactoring + +### 2. ESLint +**Command**: +- Check: `npm run lintcheck` +- Fix: `npm run lint` + +**Purpose**: Code quality, style, and complexity analysis +**Plugins used**: +- `eslint-plugin-sonarjs` - Complexity metrics (THE KEY PLUGIN) +- `typescript-eslint` - TypeScript-aware linting +- `eslint-plugin-react` - React best practices +- `eslint-plugin-react-hooks` - Hooks rules enforcement +- `eslint-plugin-jsx-a11y` - Accessibility rules +- `eslint-plugin-import` - Import/export management +- `eslint-plugin-unused-imports` - Remove dead code +- `eslint-plugin-simple-import-sort` - Auto-sort imports +- `eslint-plugin-promise` - Async/await best practices +- `eslint-plugin-security` - Security vulnerabilities + +**Can auto-fix**: Many rules (formatting, imports, simple violations) +**Requires refactoring**: Complexity rules, design issues + +### 3. Prettier +**Command**: +- Check: `npm run formatcheck` +- Fix: `npm run format` + +**Purpose**: Code formatting consistency +**Can auto-fix**: Always (100% auto-fixable) + +### 4. Stylelint +**Command**: +- Check: `npm run stylecheck` +- Fix: `npm run stylefix` + +**Purpose**: SCSS/CSS linting +**Can auto-fix**: Most rules + +## Workflow Phases in Detail + +### Phase 1: Design + +**Trigger**: New components, custom hooks, major architectural changes + +**Actions**: +1. Invoke @component-designing skill +2. Answer design questions: + - Component composition strategy? + - State management approach? + - Custom hooks needed? + - Type definitions required? +3. Receive design plan with: + - Component structure + - Props interfaces + - Custom hooks + - Type definitions + - File organization + +**Output**: Design document ready for implementation + +### Phase 2: Implementation + Testing + +**Testing principles** (from @testing skill): +- Write tests for public API only +- Use React Testing Library patterns +- Test user behavior, not implementation +- Use MSW for API mocking (real HTTP) +- Avoid `waitFor` with arbitrary delays +- Achieve 100% coverage on leaf components/hooks (no dependencies on other types) +- Integration tests for orchestrating components (test interactions and composition) + +**Implementation approach**: +- Parallel development (test + code together) +- Focus on behavior validation +- Use real implementations over mocks +- Follow component composition patterns +- Push business logic into leaf types for better testability + +### Phase 3: Linter Loop + +This is the core quality gate with multiple sub-checks: + +#### Step 1: Type Checking +```bash +npm run typecheck +``` +**Checks**: TypeScript compilation, type safety +**Failures**: Type errors, missing types, type mismatches +**Resolution**: +- Fix types manually +- Add type assertions where needed +- Use type guards for narrowing +- Cannot proceed if failing + +#### Step 2: ESLint Check +```bash +npm run lintcheck +``` +**Checks**: Code quality, complexity, React rules, hooks rules, a11y +**Failures**: See "ESLint Failure Categories" below +**Resolution**: +- Auto-fix: `npm run lint` +- If auto-fix insufficient → manual fixes or invoke @refactoring + +#### Step 3: Prettier Check +```bash +npm run formatcheck +``` +**Checks**: Code formatting consistency +**Failures**: Inconsistent formatting +**Resolution**: Always auto-fix with `npm run format` + +#### Step 4: Stylelint Check +```bash +npm run stylecheck +``` +**Checks**: SCSS/CSS quality and consistency +**Failures**: Style violations, naming issues +**Resolution**: Auto-fix with `npm run stylefix`, some manual fixes + +#### Loop Behavior +``` +Run all checks → Any fail? → Run auto-fixes → Re-run checks + ↓ + Still failing? + ↓ + Complexity/design issues? + ↓ + Invoke @refactoring + ↓ + Re-run checks + ↓ + Loop until pass +``` + +### Phase 4: Pre-Commit Review (Advisory) + +**Always invoked**, even if linter passes. + +**Purpose**: Validate design principles that linters cannot enforce + +**Scope**: +- **Primary**: All changed code in current commit +- **Secondary**: Broader file context (flags patterns for future refactoring) + +**Categories**: +- 🔴 **Design Debt**: Will cause pain when extending code + - Primitive obsession (string IDs, unvalidated inputs) + - Prop drilling (state passed through 3+ levels) + - Tight coupling + - Missing error boundaries + - Non-self-validating types + +- 🟡 **Readability Debt**: Hard to understand and work with + - Mixed abstraction levels + - Complex nested logic + - Inline styles or logic + - Poor naming + - Missing component extraction + +- 🟢 **Polish Opportunities**: Minor improvements + - Missing JSDoc + - Accessibility enhancements + - Type refinements + - Better naming + +**Output**: Advisory report with specific line references and fix suggestions + +**User Decision Points**: +1. Commit as-is (accept debt) +2. Fix design debt (🔴) - recommended +3. Fix design + readability (🔴 + 🟡) +4. Fix all (🔴 🟡 🟢) +5. Expand scope (refactor related code) + +### Phase 5: Commit Ready + +**Checklist**: +- ✅ Type checking passes +- ✅ ESLint passes +- ✅ Prettier passes +- ✅ Stylelint passes +- ✅ Tests pass (100% coverage on leaf types, integration tests on orchestrating components) +- ✅ Design review complete (advisory) + +**Output**: +- Commit readiness summary +- Suggested commit message +- List of modified/added files +- Coverage report +- Design review findings +- User decision prompt + +## Coverage Targets + +Follow @testing skill principles for coverage strategy: + +- **Leaf types** (pure logic, no external dependencies): 100% unit test coverage +- **Orchestrating types** (coordinate pieces, call external systems): Integration tests + +See **testing/reference.md** for: +- Detailed coverage targets explanation +- Examples of leaf vs orchestrating types +- Testing approach for each type +- Architectural benefits + +## ESLint Failure Categories + +### Category 1: Auto-Fixable (npm run lint) +These are fixed automatically: +- Unused imports (`unused-imports/no-unused-imports`) +- Import sorting (`simple-import-sort/imports`) +- Missing semicolons, quotes, spacing (handled by Prettier) +- Simple style violations +- Arrow function simplification (`arrow-body-style`) + +**Action**: Run `npm run lint` → Re-run checks → Continue + +### Category 2: Requires Manual Fix +These need developer intervention but are straightforward: +- `@typescript-eslint/no-explicit-any` - Replace any with proper types +- `no-console` - Remove or replace with proper logging +- `react/jsx-key` - Add key props to list items +- `react-hooks/exhaustive-deps` - Fix hook dependencies +- Type-related issues + +**Action**: Fix issues manually → Re-run checks → Continue + +### Category 3: Requires Refactoring (invoke @refactoring) +These indicate design or complexity problems: +- `sonarjs/cognitive-complexity` (max: 15) +- `sonarjs/cyclomatic-complexity` (max: 10) +- `sonarjs/expression-complexity` (max: 5) +- `sonarjs/max-lines-per-function` (max: 200) +- `sonarjs/max-lines` (max: 600) +- `sonarjs/nested-control-flow` (max: 4 levels) +- `react/no-unstable-nested-components` - Extract components +- `react/no-multi-comp` - Split into multiple files + +**Action**: Invoke @refactoring skill → Apply patterns (storifying, extract hooks/functions, early returns, simplify conditionals) → Re-run checks → Continue + +## Complexity Thresholds Explained + +### Cognitive Complexity (max: 15) +**What it measures**: How difficult is it to understand the code? +**Increments for**: Nested structures, breaks in linear flow, recursion +**Why it matters**: High cognitive load → more bugs, harder maintenance + +**How to fix**: Invoke @refactoring skill which applies **storifying** - making code read like a story at single abstraction level. See refactoring/reference.md for detailed techniques and examples. + +**Example violation**: +```tsx +// Cognitive complexity: 18 (too high!) +function validateUser(user: User): ValidationResult { + if (user) { // +1 + if (user.email) { // +2 (nested) + if (isValidEmail(user.email)) { // +3 (nested) + if (user.age >= 18) { // +4 (nested) + if (user.country === 'US') { // +5 (nested) + return { valid: true } + } else { // +1 + return { valid: false, reason: 'Not in US' } + } + } + } else { // +1 + return { valid: false, reason: 'Invalid email' } + } + } + } + return { valid: false, reason: 'Missing user' } +} +``` + +**How to fix**: Invoke @refactoring skill to storify with early returns (see refactoring/reference.md) + +### Cyclomatic Complexity (max: 10) +**What it measures**: Number of independent paths through code +**Increments for**: if, else, case, &&, ||, while, for, catch +**Why it matters**: More paths → more test cases needed, higher bug risk + +**Fix strategies**: Extract functions, use polymorphism, simplify conditionals + +### Expression Complexity (max: 5) +**What it measures**: Number of operators in a single expression +**Increments for**: &&, ||, ternary operators +**Why it matters**: Hard to read, error-prone + +**Example violation**: +```tsx +// Expression complexity: 6 (too high!) +const isValid = user && user.email && isValidEmail(user.email) && user.age >= 18 && user.country === 'US' && !user.banned +``` + +**Fix**: Extract to variables or validation function + +### Storifying Pattern + +When cognitive complexity is high, invoke @refactoring skill which applies storifying patterns (making code read like a story at single abstraction level). + +See **refactoring/reference.md** for: +- Detailed storifying explanation with examples +- TypeScript and React examples +- When and how to apply storifying +- Step-by-step storifying process + +## Integration with Other Skills + +### @component-designing +**When invoked**: Phase 1, if new components/major changes needed +**Input**: Feature requirements +**Output**: Component structure, props, hooks, types +**Next step**: Proceed to Phase 2 implementation + +### @testing +**When applied**: Phase 2, during implementation +**Input**: Component/hook to test +**Output**: Test files with React Testing Library +**Principles**: User-centric testing, real implementations + +### @refactoring +**When invoked**: Phase 3, when linter fails on complexity +**Input**: Failing component/function + linter error +**Output**: Refactored code that passes linter +**Patterns**: Storifying, extract hooks/functions, simplify logic, early returns, single abstraction levels + +### @pre-commit-review +**When invoked**: Phase 4, always (advisory) +**Input**: All changed code + file context +**Output**: Design review findings (🔴 🟡 🟢) +**Decision**: User chooses whether to apply fixes + +### @documentation +**When invoked**: After commit (feature complete) +**Input**: Implemented feature +**Output**: Storybook stories, JSDoc, feature docs +**Purpose**: Documentation for humans and AI + +## Command Alternatives + +Different projects may use different naming conventions: + +### Option 1: Separate check/fix commands +```bash +# Checks (validation only) +npm run typecheck +npm run lintcheck +npm run formatcheck +npm run stylecheck + +# Fixes (auto-fix where possible) +npm run lint +npm run format +npm run stylefix +``` + +### Option 2: Combined commands +```bash +# Check all quality gates +npm run checkall # or npm run check + +# Fix all auto-fixable issues +npm run fix +``` + +### Option 3: Task runner (if available) +```bash +# Using task runner (e.g., make, task) +task lint +task fix +``` + +**Plugin should detect** which commands are available from `package.json` scripts. + +## Best Practices + +### 1. Fail Fast, Fix Fast +- Run linter frequently during development +- Don't accumulate linter errors +- Fix auto-fixable issues immediately + +### 2. Trust Complexity Metrics +- SonarJS complexity rules are calibrated well +- If it flags complexity → there's real complexity +- Refactor rather than disable + +### 3. Respect Advisory Review +- Design debt compounds over time +- Fix 🔴 before committing when possible +- Track accepted debt in tickets + +### 4. Test After Refactoring +- Complexity fixes can introduce bugs +- Re-run tests after @refactoring +- Verify behavior unchanged + +### 5. Commit Granularly +- Small, focused commits +- Each commit passes all gates +- Easy to review and revert + +## Troubleshooting + +### "Type errors won't go away" +- TypeScript errors require manual fixes +- Consider if types are correct (not the code) +- Use type guards for narrowing +- Add type assertions as last resort + +### "ESLint keeps failing after auto-fix" +- Auto-fix only handles simple rules +- Complexity rules need refactoring +- Invoke @refactoring skill +- May need architectural changes + +### "Linter passes but review finds issues" +- Expected! Linters can't enforce design principles +- Review catches: primitive obsession, coupling, architecture +- User decides whether to fix + +### "Too many findings in review" +- Common for legacy code +- Fix incrementally (design debt first) +- Consider broader refactor ticket +- Don't let perfect be enemy of good + +## Related Files +- For design patterns: See @component-designing/reference.md +- For testing strategies: See @testing/reference.md +- For refactoring patterns: See @refactoring/reference.md +- For review principles: See @pre-commit-review/reference.md diff --git a/skills/pre-commit-review/SKILL.md b/skills/pre-commit-review/SKILL.md new file mode 100644 index 0000000..16e373d --- /dev/null +++ b/skills/pre-commit-review/SKILL.md @@ -0,0 +1,467 @@ +--- +name: pre-commit-review +description: ADVISORY validation of code against design principles, accessibility, and best practices that linters cannot fully enforce. Use after linter passes and tests pass to validate design quality. Categorizes findings as Design Debt, Readability Debt, or Polish Opportunities. Does NOT block commits. +--- + +# Pre-Commit Design Review (React/TypeScript) + +ADVISORY validation of code against design principles, accessibility, and practices that linters cannot fully enforce. +Categorizes findings as Design Debt, Readability Debt, or Polish Opportunities. + +## When to Use +- Automatically invoked by @linter-driven-development (Phase 4) +- Manually before committing (to validate design quality) +- After linter passes and tests pass + +## What This Reviews +- **NOT code correctness** (tests verify that) +- **NOT syntax/style** (ESLint/Prettier enforce that) +- **YES design principles** (primitive obsession, composition, architecture) +- **YES maintainability** (readability, complexity, testability) +- **YES accessibility** (semantic HTML, ARIA, keyboard nav) + +## Review Scope + +**Primary Scope**: Changed code in commit +- All modified lines +- All new components/hooks +- Specific focus on design principle adherence +- Accessibility compliance + +**Secondary Scope**: Context around changes +- Entire files containing modifications +- Flag patterns/issues outside commit scope +- Suggest broader refactoring opportunities + +## Finding Categories (Debt-Based) + +### 🔴 Design Debt +**Will cause pain when extending/modifying code** + +Violations: +- **Primitive obsession**: string IDs, unvalidated inputs, no branded types +- **Wrong architecture**: Technical layers instead of feature-based +- **Prop drilling**: State passed through 3+ component levels +- **Tight coupling**: Components tightly coupled to specific implementations +- **Missing error boundaries**: No error handling for async operations +- **No type validation**: Runtime data not validated (no Zod schemas) + +Impact: Future changes will require more work and introduce bugs + +### 🟡 Readability Debt +**Makes code harder to understand and work with** + +Violations: +- **Mixed abstractions**: Business logic mixed with UI in same component +- **Complex conditions**: Deeply nested or complex boolean expressions +- **Inline styles/logic**: Complex logic directly in JSX +- **Poor naming**: Generic names (data, handler, manager, utils) +- **God components**: Components doing too many things +- **Missing extraction**: Logic that should be custom hooks + +Impact: Team members (and AI) will struggle to understand intent + +### 🟢 Polish Opportunities +**Minor improvements for consistency and quality** + +Violations: +- **Missing JSDoc**: Complex types/hooks without documentation +- **Accessibility enhancements**: Could be more accessible (but not broken) +- **Type improvements**: Could use more specific types (vs any/unknown) +- **Better naming**: Non-idiomatic or unclear names +- **Performance**: Unnecessary rerenders, missing memoization +- **Bundle size**: Unused dependencies, large imports + +Impact: Low, but improves codebase quality + +## Review Workflow + +### 0. Architecture Pattern Validation (FIRST CHECK) + +**Expected: Feature-based architecture. Design Debt (ADVISORY) - never blocks commit.** + +Check file patterns: +- `src/features/[feature]/{components,hooks,context}/` → ✅ Feature-based +- `src/{components,hooks,contexts}/[feature].tsx` → 🔴 Technical layers (Design Debt) + +**Advisory Categories**: +1. **✅ Feature-based** → Praise, note migration progress if applicable +2. **🟢 Mixed without docs** → Suggest creating `docs/architecture/feature-based-migration.md` +3. **🔴 Technical layers (advisory)** → Suggest feature-based alternative, respect constraints + +**Report Template**: +``` +🔴 Design Debt (Advisory): Technical Layer Architecture +- Current: Code organized by technical type (components/, hooks/, etc.) +- Preferred: Feature-based structure for better cohesion/maintainability +- Alternative: Continue as-is (time constraints, team decision valid) +- Offer: Create migration docs? Refactor? Proceed as-is? +``` + +**Always acknowledge**: Time pressure, consistency needs, team decisions are valid reasons to proceed. + +--- + +### 1. Analyze Commit Scope +```bash +# Identify what changed +git diff --name-only + +# See actual changes +git diff +``` + +### 2. Review Design Principles + +Check for each principle in changed code: + +#### Primitive Obsession +**Look for**: +- String types for domain concepts (email, userId, etc.) +- Numbers without validation (age, price, quantity) +- Booleans representing state (use discriminated unions) + +**Example violation**: +```typescript +// 🔴 Design Debt +interface User { + id: string // What if empty? Not UUID? + email: string // What if invalid? +} + +// ✅ Better +type UserId = Brand +const EmailSchema = z.string().email() +``` + +#### Component Composition +**Look for**: +- Prop drilling (state passed through 3+ levels) +- Giant components (>200 lines) +- Mixed UI and business logic +- Inline complex logic in JSX + +**Example violation**: +```typescript +// 🔴 Design Debt: Prop drilling + + + + + + + + +// ✅ Better: Use context or composition + + + + + +``` + +#### Custom Hooks +**Look for**: +- Complex logic in components (should be in hooks) +- Duplicated logic across components +- useEffect with complex dependencies + +**Example violation**: +```typescript +// 🟡 Readability Debt: Logic in component +function UserProfile() { + const [user, setUser] = useState(null) + const [loading, setLoading] = useState(false) + useEffect(() => { + // 50 lines of fetch logic + }, []) + // More logic... +} + +// ✅ Better: Extract to hook +function useUser(id) { /* fetch logic */ } +function UserProfile() { + const { user, loading } = useUser(userId) + return +} +``` + +### 3. Review Accessibility + +**Check for each component** (jsx-a11y rules + manual review): + +#### Semantic HTML +- Using correct HTML elements ( +

Title

+

Subtitle

+``` + +#### ARIA Attributes +- Form inputs have labels +- Interactive elements have accessible names +- Images have alt text +- Dialogs have proper roles and labels + +**Example violations**: +```typescript +// 🔴 Design Debt: Missing label + + +// 🟢 Polish: Could improve alt text +image // Generic + +// ✅ Better + + + +John Doe's profile picture +``` + +#### Keyboard Navigation +- All interactive elements keyboard accessible +- Focus styles visible +- Logical tab order +- Escape closes modals + +**Example violations**: +```typescript +// 🔴 Design Debt: No keyboard support +
Action
+ +// ✅ Better + + +// Or if div required: +
e.key === 'Enter' && handleClick()} +> + Action +
+``` + +#### Color and Contrast +- Text readable (sufficient contrast) +- Not relying on color alone for meaning +- Focus indicators visible + +**Example violations**: +```typescript +// 🟡 Readability Debt: Color only indicates error + + +// ✅ Better: Visual + text indicator + +Email is invalid +``` + +#### Screen Reader Support +- Dynamic content announces updates (aria-live) +- Loading states communicated +- Error messages associated with fields + +**Example violations**: +```typescript +// 🟢 Polish: Loading not announced +{isLoading && } + +// ✅ Better +{isLoading && ( +
+ Loading user data... +
+)} +``` + +### 4. Review TypeScript Usage + +**Type safety**: +- Using `any` or `unknown` without validation +- Missing type definitions for props +- Not using branded types for domain concepts +- Not using Zod for runtime validation + +**Example violations**: +```typescript +// 🔴 Design Debt: Using any +function processData(data: any) { } + +// 🟡 Readability Debt: Inline type +function Button(props: { label: string; onClick: () => void }) { } + +// ✅ Better +const DataSchema = z.object({ /* ... */ }) +function processData(data: z.infer) { } + +interface ButtonProps { + label: string + onClick: () => void +} +function Button({ label, onClick }: ButtonProps) { } +``` + +### 5. Review Testing Implications + +**Testability**: +- Components too complex to test +- Logic not extracted to testable units +- Testing implementation details (bad) + +**Example violations**: +```typescript +// 🟡 Readability Debt: Hard to test +function ComplexForm() { + // 200 lines of intertwined logic and UI + // Would need to test implementation details +} + +// ✅ Better: Separated concerns +function useFormLogic() { /* testable hook */ } +function FormUI({ state, actions }) { /* testable UI */ } +``` + +### 6. Broader Context Review + +After reviewing changed code, scan entire modified files for: +- Similar violations elsewhere in file +- Patterns suggesting broader refactoring +- Opportunities for consistency improvements + +**Report format**: +``` +📝 BROADER CONTEXT: +While reviewing LoginForm.tsx, noticed similar validation patterns in +RegisterForm.tsx and ProfileForm.tsx (src/features/user/). Consider +extracting shared validation logic to a useFormValidation hook or +creating branded types for Email, Password used across features. +``` + +## Output Format + +After review: + +``` +⚠️ PRE-COMMIT REVIEW FINDINGS + +Reviewed: +- src/features/auth/LoginForm.tsx (+45, -20 lines) +- src/features/auth/types.ts (+15, -0 lines) +- src/features/auth/useAuth.ts (+30, -5 lines) + +🔴 DESIGN DEBT (2 findings) - Recommended to fix: + +1. src/features/auth/LoginForm.tsx:45 - Primitive obsession + Current: email validation with regex inline + Better: Use Zod schema or branded Email type + Why: Type safety, validation guarantee, reusable across features + Fix: Use @component-designing to create Email type + +2. src/features/auth/LoginForm.tsx:89 - Missing error boundary + Current: Async login can fail silently + Better: Wrap with ErrorBoundary or add error handling + Why: Better user experience, prevents broken UI + Fix: Add ErrorBoundary or try-catch with user feedback + +🟡 READABILITY DEBT (3 findings) - Consider fixing: + +1. src/features/auth/LoginForm.tsx:120 - Mixed abstractions + Component mixes validation logic with UI rendering + Why: Harder to understand and test independently + Fix: Extract validation to useFormValidation hook + +2. src/features/auth/LoginForm.tsx:67 - Complex condition + if (email && email.length > 0 && /regex/.test(email) && !isSubmitting && !error) + Why: Hard to understand intent + Fix: Extract to: const canSubmit = isFormValid(email, isSubmitting, error) + +3. src/features/auth/useAuth.ts:34 - Missing hook extraction + Complex useEffect with multiple concerns + Why: Hard to test, hard to reuse + Fix: Split into useLogin and useAuthState hooks + +🟢 POLISH OPPORTUNITIES (4 findings) - Optional improvements: + +1. src/features/auth/types.ts:10 - Missing JSDoc + Public Email type should have documentation + Suggestion: Add JSDoc explaining validation rules + +2. src/features/auth/LoginForm.tsx:12 - Accessibility enhancement + Form could use aria-describedby for better screen reader support + Current: + Better: + Impact: Better accessibility for screen reader users + +3. src/features/auth/LoginForm.tsx:55 - Keyboard navigation + Close button could have Escape key handler + Suggestion: Add onKeyDown handler for Escape key + +4. src/features/auth/useAuth.ts:89 - Type improvement + Return type could be more specific than { user: User | null } + Suggestion: Use discriminated union for different states + +📝 BROADER CONTEXT: +While reviewing LoginForm.tsx, noticed similar validation patterns in +RegisterForm.tsx (lines 45-67) and ProfileForm.tsx (lines 89-110). +Consider: +- Extract shared validation to useFormValidation hook +- Create branded Email and Password types used across auth feature +- Add error boundaries to all auth forms consistently + +──────────────────────────────────────── + +💡 RECOMMENDATION: +Fix design debt (🔴) before committing if possible. Design debt compounds +over time and makes future changes harder. Readability and Polish can be +addressed in follow-up commits. + +Would you like to: +1. Commit as-is (accept debt) +2. Fix design debt (🔴), then commit +3. Fix design + readability (🔴 + 🟡), then commit +4. Fix all findings (🔴 🟡 🟢), then commit +5. Refactor broader scope (address validation patterns across features) +``` + +## Key Principles + +See reference.md for detailed principles: +- Primitive obsession prevention +- Component composition over prop drilling +- Custom hooks for reusable logic +- Semantic HTML and ARIA +- Type safety with TypeScript and Zod +- Testability and separation of concerns +- Accessibility is not optional + +## After Review + +This is **ADVISORY** only. User decides: +- Accept debt knowingly +- Fix critical issues (design debt) +- Fix all findings +- Expand refactoring scope + +The review never blocks commits. It informs decisions. + +See reference.md for complete review checklist and examples. diff --git a/skills/pre-commit-review/reference.md b/skills/pre-commit-review/reference.md new file mode 100644 index 0000000..d701831 --- /dev/null +++ b/skills/pre-commit-review/reference.md @@ -0,0 +1,610 @@ +# Pre-Commit Review Reference (React/TypeScript) + +## Review Philosophy + +**Advisory, not blocking**: Inform decisions, don't prevent commits. + +**Debt-based categories**: Focus on future maintainability cost. + +**Context-aware**: Review changes plus broader file context. + +## Complete Review Checklist + +### 1. Architecture Patterns + +#### ✅ Feature-Based Structure +``` +src/features/auth/ +├── components/ # Auth UI components +├── hooks/ # Auth custom hooks +├── context/ # Auth context +├── types.ts # Auth types +└── index.ts # Public API +``` + +#### 🔴 Technical Layer Structure (Design Debt) +``` +src/ +├── components/auth.tsx +├── hooks/auth.ts +├── contexts/auth.tsx +└── types/auth.ts +``` + +**Impact**: Features spread across directories, hard to find all related code. + +### 2. Primitive Obsession + +#### String Primitives + +**🔴 Design Debt**: +```typescript +interface User { + id: string // Empty? Invalid format? + email: string // Validated? Format? + phone: string // Format? Country code? +} + +function getUser(id: string): User // Any string accepted +``` + +**✅ Better**: +```typescript +// Branded types +type UserId = Brand +type Email = Brand + +// Or Zod schemas +const UserIdSchema = z.string().uuid() +const EmailSchema = z.string().email() + +type UserId = z.infer +type Email = z.infer + +interface User { + id: UserId + email: Email + phone: PhoneNumber +} + +function getUser(id: UserId): User // Only valid IDs accepted +``` + +#### Number Primitives + +**🔴 Design Debt**: +```typescript +interface Product { + price: number // Negative? Too large? + quantity: number // Negative? Zero? + rating: number // Range? Decimal places? +} +``` + +**✅ Better**: +```typescript +const PriceSchema = z.number().positive().max(1000000) +const QuantitySchema = z.number().int().nonnegative() +const RatingSchema = z.number().min(0).max(5) + +type Price = z.infer +type Quantity = z.infer +type Rating = z.infer +``` + +#### Boolean State Machines + +**🟡 Readability Debt**: +```typescript +const [isLoading, setIsLoading] = useState(false) +const [isSuccess, setIsSuccess] = useState(false) +const [isError, setIsError] = useState(false) + +// Can have invalid states: isLoading && isSuccess +``` + +**✅ Better**: Discriminated union +```typescript +type State = + | { status: 'idle' } + | { status: 'loading' } + | { status: 'success'; data: T } + | { status: 'error'; error: Error } + +// Impossible states are impossible +``` + +### 3. Component Design + +#### Prop Drilling + +**🔴 Design Debt**: State passed through 3+ levels +```typescript + + + + + + + +``` + +**✅ Fix options**: +1. Context for truly global state +2. Composition to avoid passing props +3. Accept prop drilling for 1-2 levels (it's fine!) + +#### Mixed Concerns + +**🟡 Readability Debt**: UI + business logic mixed +```typescript +function UserProfile() { + // 50 lines of data fetching + // 30 lines of validation + // 40 lines of state management + // 80 lines of JSX + // Total: 200 lines, hard to test +} +``` + +**✅ Better**: Separated concerns +```typescript +// testable business logic +function useUserProfile(userId) { + // fetch, validate, manage state + return { user, isLoading, error, actions } +} + +// testable UI +function UserProfile({ userId }) { + const { user, isLoading, error, actions } = useUserProfile(userId) + + if (isLoading) return + if (error) return + return +} +``` + +#### God Components + +**🔴 Design Debt**: Component doing too much (>200 lines) + +**Signs**: +- Multiple state variables (5+) +- Many useEffect hooks +- Complex conditional rendering +- Mixed abstraction levels + +**Fix**: Extract components and hooks + +### 4. Hook Design + +#### Hook Extraction + +**🟡 Readability Debt**: Logic that should be a hook +```typescript +function Component() { + // 50 lines of reusable logic + // directly in component +} +``` + +**✅ Better**: +```typescript +function useFeature() { + // extracted, testable, reusable +} + +function Component() { + const feature = useFeature() + return +} +``` + +#### Hook Dependencies + +**🟡 Readability Debt**: Complex dependencies +```typescript +useEffect(() => { + fetchData(id, filters.category, filters.price, sort, page) +}, [id, filters, filters.category, filters.price, sort, page]) // Redundant, complex +``` + +**✅ Better**: +```typescript +const params = useMemo( + () => ({ id, category: filters.category, price: filters.price, sort, page }), + [id, filters.category, filters.price, sort, page] +) + +useEffect(() => { + fetchData(params) +}, [params]) + +// Or extract to custom hook +function useData(id, filters, sort, page) { + useEffect(() => { + fetchData(id, filters, sort, page) + }, [id, filters, sort, page]) +} +``` + +### 5. Accessibility Review + +#### Semantic HTML + +| ❌ Don't Use | ✅ Use Instead | Why | +|-------------|--------------|-----| +| `
` | ` +``` + +**If div required**: +```typescript +
{ + if (e.key === 'Enter' || e.key === ' ') { + handleClick() + } + }} +> + Click me +
+``` + +#### Images and Media + +**🔴 Design Debt**: Missing alt text +```typescript + +``` + +**🟢 Polish**: Generic alt text +```typescript +avatar +``` + +**✅ Better**: Descriptive alt text +```typescript +John Doe's profile picture +``` + +**Decorative images**: +```typescript + +``` + +#### Dynamic Content + +**🟢 Polish**: Loading without announcement +```typescript +{isLoading && } +``` + +**✅ Better**: Announced loading +```typescript +{isLoading && ( +
+ Loading user data... +
+)} +``` + +**Error announcements**: +```typescript +{error && ( +
+ {error.message} +
+)} +``` + +#### Modal Accessibility + +**🔴 Design Debt**: Basic modal +```typescript +{isOpen && ( +
+
+

Title

+

Content

+ +
+
+)} +``` + +**✅ Better**: Accessible modal +```typescript +{isOpen && ( +
e.key === 'Escape' && onClose()} + > +
+ +

Content

+ +
+
+)} +``` + +**With focus management**: +```typescript +function Modal({ isOpen, onClose, children }) { + const modalRef = useRef(null) + + useEffect(() => { + if (isOpen && modalRef.current) { + const previousActiveElement = document.activeElement + modalRef.current.focus() + + return () => { + previousActiveElement?.focus() + } + } + }, [isOpen]) + + // ... rest of modal +} +``` + +#### Color and Contrast + +**🟡 Readability Debt**: Color-only indicators +```typescript +Error +Success +``` + +**✅ Better**: Multiple indicators +```typescript + + +``` + +**With ARIA**: +```typescript + + +``` + +### 6. TypeScript Usage + +#### Type Safety + +**🔴 Design Debt**: Using `any` +```typescript +function processData(data: any) { + // No type safety +} +``` + +**✅ Better**: Proper types or `unknown` with validation +```typescript +const DataSchema = z.object({ + id: z.string(), + name: z.string() +}) + +function processData(data: unknown) { + const validated = DataSchema.parse(data) // Throws on invalid + // validated is now typed +} +``` + +#### Props Interfaces + +**🟡 Readability Debt**: Inline props type +```typescript +function Button(props: { + label: string + onClick: () => void + variant?: 'primary' | 'secondary' +}) { + // ... +} +``` + +**✅ Better**: Named interface +```typescript +interface ButtonProps { + label: string + onClick: () => void + variant?: 'primary' | 'secondary' +} + +function Button({ label, onClick, variant = 'primary' }: ButtonProps) { + // ... +} +``` + +#### Discriminated Unions + +**🟡 Readability Debt**: Multiple booleans for state +```typescript +interface State { + isLoading: boolean + isSuccess: boolean + isError: boolean + data?: Data + error?: Error +} +``` + +**✅ Better**: Discriminated union +```typescript +type State = + | { status: 'idle' } + | { status: 'loading' } + | { status: 'success'; data: Data } + | { status: 'error'; error: Error } + +// Type narrowing works automatically +if (state.status === 'success') { + state.data // Available, typed correctly +} +``` + +### 7. Testing Implications + +#### Testability + +**🔴 Design Debt**: Untestable component +```typescript +function Component() { + // 200 lines of tightly coupled logic + // Can't test without implementation details +} +``` + +**✅ Better**: Separated, testable +```typescript +// testable hook +function useLogic() { + return { state, actions } +} + +// testable component +function Component() { + const { state, actions } = useLogic() + return +} +``` + +### 8. Error Handling + +#### Missing Error Boundaries + +**🔴 Design Debt**: No error handling +```typescript +function AsyncComponent() { + const data = useAsyncData() // Can throw + return +} +``` + +**✅ Better**: Error boundary wrapper +```typescript +}> + + +``` + +**Or component-level handling**: +```typescript +function AsyncComponent() { + const { data, error, isLoading } = useAsyncData() + + if (isLoading) return + if (error) return + if (!data) return + + return +} +``` + +## Review Priority + +### Must Review (🔴 Design Debt) +1. Primitive obsession +2. Prop drilling (3+ levels) +3. Missing error boundaries +4. Non-semantic interactive elements +5. Missing form labels +6. Using `any` without validation + +### Should Review (🟡 Readability Debt) +1. Mixed abstractions +2. Complex conditions +3. God components (>200 lines) +4. Missing hook extraction +5. Inline complex logic + +### Nice to Review (🟢 Polish) +1. Missing JSDoc +2. Accessibility enhancements +3. Type improvements +4. Performance optimizations + +## Advisory Stance + +**Remember**: This is advisory, not blocking. + +**User decides**: +- Accept debt (with awareness) +- Fix critical (design debt) +- Fix all +- Expand scope + +**Always acknowledge**: +- Time constraints are real +- Team decisions are valid +- Consistency matters +- Sometimes "good enough" is right choice + +**Provide options, not mandates**. diff --git a/skills/refactoring/SKILL.md b/skills/refactoring/SKILL.md new file mode 100644 index 0000000..03103e4 --- /dev/null +++ b/skills/refactoring/SKILL.md @@ -0,0 +1,576 @@ +--- +name: refactoring +description: Linter-driven refactoring patterns to reduce complexity and improve code quality in React/TypeScript. Use when ESLint fails with SonarJS complexity issues (cognitive, cyclomatic, expression) or when code feels hard to read/maintain. Applies component extraction, hook extraction, and simplification patterns. +--- + +# Refactoring (React/TypeScript) + +Linter-driven refactoring patterns to reduce complexity and improve React code quality. + +## When to Use +- ESLint fails with SonarJS complexity issues +- Code feels hard to read or maintain +- Components/functions are too long or deeply nested +- Automatically invoked by @linter-driven-development when linter fails + +## Refactoring Signals + +### SonarJS Linter Failures +- **sonarjs/cognitive-complexity** (max: 15) → Simplify logic, extract functions/hooks +- **sonarjs/cyclomatic-complexity** (max: 10) → Reduce branches, early returns +- **sonarjs/expression-complexity** (max: 5) → Extract variables, simplify conditions +- **sonarjs/max-lines-per-function** (max: 200) → Extract components/hooks +- **sonarjs/max-lines** (max: 600) → Split file into multiple files +- **sonarjs/nested-control-flow** (max: 4) → Early returns, guard clauses + +### React-Specific Signals +- **react/no-unstable-nested-components** → Extract component definitions +- **react/no-multi-comp** → Split into separate files +- **react-hooks/exhaustive-deps** → Simplify dependencies, extract logic + +### Code Smells +- Components > 200 LOC +- Functions with > 4 levels of nesting +- Mixed abstraction levels (UI + business logic) +- Inline complex logic in JSX +- Deeply nested conditionals + +## Workflow + +### 1. Interpret Linter Output + +Run `npm run lintcheck` and analyze failures: +``` +src/features/auth/LoginForm.tsx:45:1: Cognitive Complexity of 18 exceeds max of 15 +src/features/users/UserList.tsx:120:5: Cyclomatic Complexity of 12 exceeds max of 10 +src/components/DataTable.tsx:89:1: Function has 250 lines, max is 200 +``` + +### 2. Diagnose Root Cause + +For each failure, ask: +- **Mixed abstractions?** → Extract custom hooks, extract components +- **Complex conditionals?** → Early returns, guard clauses, extract conditions +- **Primitive obsession?** → Create Zod schemas or branded types +- **Long component?** → Split into smaller components +- **Nested components?** → Extract to separate components +- **Complex JSX logic?** → Extract to helper functions or hooks + +### 3. Apply Refactoring Pattern + +Choose appropriate pattern: +- **Extract Custom Hook**: Move logic out of component +- **Extract Component**: Break down large components +- **Extract Helper Function**: Simplify complex logic +- **Early Returns/Guard Clauses**: Reduce nesting +- **Simplify Conditions**: Extract to variables, use early returns +- **Extract Validation**: Move to Zod schemas or validation functions + +### 4. Verify Improvement + +- Re-run linter: `npm run lintcheck` +- Tests still pass: `npm test` +- Code more readable? + +## Refactoring Patterns + +### Pattern 1: Extract Custom Hook (Business Logic) + +**Signal**: Component mixing UI with complex logic + +```typescript +// ❌ Before - Complex logic in component (Cognitive Complexity: 18) +function UserProfile({ userId }: { userId: string }) { + const [user, setUser] = useState(null) + const [isLoading, setIsLoading] = useState(false) + const [error, setError] = useState(null) + + useEffect(() => { + const fetchUser = async () => { + setIsLoading(true) + setError(null) + try { + const response = await fetch(`/api/users/${userId}`) + if (!response.ok) { + throw new Error('Failed to fetch user') + } + const data = await response.json() + setUser(data) + } catch (err) { + setError(err as Error) + } finally { + setIsLoading(false) + } + } + fetchUser() + }, [userId]) + + if (isLoading) return + if (error) return + if (!user) return + + return ( +
+

{user.name}

+

{user.email}

+
+ ) +} + +// ✅ After - Logic extracted to hook (Component Complexity: 4) +function useUser(userId: string) { + const [user, setUser] = useState(null) + const [isLoading, setIsLoading] = useState(false) + const [error, setError] = useState(null) + + useEffect(() => { + const fetchUser = async () => { + setIsLoading(true) + setError(null) + try { + const response = await fetch(`/api/users/${userId}`) + if (!response.ok) throw new Error('Failed to fetch user') + setUser(await response.json()) + } catch (err) { + setError(err as Error) + } finally { + setIsLoading(false) + } + } + fetchUser() + }, [userId]) + + return { user, isLoading, error } +} + +function UserProfile({ userId }: { userId: string }) { + const { user, isLoading, error } = useUser(userId) + + if (isLoading) return + if (error) return + if (!user) return + + return ( +
+

{user.name}

+

{user.email}

+
+ ) +} +``` + +### Pattern 2: Extract Component (Break Down Large Components) + +**Signal**: Component > 200 lines, doing too much + +```typescript +// ❌ Before - Large component (250 lines, Cognitive Complexity: 22) +function UserDashboard() { + const [users, setUsers] = useState([]) + const [selectedUser, setSelectedUser] = useState(null) + const [isEditing, setIsEditing] = useState(false) + const [searchTerm, setSearchTerm] = useState('') + + // ... 200+ lines of logic and JSX + + return ( +
+ {/* Search bar */} + setSearchTerm(e.target.value)} /> + + {/* User list */} +
    + {users.filter(u => u.name.includes(searchTerm)).map(user => ( +
  • setSelectedUser(user)}> + {user.name} - {user.email} + + +
  • + ))} +
+ + {/* User detail */} + {selectedUser && ( +
+ {isEditing ? ( + ... + ) : ( +
...
+ )} +
+ )} +
+ ) +} + +// ✅ After - Broken into focused components +function UserDashboard() { + const [selectedUser, setSelectedUser] = useState(null) + + return ( +
+ + + {selectedUser && } +
+ ) +} + +function UserSearch() { + const [searchTerm, setSearchTerm] = useState('') + // Search logic + return +} + +function UserList({ onSelectUser }: { onSelectUser: (user: User) => void }) { + const { users } = useUsers() + return ( +
    + {users.map(user => ( + + ))} +
+ ) +} + +function UserListItem({ user, onSelect }: UserListItemProps) { + return ( +
  • onSelect(user)}> + {user.name} + +
  • + ) +} +``` + +### Pattern 3: Early Returns / Guard Clauses (Reduce Nesting) + +**Signal**: Deeply nested conditionals, cyclomatic complexity high + +```typescript +// ❌ Before - Deep nesting (Cyclomatic Complexity: 12, Nesting: 5) +function validateAndSubmit(data: FormData) { + if (data) { + if (data.email) { + if (isValidEmail(data.email)) { + if (data.password) { + if (data.password.length >= 8) { + if (data.terms) { + return submitForm(data) + } else { + return { error: 'Must accept terms' } + } + } else { + return { error: 'Password too short' } + } + } else { + return { error: 'Password required' } + } + } else { + return { error: 'Invalid email' } + } + } else { + return { error: 'Email required' } + } + } + return { error: 'No data' } +} + +// ✅ After - Early returns (Cyclomatic Complexity: 7, Nesting: 1) +function validateAndSubmit(data: FormData) { + if (!data) return { error: 'No data' } + if (!data.email) return { error: 'Email required' } + if (!isValidEmail(data.email)) return { error: 'Invalid email' } + if (!data.password) return { error: 'Password required' } + if (data.password.length < 8) return { error: 'Password too short' } + if (!data.terms) return { error: 'Must accept terms' } + + return submitForm(data) +} + +// ✅ Even better - Use Zod schema +const FormDataSchema = z.object({ + email: z.string().email(), + password: z.string().min(8), + terms: z.boolean().refine(val => val === true, 'Must accept terms') +}) + +function validateAndSubmit(data: unknown) { + const result = FormDataSchema.safeParse(data) + if (!result.success) { + return { error: result.error.errors[0].message } + } + return submitForm(result.data) +} +``` + +### Pattern 4: Extract Complex Conditions (Simplify Expression Complexity) + +**Signal**: Complex boolean expressions, expression complexity > 5 + +```typescript +// ❌ Before - Complex condition (Expression Complexity: 8) +if ( + user && + user.isActive && + !user.isBanned && + user.subscription && + user.subscription.status === 'active' && + user.subscription.expiresAt > Date.now() && + (user.roles.includes('admin') || user.roles.includes('moderator')) +) { + // Allow access +} + +// ✅ After - Extracted to helper functions +function hasActiveSubscription(user: User): boolean { + return ( + user.subscription?.status === 'active' && + user.subscription.expiresAt > Date.now() + ) +} + +function hasModeratorAccess(user: User): boolean { + return user.roles.includes('admin') || user.roles.includes('moderator') +} + +function canAccessFeature(user: User): boolean { + return ( + user.isActive && + !user.isBanned && + hasActiveSubscription(user) && + hasModeratorAccess(user) + ) +} + +if (user && canAccessFeature(user)) { + // Allow access +} + +// ✅ Or extract to variables +const isUserValid = user.isActive && !user.isBanned +const hasSubscription = hasActiveSubscription(user) +const isModerator = hasModeratorAccess(user) + +if (user && isUserValid && hasSubscription && isModerator) { + // Allow access +} +``` + +### Pattern 5: Extract Unstable Nested Components + +**Signal**: react/no-unstable-nested-components + +```typescript +// ❌ Before - Component defined inside component +function UserList() { + const users = useUsers() + + // ❌ Recreated on every render + const UserCard = ({ user }: { user: User }) => ( +
    +

    {user.name}

    +

    {user.email}

    +
    + ) + + return ( +
    + {users.map(user => )} +
    + ) +} + +// ✅ After - Component extracted +function UserCard({ user }: { user: User }) { + return ( +
    +

    {user.name}

    +

    {user.email}

    +
    + ) +} + +function UserList() { + const users = useUsers() + return ( +
    + {users.map(user => )} +
    + ) +} +``` + +### Pattern 6: Simplify Hook Dependencies + +**Signal**: react-hooks/exhaustive-deps warnings, complex useEffect + +```typescript +// ❌ Before - Complex dependencies +function SearchResults({ initialQuery, filters, sortBy }: Props) { + const [results, setResults] = useState([]) + + useEffect(() => { + const fetchResults = async () => { + const response = await api.search({ + query: initialQuery, + filters: filters, + sort: sortBy, + page: 1 + }) + setResults(response.data) + } + fetchResults() + }, [initialQuery, filters, sortBy, filters.category, filters.price]) // ❌ Duplicates, object deps +} + +// ✅ After - Simplified with custom hook +function useSearchResults(query: string, filters: Filters, sortBy: string) { + const [results, setResults] = useState([]) + + // Stable object reference + const searchParams = useMemo( + () => ({ query, filters, sort: sortBy, page: 1 }), + [query, filters, sortBy] + ) + + useEffect(() => { + api.search(searchParams).then(response => setResults(response.data)) + }, [searchParams]) + + return results +} + +function SearchResults({ initialQuery, filters, sortBy }: Props) { + const results = useSearchResults(initialQuery, filters, sortBy) + return +} +``` + +### Pattern 7: Extract Form Validation Logic + +**Signal**: Complex validation in components + +```typescript +// ❌ Before - Validation scattered in component +function LoginForm() { + const [email, setEmail] = useState('') + const [password, setPassword] = useState('') + const [errors, setErrors] = useState({}) + + const handleSubmit = () => { + const newErrors = {} + + if (!email) { + newErrors.email = 'Email required' + } else if (!/\S+@\S+\.\S+/.test(email)) { + newErrors.email = 'Invalid email' + } + + if (!password) { + newErrors.password = 'Password required' + } else if (password.length < 8) { + newErrors.password = 'Password too short' + } + + if (Object.keys(newErrors).length > 0) { + setErrors(newErrors) + return + } + + submitLogin(email, password) + } + + // ...JSX +} + +// ✅ After - Validation with Zod +import { z } from 'zod' + +const LoginSchema = z.object({ + email: z.string().email('Invalid email'), + password: z.string().min(8, 'Password must be at least 8 characters') +}) + +function LoginForm() { + const { values, errors, setValue, handleSubmit } = useFormValidation( + LoginSchema, + { email: '', password: '' }, + submitLogin + ) + + return ( +
    + setValue('email', e.target.value)} + error={errors.email} + /> + setValue('password', e.target.value)} + error={errors.password} + /> + +
    + ) +} +``` + +## Refactoring Decision Tree + +When linter fails, follow this decision tree: + +``` +Linter Failure + ├─ Cognitive Complexity > 15 + │ ├─ Mixed abstractions? → Extract custom hooks + │ ├─ Complex conditions? → Extract to helper functions + │ └─ Deep nesting? → Early returns, guard clauses + │ + ├─ Cyclomatic Complexity > 10 + │ ├─ Many branches? → Early returns + │ ├─ Complex switch? → Use object mapping or extract functions + │ └─ Multiple &&/|| chains? → Extract conditions to variables + │ + ├─ Expression Complexity > 5 + │ ├─ Long boolean expressions? → Extract to variables + │ └─ Nested ternaries? → Extract to function or if statements + │ + ├─ Max Lines Per Function > 200 + │ ├─ Large component? → Extract smaller components + │ ├─ Complex logic? → Extract custom hooks + │ └─ Mixed concerns? → Separate UI from business logic + │ + ├─ Nested Control Flow > 4 + │ └─ Deep nesting? → Early returns, guard clauses + │ + └─ React-specific + ├─ no-unstable-nested-components → Extract component definition + ├─ no-multi-comp → Split into separate files + └─ exhaustive-deps → Simplify dependencies, extract logic +``` + +## Key Principles + +See reference.md for detailed principles: +- Single Responsibility: Each component/hook does one thing +- Extract Early, Extract Often: Don't wait for linter to fail +- Composition Over Complexity: Combine simple pieces +- Guard Clauses: Exit early, reduce nesting +- Extract Helper Functions: Name complex logic +- Custom Hooks: Reusable logic outside components +- Zod for Validation: Move validation out of components + +## After Refactoring + +- [ ] Re-run linter: `npm run lintcheck` +- [ ] Run tests: `npm test` +- [ ] Verify behavior unchanged +- [ ] Check if more readable +- [ ] Consider broader refactoring if patterns repeat + +See reference.md for complete refactoring patterns and decision trees. diff --git a/skills/refactoring/reference.md b/skills/refactoring/reference.md new file mode 100644 index 0000000..d3d917b --- /dev/null +++ b/skills/refactoring/reference.md @@ -0,0 +1,1210 @@ +# Refactoring Reference (React/TypeScript) + +## Linter-Driven Refactoring Philosophy + +Let complexity metrics guide refactoring decisions: +- **Cognitive Complexity**: How hard is it to understand? +- **Cyclomatic Complexity**: How many paths through the code? +- **Expression Complexity**: How many operators in one expression? +- **Nesting Level**: How deeply nested are control structures? + +When these exceed thresholds, code becomes harder to maintain, test, and debug. + +## SonarJS Complexity Metrics + +### Cognitive Complexity (max: 15) + +**What it measures**: Mental effort to understand code + +**Increments for**: +- Nested structures (+1 per level) +- Breaks in linear flow (if, switch, loops) +- Recursion +- Logical operators in conditions + +**Primary fix**: **Storifying** - See dedicated section below for detailed examples and techniques + +**Fix strategies**: +- Storify the code (make it read like a story at single abstraction level) +- Extract custom hooks +- Extract helper functions at same conceptual level +- Early returns +- Guard clauses + +### Cyclomatic Complexity (max: 10) + +**What it measures**: Number of independent paths through code + +**Increments for**: +- if, else, case +- &&, || +- while, for, do-while +- catch +- Ternary operators + +**Fix strategies**: +- Early returns +- Extract conditions to variables +- Replace complex switches with object mapping +- Simplify boolean logic + +### Expression Complexity (max: 5) + +**What it measures**: Number of operators in single expression + +**Increments for**: +- &&, || +- Ternary operators (? :) + +**Fix strategies**: +- Extract to intermediate variables +- Extract to helper functions +- Use early returns instead of complex conditions + +### Max Lines Per Function (max: 200) + +**What it measures**: Function/component size + +**Fix strategies**: +- Extract components +- Extract custom hooks +- Extract helper functions +- Split concerns (UI vs logic) + +### Nesting Level (max: 4) + +**What it measures**: Depth of nested control structures + +**Fix strategies**: +- Early returns +- Guard clauses +- Invert conditions +- Extract nested logic to functions + +## Storifying: Making Code Read Like a Story + +**Core Principle**: Each function should operate at a single conceptual level. Don't mix high-level business logic with low-level implementation details in the same function. + +**Why it matters**: +- Reduces cognitive complexity +- Makes code easier to understand at a glance +- Each abstraction level is testable independently +- Changes are isolated to appropriate levels + +This is the PRIMARY technique for reducing cognitive complexity. When the linter flags high cognitive complexity, storifying is usually the solution. + +### Bad Example - Mixed Abstraction Levels + +```tsx +function processUserRegistration(userData: FormData) { + // High-level step + const user = { + name: userData.get('name'), + // Low-level validation detail (wrong level!) + email: userData.get('email')?.toString().toLowerCase().trim() || '', + } + + // More low-level details mixed with high-level logic + if (!user.email.includes('@') || user.email.length < 5) { + throw new Error('Invalid email') + } + + // Database call (infrastructure detail) + const existingUser = db.query('SELECT * FROM users WHERE email = ?', [user.email]) + if (existingUser) { + throw new Error('User exists') + } + + // More database details + db.query('INSERT INTO users (name, email) VALUES (?, ?)', [user.name, user.email]) + + // Email sending details + const transporter = nodemailer.createTransport({...}) + transporter.sendMail({ + to: user.email, + subject: 'Welcome', + text: 'Welcome to our app!' + }) +} +``` + +**Problems**: +- Mixes 4 levels: parsing, validation, database, email +- Hard to test (requires mocking database and email) +- Hard to understand (what's the main flow?) +- Hard to change (modifications touch many concerns) + +### Good Example - Storified (Single Abstraction Level) + +```tsx +// Top-level function reads like a story - all at same conceptual level +function processUserRegistration(userData: FormData): void { + const user = parseUserData(userData) + validateUserDoesNotExist(user.email) + saveUser(user) + sendWelcomeEmail(user.email) +} + +// Each helper function handles one level of abstraction +function parseUserData(formData: FormData): User { + const rawEmail = formData.get('email')?.toString() || '' + return { + name: formData.get('name')?.toString() || '', + email: Email.parse(rawEmail), // Email is a branded type with validation + } +} + +function validateUserDoesNotExist(email: Email): void { + const exists = userRepository.existsByEmail(email) + if (exists) { + throw new UserAlreadyExistsError(email) + } +} + +function saveUser(user: User): void { + userRepository.save(user) +} + +function sendWelcomeEmail(email: Email): void { + emailService.send({ + to: email, + template: 'welcome', + }) +} +``` + +**Benefits**: +- Main function reads like a story: parse, validate, save, email +- Each step at same abstraction level +- Easy to test each function independently +- Easy to understand the flow +- Easy to change (each function isolated) + +### React Component Example - Mixed vs Storified + +**Bad - Mixed Levels**: +```tsx +function UserProfile({ userId }: Props) { + const [user, setUser] = useState(null) + const [loading, setLoading] = useState(true) + + useEffect(() => { + // Mixing: HTTP details + state management + error handling + fetch(`/api/users/${userId}`) + .then(res => { + if (!res.ok) throw new Error('Failed') + return res.json() + }) + .then(data => { + setUser(data) + setLoading(false) + }) + .catch(err => { + console.error(err) + setLoading(false) + }) + }, [userId]) + + // Mixing: loading logic + rendering + styling + if (loading) return
    Loading...
    + if (!user) return
    Not found
    + + return ( +
    +

    {user.name}

    +

    {user.email}

    +
    + ) +} +``` + +**Good - Storified**: +```tsx +// Component: High-level story of "display user profile" +function UserProfile({ userId }: Props) { + const { user, loading, error } = useUser(userId) + + return ( + + ) +} + +// Hook: Handles all data fetching concerns at one level +function useUser(userId: string) { + const [state, setState] = useState({ status: 'loading' }) + + useEffect(() => { + loadUser(userId).then( + user => setState({ status: 'success', user }), + error => setState({ status: 'error', error }) + ) + }, [userId]) + + return { + user: state.status === 'success' ? state.user : null, + loading: state.status === 'loading', + error: state.status === 'error' ? state.error : null, + } +} + +// API layer: Handles HTTP concerns at one level +async function loadUser(userId: string): Promise { + const response = await fetch(`/api/users/${userId}`) + if (!response.ok) { + throw new UserLoadError(userId) + } + return response.json() +} + +// View: Handles rendering concerns at one level +function UserProfileView({ user, loading, error }: ViewProps) { + if (loading) return + if (error) return + if (!user) return + + return ( + + + + + ) +} +``` + +### Storifying with Types/Classes + +Sometimes the best way to storify is to extract to a **new type or class** instead of functions. This creates an elegant API, organizes related logic, and makes testing trivial. + +#### When to Extract to Types (Not Just Functions) + +Extract to a type/class when: +- Function has many parameters (>3-4) +- Related data and behavior should be grouped together +- Type represents a domain concept (Order, User, Payment) +- Logic needs to be reusable and testable independently +- You're passing the same set of data through multiple functions + +#### Bad - Many Parameters + +```tsx +function processOrder( + orderId: string, + userId: string, + items: CartItem[], + shippingStreet: string, + shippingCity: string, + shippingZip: string, + paymentMethod: string, + cardNumber: string, + discountCode: string, + giftWrap: boolean +): Promise { + // Validate order ID + if (!orderId || orderId.length < 10) { + throw new Error('Invalid order ID') + } + + // Calculate totals + let total = 0 + for (const item of items) { + total += item.price * item.quantity + } + + // Apply discount + if (discountCode === 'SAVE10') { + total *= 0.9 + } + + // Add shipping + if (shippingCity === 'Remote') { + total += 15 + } else { + total += 5 + } + + // Process payment + // ... more logic + + return { success: true, total } +} + +// Nightmare to call +await processOrder( + 'ORD-123456', + 'USR-789', + cartItems, + '123 Main St', + 'Springfield', + '12345', + 'credit_card', + '4111-1111-1111-1111', + 'SAVE10', + true +) +``` + +**Problems**: +- 10 parameters (hard to remember order) +- Mixed abstraction levels +- Hard to test (need to mock everything) +- Hard to extend (adding parameter breaks all callers) +- No place for related behavior + +#### Good - Extract to Order Type + +```tsx +// Domain types with validation +type OrderId = string & { readonly __brand: 'OrderId' } +type UserId = string & { readonly __brand: 'UserId' } + +class Money { + constructor(private readonly cents: number) {} + + static dollars(amount: number): Money { + return new Money(Math.round(amount * 100)) + } + + add(other: Money): Money { + return new Money(this.cents + other.cents) + } + + multiply(factor: number): Money { + return new Money(Math.round(this.cents * factor)) + } + + toDollars(): number { + return this.cents / 100 + } +} + +class ShippingAddress { + constructor( + readonly street: string, + readonly city: string, + readonly zip: string + ) {} + + isRemoteLocation(): boolean { + return this.city === 'Remote' + } +} + +class Order { + constructor( + private readonly orderId: OrderId, + private readonly userId: UserId, + private readonly items: CartItem[], + private readonly shipping: ShippingAddress, + private readonly payment: PaymentMethod, + private discount?: DiscountCode, + private giftWrap: boolean = false + ) {} + + // All logic organized into methods + validate(): ValidationResult { + if (this.items.length === 0) { + return { valid: false, error: 'Cart is empty' } + } + return { valid: true } + } + + calculateSubtotal(): Money { + return this.items.reduce( + (sum, item) => sum.add(Money.dollars(item.price).multiply(item.quantity)), + Money.dollars(0) + ) + } + + calculateShipping(): Money { + return this.shipping.isRemoteLocation() + ? Money.dollars(15) + : Money.dollars(5) + } + + applyDiscount(code: DiscountCode): void { + this.discount = code + } + + calculateTotal(): Money { + let total = this.calculateSubtotal() + + if (this.discount) { + total = total.multiply(this.discount.factor) + } + + total = total.add(this.calculateShipping()) + + if (this.giftWrap) { + total = total.add(Money.dollars(5)) + } + + return total + } + + async process(): Promise { + const validation = this.validate() + if (!validation.valid) { + throw new OrderValidationError(validation.error) + } + + const total = this.calculateTotal() + await this.payment.charge(total) + await this.sendConfirmation() + + return { success: true, total: total.toDollars() } + } + + private async sendConfirmation(): Promise { + // Email logic + } +} + +// Usage - reads like a story, elegant API +const order = new Order( + orderId, + userId, + cartItems, + new ShippingAddress('123 Main St', 'Springfield', '12345'), + new CreditCardPayment('4111-1111-1111-1111'), + undefined, + true +) + +order.applyDiscount(DiscountCode.SAVE10) +await order.process() +``` + +**Benefits**: +- Fields instead of long parameter list +- Related logic organized into methods +- Each method does one thing (single abstraction level) +- Easy to test each method independently +- Natural place for domain behavior +- Type-safe, self-documenting + +#### Testing Benefits + +```tsx +// Testing a type is trivial - just instantiate and call methods +describe('Order', () => { + const createTestOrder = () => new Order( + OrderId.parse('ORD-123'), + UserId.parse('USR-789'), + [{ id: '1', price: 50, quantity: 2 }], + new ShippingAddress('123 Main', 'Springfield', '12345'), + new MockPayment() + ) + + it('calculates subtotal correctly', () => { + const order = createTestOrder() + expect(order.calculateSubtotal()).toEqual(Money.dollars(100)) + }) + + it('applies discount correctly', () => { + const order = createTestOrder() + order.applyDiscount(DiscountCode.SAVE10) + const total = order.calculateTotal() + // 100 * 0.9 + 5 shipping = 95 + expect(total.toDollars()).toBe(95) + }) + + it('charges remote shipping for remote locations', () => { + const order = new Order( + orderId, + userId, + items, + new ShippingAddress('123 Main', 'Remote', '12345'), // Remote city + payment + ) + expect(order.calculateShipping()).toEqual(Money.dollars(15)) + }) + + it('validates empty cart', () => { + const order = new Order(orderId, userId, [], shipping, payment) + expect(order.validate()).toEqual({ + valid: false, + error: 'Cart is empty' + }) + }) +}) + +// Compare to testing the function version - nightmare with 10 parameters! +``` + +#### React Hook Example with Type + +**Bad - Complex hook with scattered logic**: +```tsx +function useFormValidation(initialValues: Record) { + const [values, setValues] = useState(initialValues) + const [errors, setErrors] = useState>({}) + const [touched, setTouched] = useState>({}) + const [isSubmitting, setIsSubmitting] = useState(false) + + const validateEmail = (email: string) => { + return email.includes('@') && email.length > 5 + } + + const validatePassword = (password: string) => { + return password.length >= 8 + } + + const handleChange = (field: string, value: string) => { + setValues(prev => ({ ...prev, [field]: value })) + + // Validation logic mixed in + if (field === 'email' && !validateEmail(value)) { + setErrors(prev => ({ ...prev, email: 'Invalid email' })) + } else if (field === 'password' && !validatePassword(value)) { + setErrors(prev => ({ ...prev, password: 'Password too short' })) + } else { + setErrors(prev => { + const newErrors = { ...prev } + delete newErrors[field] + return newErrors + }) + } + } + + const handleBlur = (field: string) => { + setTouched(prev => ({ ...prev, [field]: true })) + } + + return { values, errors, touched, isSubmitting, handleChange, handleBlur } +} +``` + +**Good - Extract to FormState type**: +```tsx +class FormState { + constructor( + private values: Record, + private errors: Record = {}, + private touched: Record = {} + ) {} + + getValue(field: string): string { + return this.values[field] || '' + } + + getError(field: string): string | undefined { + return this.touched[field] ? this.errors[field] : undefined + } + + setValue(field: string, value: string): FormState { + const newValues = { ...this.values, [field]: value } + const validation = this.validateField(field, value) + + return new FormState( + newValues, + validation.valid + ? this.removeError(field) + : { ...this.errors, [field]: validation.error }, + this.touched + ) + } + + markTouched(field: string): FormState { + return new FormState( + this.values, + this.errors, + { ...this.touched, [field]: true } + ) + } + + private validateField(field: string, value: string): ValidationResult { + switch (field) { + case 'email': + return this.validateEmail(value) + case 'password': + return this.validatePassword(value) + default: + return { valid: true } + } + } + + private validateEmail(email: string): ValidationResult { + return email.includes('@') && email.length > 5 + ? { valid: true } + : { valid: false, error: 'Invalid email' } + } + + private validatePassword(password: string): ValidationResult { + return password.length >= 8 + ? { valid: true } + : { valid: false, error: 'Password must be at least 8 characters' } + } + + private removeError(field: string): Record { + const { [field]: _, ...rest } = this.errors + return rest + } + + isValid(): boolean { + return Object.keys(this.errors).length === 0 + } +} + +// Hook is now trivial - just wraps the type +function useFormState(initialValues: Record) { + const [state, setState] = useState(() => new FormState(initialValues)) + + const handleChange = (field: string, value: string) => { + setState(prev => prev.setValue(field, value)) + } + + const handleBlur = (field: string) => { + setState(prev => prev.markTouched(field)) + } + + return { state, handleChange, handleBlur } +} + +// Usage in component - clean and elegant +function LoginForm() { + const { state, handleChange, handleBlur } = useFormState({ + email: '', + password: '' + }) + + return ( +
    + handleChange('email', e.target.value)} + onBlur={() => handleBlur('email')} + /> +
    + ) +} + +// Testing the FormState type is trivial +describe('FormState', () => { + it('validates email correctly', () => { + let state = new FormState({ email: 'invalid' }) + state = state.setValue('email', 'invalid') + state = state.markTouched('email') + expect(state.getError('email')).toBe('Invalid email') + }) +}) +``` + +### Key Storifying Techniques + +1. **Extract functions** - One responsibility per function +2. **Extract to types/classes** - Group related data and behavior, create elegant APIs +3. **Use custom hooks** - Separate stateful logic from presentation +4. **Create layers** - Component → Hook → Service → API +5. **Name descriptively** - Function names tell the story +6. **Early returns** - Reduce nesting, clarify flow +7. **Branded types** - Push validation to type level (Email, UserId) + +### When to Apply Storifying + +Apply storifying when: +- Linter reports cognitive complexity > 15 +- Function mixes different levels of abstraction +- Hard to summarize what function does in one sentence +- Testing requires many mocks +- Making changes touches unrelated concerns + +### Storifying Process + +1. **Read the function** - What's the high-level story? +2. **Identify abstraction levels** - What's high-level? What's low-level? +3. **Extract low-level details** - Create helper functions with descriptive names +4. **Rewrite main function** - Use only extracted helpers +5. **Verify readability** - Main function should read like a story +6. **Test each piece** - Each extracted function is now testable + +## React-Specific Refactoring Patterns + +### 1. Extract Custom Hook + +**When**: Component mixing UI with business logic + +```typescript +// Pattern +function useFeature(input: Input): Output { + const [state, setState] = useState(initialState) + + // Complex logic here + useEffect(() => { + // Side effects + }, [dependencies]) + + return { state, actions } +} + +function Component() { + const { state, actions } = useFeature(input) + return +} +``` + +**Benefits**: +- Testable in isolation +- Reusable across components +- Clearer component responsibility +- Reduced component complexity + +### 2. Extract Component + +**When**: Large component (>200 lines) or doing multiple things + +```typescript +// Pattern: Break down by responsibility +function FeatureComponent() { + return ( + + + + + + ) +} + +// Each sub-component focuses on one thing +function FeatureHeader() { /* ... */ } +function FeatureContent() { /* ... */ } +function FeatureActions() { /* ... */ } +``` + +**Benefits**: +- Smaller, focused components +- Easier to test +- Reusable pieces +- Clear separation of concerns + +### 3. Extract Helper Function + +**When**: Complex logic inline in component/hook + +```typescript +// Pattern +function determineUserAccess(user: User, resource: Resource): AccessLevel { + // Complex logic extracted + if (!user.isActive) return 'none' + if (user.roles.includes('admin')) return 'full' + if (isResourceOwner(user, resource)) return 'owner' + return 'read' +} + +function Component() { + const access = determineUserAccess(user, resource) + if (access === 'none') return + return +} +``` + +**Benefits**: +- Named, understandable logic +- Testable independently +- Reusable +- Reduces component complexity + +### 4. Extract Validation (Zod) + +**When**: Complex validation logic in components + +```typescript +// Pattern +import { z } from 'zod' + +const UserSchema = z.object({ + email: z.string().email(), + age: z.number().min(18).max(150), + country: z.enum(['US', 'UK', 'CA']) +}) + +type User = z.infer + +// Validation separated from component logic +function validateUser(data: unknown): User { + return UserSchema.parse(data) +} +``` + +**Benefits**: +- Declarative validation +- Runtime type safety +- Reusable schemas +- Clear error messages + +### 5. Compound Components + +**When**: Component has multiple related parts + +```typescript +// Pattern +function Card({ children }: { children: ReactNode }) { + return
    {children}
    +} + +Card.Header = function CardHeader({ children }) { + return
    {children}
    +} + +Card.Body = function CardBody({ children }) { + return
    {children}
    +} + +Card.Footer = function CardFooter({ children }) { + return
    {children}
    +} + +// Usage + + Title + Content + Actions + +``` + +**Benefits**: +- Flexible composition +- Related components grouped +- Clear API +- Prevents prop drilling + +## Common Refactoring Scenarios + +### Scenario 1: Form Component Too Complex + +**Problem**: Large form component with validation + +**Solution**: +```typescript +// Before: Everything in one component (300+ lines) +function RegistrationForm() { + // validation, state, submission all mixed +} + +// After: Extracted to multiple concerns +// 1. Validation schema +const RegistrationSchema = z.object({ + email: z.string().email(), + password: z.string().min(8), + confirmPassword: z.string() +}).refine( + (data) => data.password === data.confirmPassword, + { message: "Passwords don't match" } +) + +// 2. Custom hook for form logic +function useRegistrationForm() { + const { values, errors, setValue, handleSubmit } = useFormValidation( + RegistrationSchema, + { email: '', password: '', confirmPassword: '' }, + submitRegistration + ) + return { values, errors, setValue, handleSubmit } +} + +// 3. Small presentational component +function RegistrationForm() { + const { values, errors, setValue, handleSubmit } = useRegistrationForm() + + return ( +
    + setValue('email', v)} error={errors.email} /> + setValue('password', v)} error={errors.password} /> + setValue('confirmPassword', v)} error={errors.confirmPassword} /> + Register + + ) +} +``` + +### Scenario 2: Data Fetching Component + +**Problem**: Component with complex data fetching logic + +**Solution**: +```typescript +// Before: All in component +function UserProfile({ userId }: { userId: string }) { + const [user, setUser] = useState(null) + const [posts, setPosts] = useState([]) + const [isLoading, setIsLoading] = useState(false) + // ... 100+ lines of fetch logic +} + +// After: Extracted to custom hooks +function useUser(userId: string) { + const [user, setUser] = useState(null) + const [isLoading, setIsLoading] = useState(false) + const [error, setError] = useState(null) + + useEffect(() => { + fetch(`/api/users/${userId}`) + .then(res => res.json()) + .then(setUser) + .catch(setError) + .finally(() => setIsLoading(false)) + }, [userId]) + + return { user, isLoading, error } +} + +function useUserPosts(userId: string) { + const [posts, setPosts] = useState([]) + const [isLoading, setIsLoading] = useState(false) + + useEffect(() => { + fetch(`/api/users/${userId}/posts`) + .then(res => res.json()) + .then(setPosts) + .finally(() => setIsLoading(false)) + }, [userId]) + + return { posts, isLoading } +} + +// Component simplified +function UserProfile({ userId }: { userId: string }) { + const { user, isLoading: userLoading, error } = useUser(userId) + const { posts, isLoading: postsLoading } = useUserPosts(userId) + + if (userLoading) return + if (error) return + if (!user) return + + return ( +
    + + {postsLoading ? : } +
    + ) +} +``` + +### Scenario 3: Complex Conditional Rendering + +**Problem**: Many nested conditionals in JSX + +**Solution**: +```typescript +// Before: Nested ternaries (Expression Complexity: 8) +
    + {user ? ( + user.isPremium ? ( + user.hasActiveSubscription ? ( + + ) : ( + + ) + ) : ( + + ) + ) : ( + + )} +
    + +// After: Early returns in component +function ContentDisplay({ user }: { user: User | null }) { + if (!user) return + if (!user.isPremium) return + if (!user.hasActiveSubscription) return + return +} + +// Or: Extract to helper +function getContentComponent(user: User | null): ComponentType { + if (!user) return LoginPrompt + if (!user.isPremium) return FreeContent + if (!user.hasActiveSubscription) return SubscriptionExpired + return PremiumContent +} + +function ContentDisplay({ user }: { user: User | null }) { + const ContentComponent = getContentComponent(user) + return +} +``` + +### Scenario 4: Switch Statement for Component Selection + +**Problem**: Long switch statement for rendering different components + +**Solution**: +```typescript +// Before: Long switch (Cyclomatic Complexity: 12) +function NotificationDisplay({ type }: { type: string }) { + switch (type) { + case 'success': return + case 'error': return + case 'warning': return + case 'info': return + // ... 10 more cases + default: return null + } +} + +// After: Object mapping (Cyclomatic Complexity: 1) +const NOTIFICATION_COMPONENTS: Record = { + success: SuccessNotification, + error: ErrorNotification, + warning: WarningNotification, + info: InfoNotification, + // ... more mappings +} + +function NotificationDisplay({ type }: { type: NotificationType }) { + const Component = NOTIFICATION_COMPONENTS[type] + return Component ? : null +} +``` + +### Scenario 5: useEffect with Complex Dependencies + +**Problem**: useEffect with many dependencies, hard to track + +**Solution**: +```typescript +// Before: Complex dependencies +useEffect(() => { + fetchData(userId, filters.category, filters.price, sortBy, page) +}, [userId, filters, filters.category, filters.price, sortBy, page]) + +// After: Stable object with useMemo +const queryParams = useMemo( + () => ({ + userId, + category: filters.category, + price: filters.price, + sortBy, + page + }), + [userId, filters.category, filters.price, sortBy, page] +) + +useEffect(() => { + fetchData(queryParams) +}, [queryParams]) + +// Or: Extract to custom hook +function useDataFetch(userId: string, filters: Filters, sortBy: string, page: number) { + const [data, setData] = useState([]) + + useEffect(() => { + fetchData(userId, filters, sortBy, page).then(setData) + }, [userId, filters, sortBy, page]) + + return data +} +``` + +## Refactoring Checklist + +Before refactoring: +- [ ] Tests exist and pass +- [ ] Understand current behavior +- [ ] Identify root cause of complexity + +During refactoring: +- [ ] One pattern at a time +- [ ] Run tests after each change +- [ ] Keep commits small and focused +- [ ] Ensure linter passes + +After refactoring: +- [ ] All tests pass +- [ ] Linter passes +- [ ] Code more readable +- [ ] Complexity reduced +- [ ] Behavior unchanged + +## Anti-Patterns to Avoid + +### ❌ Premature Optimization + +Don't extract everything immediately. Wait for: +- Linter failures +- Code feeling complex +- Need for reuse + +### ❌ Over-Extraction + +Don't create hooks/components for single-use, simple logic: +```typescript +// ❌ Over-extracted +function useButtonClick(onClick: () => void) { + return () => onClick() +} + +// ✅ Just use directly +
    }> + + + ) + + expect(screen.getByText(/something went wrong/i)).toBeInTheDocument() + + consoleSpy.mockRestore() +}) +``` + +## Coverage Targets + +The architecture principle: **Push logic into leaf types for maximum testability**. Strive to have most of your business logic in small, focused, dependency-free leaf components/hooks/functions. + +### Leaf Components/Hooks/Functions + +**Definition**: Pure, self-contained units with **no external dependencies** (no API calls, database access, file system). They contain core business logic and can freely compose other leaf types. + +**Key Characteristics**: +- ✅ **Can depend on other leaf types** - Domain types composing domain types (e.g., `Order` uses `Money`, `ShippingAddress`) +- ❌ **Cannot depend on external systems** - No API calls, database access, file system operations +- ✅ **Deterministic** - Same input always gives same output +- ✅ **No side effects** - Pure functions or immutable objects +- ✅ **Testable without mocks** - Just instantiate and test + +**Examples**: +- **Branded types**: `Email`, `UserId`, `Price` with validation +- **Domain models**: `Order`, `Money`, `ShippingAddress` (can use other leaf types) +- **Validation functions**: `validateEmail()`, `parseDate()` +- **Custom hooks (pure)**: `useValidation()`, `useDebounce()`, `useLocalStorage()` +- **Utility functions**: `formatCurrency()`, `parseAddress()` +- **Presentational components**: `Button`, `Card`, `Input` (UI-only, no business logic) + +**Coverage Target**: **100% unit test coverage** + +**Why**: +- Leaf types contain core business logic and must be bulletproof +- No external dependencies means easy to test in isolation (no mocks needed) +- High confidence in these building blocks enables safe composition +- Most bugs happen in complex logic - leaf types isolate that complexity + +**How to test**: +- Test only the public API (exports) +- Use real implementations, not mocks +- Cover happy path, edge cases, and error cases +- For hooks: Test with `@testing-library/react-hooks` or render in test component +- For components: Use React Testing Library with user-centric tests + +**Example - Leaf Hook**: +```typescript +// useDebounce.ts - Pure hook with no dependencies +export function useDebounce(value: T, delay: number): T { + const [debouncedValue, setDebouncedValue] = useState(value) + + useEffect(() => { + const timer = setTimeout(() => setDebouncedValue(value), delay) + return () => clearTimeout(timer) + }, [value, delay]) + + return debouncedValue +} + +// useDebounce.test.ts - 100% coverage +describe('useDebounce', () => { + it('returns initial value immediately', () => { + const { result } = renderHook(() => useDebounce('initial', 500)) + expect(result.current).toBe('initial') + }) + + it('updates value after delay', async () => { + const { result, rerender } = renderHook( + ({ value, delay }) => useDebounce(value, delay), + { initialProps: { value: 'initial', delay: 500 } } + ) + + rerender({ value: 'updated', delay: 500 }) + expect(result.current).toBe('initial') // Still old value + + await waitFor(() => { + expect(result.current).toBe('updated') // Now updated + }, { timeout: 600 }) + }) + + it('cancels pending update on unmount', () => { + const { result, unmount } = renderHook(() => useDebounce('value', 500)) + unmount() + // No error should be thrown + }) +}) +``` + +**Example - Leaf Types Composing Other Leaf Types**: +```typescript +// All leaf types - can freely compose each other +class Money { + constructor(private readonly cents: number) {} + + add(other: Money): Money { + return new Money(this.cents + other.cents) + } + + multiply(factor: number): Money { + return new Money(Math.round(this.cents * factor)) + } +} + +class ShippingAddress { + constructor( + readonly street: string, + readonly city: string + ) {} + + isRemoteLocation(): boolean { + return this.city === 'Remote' + } +} + +class Order { + constructor( + private items: CartItem[], + private shipping: ShippingAddress // ✅ Leaf using another leaf + ) {} + + calculateTotal(): Money { // ✅ Returns a leaf type + const subtotal = this.items.reduce( + (sum, item) => sum.add(Money.dollars(item.price)), + Money.dollars(0) + ) + + const shippingCost = this.shipping.isRemoteLocation() + ? Money.dollars(15) + : Money.dollars(5) + + return subtotal.add(shippingCost) + } +} + +// Testing is trivial - no mocks needed! +describe('Order', () => { + it('calculates total with shipping', () => { + const order = new Order( + [{ price: 100 }], + new ShippingAddress('123 Main', 'Springfield') + ) + expect(order.calculateTotal()).toEqual(Money.dollars(105)) + }) + + it('adds remote shipping for remote locations', () => { + const order = new Order( + [{ price: 100 }], + new ShippingAddress('456 Main', 'Remote') + ) + expect(order.calculateTotal()).toEqual(Money.dollars(115)) + }) +}) +``` + +**Example - NOT Leaf Types (External Dependencies)**: +```typescript +// NOT a leaf - depends on external API +class OrderService { + constructor(private api: ApiClient) {} // ❌ External dependency + + async fetchOrder(id: string): Promise { + return this.api.get(`/orders/${id}`) // ❌ API call + } + + async saveOrder(order: Order): Promise { + await this.api.post('/orders', order) // ❌ API call + } +} + +// NOT a leaf - depends on database +class OrderRepository { + constructor(private db: Database) {} // ❌ External dependency + + async findById(id: string): Promise { + return this.db.query('SELECT * FROM orders WHERE id = ?', [id]) // ❌ DB access + } +} + +// NOT a leaf - custom hook that fetches data +function useOrder(orderId: string) { + const [order, setOrder] = useState(null) + + useEffect(() => { + fetch(`/api/orders/${orderId}`) // ❌ API call + .then(res => res.json()) + .then(setOrder) + }, [orderId]) + + return order +} + +// Testing these requires mocks or test servers (integration tests) +``` + +**Key Insight**: Complex domain models like `Order`, `Money`, `ShippingAddress` are ALL leaf types because they're pure logic. They can compose each other freely. The boundary is **external systems**, not other application types. + +### Orchestrating Components/Functions + +**Definition**: Coordinate multiple leaf types, hooks, or services. They compose smaller pieces but contain minimal logic themselves. + +**Examples**: +- **Feature components**: `UserProfile`, `LoginForm`, `CheckoutFlow` +- **Page components**: `HomePage`, `DashboardPage` +- **Context providers**: `AuthProvider`, `ThemeProvider` +- **Container components**: Connect data (hooks/APIs) to presentation +- **Coordinator functions**: `processCheckout()` that calls multiple services + +**Coverage Target**: **Integration test coverage** + +**Why**: +- Test seams and interactions between leaf types +- Verify correct composition and data flow +- Can have some overlap with leaf type coverage +- Focus on behavior from user perspective + +**How to test**: +- Test entire feature flows +- Use MSW (Mock Service Worker) for API calls +- Test user interactions with `userEvent` +- Verify multiple components working together +- Test loading/error/success states + +**Example - Orchestrating Component**: +```typescript +// LoginForm.tsx - Orchestrates multiple pieces +export function LoginForm() { + const { login } = useAuth() // Leaf hook + const { validateEmail } = useValidation() // Leaf hook + const navigate = useNavigate() + + const handleSubmit = async (email: string, password: string) => { + if (!validateEmail(email)) return + await login(email, password) + navigate('/dashboard') + } + + return +} + +// LoginForm.test.tsx - Integration test (not 100% coverage required) +test('successful login navigates to dashboard', async () => { + const user = userEvent.setup() + + // Mock API + server.use( + http.post('/api/login', () => { + return HttpResponse.json({ token: 'abc123' }) + }) + ) + + render() + + await user.type(screen.getByLabelText(/email/i), 'user@example.com') + await user.type(screen.getByLabelText(/password/i), 'password123') + await user.click(screen.getByRole('button', { name: /sign in/i })) + + // Verify navigation happened + await waitFor(() => { + expect(window.location.pathname).toBe('/dashboard') + }) +}) +``` + +### Architectural Benefits + +When you follow this pattern: +1. **Easier testing**: Leaf types are trivial to test (100% coverage achievable) +2. **Better composition**: Small, focused pieces are easier to combine +3. **Easier refactoring**: Changes isolated to leaf types +4. **Lower cognitive load**: Each piece has single responsibility +5. **Reusability**: Leaf types naturally reusable across features + +**Anti-pattern**: Putting complex business logic directly in orchestrating components makes testing hard and coupling high. Always extract to leaf types. + +### Coverage Strategy Summary + +| Type | Coverage Target | Test Approach | Example | +|------|----------------|---------------|---------| +| Leaf | 100% unit tests | Isolated, no mocks | `useDebounce()`, `Email` type, `Button` | +| Orchestrating | Integration tests | User flows, MSW | `LoginForm`, `UserProfile`, `AuthProvider` | + +## Jest Matchers (jest-dom) + +Common assertions from `@testing-library/jest-dom`: + +```typescript +// Presence +expect(element).toBeInTheDocument() +expect(element).not.toBeInTheDocument() + +// Visibility +expect(element).toBeVisible() +expect(element).not.toBeVisible() + +// Enabled/Disabled +expect(button).toBeEnabled() +expect(button).toBeDisabled() + +// Form values +expect(input).toHaveValue('text') +expect(checkbox).toBeChecked() +expect(checkbox).not.toBeChecked() + +// Text content +expect(element).toHaveTextContent('Hello') +expect(element).toHaveTextContent(/hello/i) + +// Attributes +expect(element).toHaveAttribute('href', '/home') +expect(element).toHaveClass('active') +expect(element).toHaveStyle({ color: 'red' }) + +// Focus +expect(input).toHaveFocus() + +// Accessibility +expect(button).toHaveAccessibleName('Submit') +expect(button).toHaveAccessibleDescription('Submit the form') +``` + +## Coverage Configuration + +**jest.config.js**: +```javascript +module.exports = { + collectCoverageFrom: [ + 'src/**/*.{ts,tsx}', + '!src/**/*.d.ts', + '!src/**/*.stories.tsx', + '!src/test/**', + '!src/index.tsx' + ], + coverageThresholds: { + global: { + branches: 80, + functions: 80, + lines: 80, + statements: 80 + }, + // Pure components/hooks: 100% + './src/shared/components/**/*.{ts,tsx}': { + branches: 100, + functions: 100, + lines: 100, + statements: 100 + }, + './src/shared/hooks/**/*.{ts,tsx}': { + branches: 100, + functions: 100, + lines: 100, + statements: 100 + } + } +} +``` + +## Best Practices Summary + +### ✅ DO + +- Test user behavior, not implementation +- Use accessible queries (getByRole, getByLabelText) +- Use user-event for interactions +- Use MSW for API mocking +- Wait for conditions with waitFor/findBy +- Colocate tests with components +- Write descriptive test names +- Test error scenarios +- Test loading states +- Test accessibility + +### ❌ DON'T + +- Test implementation details (state, lifecycle) +- Use shallow rendering +- Use arbitrary timeouts (setTimeout) +- Test private methods +- Mock everything (prefer real implementations) +- Use getByTestId unless necessary +- Rely on snapshots for critical logic +- Write tests that depend on each other +- Ignore console errors/warnings + +## Common Pitfalls + +### 1. Testing implementation details + +```typescript +// ❌ Bad: Testing internal state +expect(component.state.isOpen).toBe(true) + +// ✅ Good: Testing user-visible behavior +expect(screen.getByRole('dialog')).toBeInTheDocument() +``` + +### 2. Not cleaning up + +```typescript +// ❌ Bad: No cleanup between tests +afterEach(() => { + // Test state leaks to next test +}) + +// ✅ Good: Proper cleanup +afterEach(() => { + server.resetHandlers() + jest.clearAllMocks() +}) +``` + +### 3. Arbitrary waits + +```typescript +// ❌ Bad: Arbitrary timeout +await new Promise(r => setTimeout(r, 1000)) + +// ✅ Good: Wait for specific condition +await waitFor(() => expect(screen.getByText(/loaded/i)).toBeInTheDocument()) +``` + +### 4. Overusing mocks + +```typescript +// ❌ Bad: Mock everything +jest.mock('./useAuth', () => ({ useAuth: () => mockAuth })) +jest.mock('./api', () => ({ fetchUser: mockFetch })) + +// ✅ Good: Use real implementations with MSW +// useAuth uses real context +// API calls intercepted by MSW +``` + +## Testing Checklist + +Before committing tests: +- [ ] Tests use accessible queries (getByRole, getByLabelText) +- [ ] User interactions use user-event, not fireEvent +- [ ] Async operations use waitFor/findBy, not setTimeout +- [ ] API calls mocked with MSW, not jest.mock +- [ ] Tests are independent (no shared state) +- [ ] Error scenarios covered +- [ ] Loading states covered +- [ ] Tests read like user stories +- [ ] No implementation details tested +- [ ] Coverage meets targets