Initial commit
This commit is contained in:
Zhongwei Li
467
skills/pre-commit-review/SKILL.md
Normal file
467
skills/pre-commit-review/SKILL.md
Normal file
@@ -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<string, 'UserId'>
|
||||
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
|
||||
<Parent>
|
||||
<Middle user={user} onUpdate={onUpdate}>
|
||||
<Deep user={user} onUpdate={onUpdate}>
|
||||
<VeryDeep user={user} onUpdate={onUpdate} />
|
||||
</Deep>
|
||||
</Middle>
|
||||
</Parent>
|
||||
|
||||
// ✅ Better: Use context or composition
|
||||
<UserProvider>
|
||||
<Parent>
|
||||
<Middle><Deep><VeryDeep /></Deep></Middle>
|
||||
</Parent>
|
||||
</UserProvider>
|
||||
```
|
||||
|
||||
#### 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 <UI user={user} loading={loading} />
|
||||
}
|
||||
```
|
||||
|
||||
### 3. Review Accessibility
|
||||
|
||||
**Check for each component** (jsx-a11y rules + manual review):
|
||||
|
||||
#### Semantic HTML
|
||||
- Using correct HTML elements (<button>, <form>, <nav>, <main>)
|
||||
- Proper heading hierarchy (h1 → h2 → h3, no skipping)
|
||||
- Lists for list content (<ul>, <ol>)
|
||||
|
||||
**Example violations**:
|
||||
```typescript
|
||||
// 🔴 Design Debt: Non-semantic
|
||||
<div onClick={handleClick}>Click me</div> // Should be <button>
|
||||
|
||||
// 🟡 Readability Debt: Wrong heading order
|
||||
<h1>Title</h1>
|
||||
<h3>Subtitle</h3> // Skipped h2
|
||||
|
||||
// ✅ Better
|
||||
<button onClick={handleClick}>Click me</button>
|
||||
<h1>Title</h1>
|
||||
<h2>Subtitle</h2>
|
||||
```
|
||||
|
||||
#### 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
|
||||
<input type="text" placeholder="Email" />
|
||||
|
||||
// 🟢 Polish: Could improve alt text
|
||||
<img src="avatar.jpg" alt="image" /> // Generic
|
||||
|
||||
// ✅ Better
|
||||
<label htmlFor="email">Email</label>
|
||||
<input id="email" type="text" />
|
||||
|
||||
<img src="avatar.jpg" alt="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
|
||||
<div onClick={handleClick}>Action</div>
|
||||
|
||||
// ✅ Better
|
||||
<button onClick={handleClick}>Action</button>
|
||||
|
||||
// Or if div required:
|
||||
<div
|
||||
role="button"
|
||||
tabIndex={0}
|
||||
onClick={handleClick}
|
||||
onKeyDown={(e) => e.key === 'Enter' && handleClick()}
|
||||
>
|
||||
Action
|
||||
</div>
|
||||
```
|
||||
|
||||
#### 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
|
||||
<Input style={{ borderColor: 'red' }} />
|
||||
|
||||
// ✅ Better: Visual + text indicator
|
||||
<Input
|
||||
aria-invalid="true"
|
||||
aria-describedby="email-error"
|
||||
style={{ borderColor: 'red' }}
|
||||
/>
|
||||
<span id="email-error">Email is invalid</span>
|
||||
```
|
||||
|
||||
#### 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 && <Spinner />}
|
||||
|
||||
// ✅ Better
|
||||
{isLoading && (
|
||||
<div role="status" aria-live="polite">
|
||||
Loading user data...
|
||||
<Spinner aria-hidden="true" />
|
||||
</div>
|
||||
)}
|
||||
```
|
||||
|
||||
### 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<typeof DataSchema>) { }
|
||||
|
||||
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: <input type="email" />
|
||||
Better: <input type="email" aria-describedby="email-hint" />
|
||||
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.
|
||||
610
skills/pre-commit-review/reference.md
Normal file
610
skills/pre-commit-review/reference.md
Normal file
@@ -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<string, 'UserId'>
|
||||
type Email = Brand<string, 'Email'>
|
||||
|
||||
// Or Zod schemas
|
||||
const UserIdSchema = z.string().uuid()
|
||||
const EmailSchema = z.string().email()
|
||||
|
||||
type UserId = z.infer<typeof UserIdSchema>
|
||||
type Email = z.infer<typeof EmailSchema>
|
||||
|
||||
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<typeof PriceSchema>
|
||||
type Quantity = z.infer<typeof QuantitySchema>
|
||||
type Rating = z.infer<typeof RatingSchema>
|
||||
```
|
||||
|
||||
#### 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
|
||||
<GrandParent user={user}>
|
||||
<Parent user={user} onUpdate={onUpdate}>
|
||||
<Child user={user} onUpdate={onUpdate}>
|
||||
<GrandChild user={user} onUpdate={onUpdate} />
|
||||
</Child>
|
||||
</Parent>
|
||||
</GrandParent>
|
||||
```
|
||||
|
||||
**✅ 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 <Spinner />
|
||||
if (error) return <ErrorDisplay error={error} />
|
||||
return <UserDisplay user={user} actions={actions} />
|
||||
}
|
||||
```
|
||||
|
||||
#### 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 <UI feature={feature} />
|
||||
}
|
||||
```
|
||||
|
||||
#### 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 |
|
||||
|-------------|--------------|-----|
|
||||
| `<div onClick>` | `<button>` | Keyboard accessible, screen reader friendly |
|
||||
| `<div>` for text | `<p>`, `<span>` | Proper semantics |
|
||||
| `<div>` for navigation | `<nav>` | Landmark for screen readers |
|
||||
| `<div>` for lists | `<ul>`, `<ol>` | Proper list semantics |
|
||||
| `<div>` for headings | `<h1>`-`<h6>` | Document outline |
|
||||
|
||||
#### Form Accessibility
|
||||
|
||||
**🔴 Design Debt**: Missing labels
|
||||
```typescript
|
||||
<input type="text" placeholder="Email" />
|
||||
<input type="password" placeholder="Password" />
|
||||
```
|
||||
|
||||
**✅ Better**:
|
||||
```typescript
|
||||
<label htmlFor="email">Email</label>
|
||||
<input id="email" type="text" />
|
||||
|
||||
<label htmlFor="password">Password</label>
|
||||
<input id="password" type="password" />
|
||||
```
|
||||
|
||||
**🟢 Polish**: Enhanced with descriptions
|
||||
```typescript
|
||||
<label htmlFor="email">Email</label>
|
||||
<input
|
||||
id="email"
|
||||
type="email"
|
||||
aria-describedby="email-hint"
|
||||
aria-required="true"
|
||||
/>
|
||||
<span id="email-hint">We'll never share your email.</span>
|
||||
```
|
||||
|
||||
#### Interactive Elements
|
||||
|
||||
**🔴 Design Debt**: Non-semantic interactive elements
|
||||
```typescript
|
||||
<div onClick={handleClick}>
|
||||
Click me
|
||||
</div>
|
||||
```
|
||||
|
||||
**✅ Better**: Semantic button
|
||||
```typescript
|
||||
<button onClick={handleClick}>
|
||||
Click me
|
||||
</button>
|
||||
```
|
||||
|
||||
**If div required**:
|
||||
```typescript
|
||||
<div
|
||||
role="button"
|
||||
tabIndex={0}
|
||||
onClick={handleClick}
|
||||
onKeyDown={(e) => {
|
||||
if (e.key === 'Enter' || e.key === ' ') {
|
||||
handleClick()
|
||||
}
|
||||
}}
|
||||
>
|
||||
Click me
|
||||
</div>
|
||||
```
|
||||
|
||||
#### Images and Media
|
||||
|
||||
**🔴 Design Debt**: Missing alt text
|
||||
```typescript
|
||||
<img src="avatar.jpg" />
|
||||
```
|
||||
|
||||
**🟢 Polish**: Generic alt text
|
||||
```typescript
|
||||
<img src="avatar.jpg" alt="avatar" />
|
||||
```
|
||||
|
||||
**✅ Better**: Descriptive alt text
|
||||
```typescript
|
||||
<img src="avatar.jpg" alt="John Doe's profile picture" />
|
||||
```
|
||||
|
||||
**Decorative images**:
|
||||
```typescript
|
||||
<img src="decoration.svg" alt="" role="presentation" />
|
||||
```
|
||||
|
||||
#### Dynamic Content
|
||||
|
||||
**🟢 Polish**: Loading without announcement
|
||||
```typescript
|
||||
{isLoading && <Spinner />}
|
||||
```
|
||||
|
||||
**✅ Better**: Announced loading
|
||||
```typescript
|
||||
{isLoading && (
|
||||
<div role="status" aria-live="polite">
|
||||
<span className="sr-only">Loading user data...</span>
|
||||
<Spinner aria-hidden="true" />
|
||||
</div>
|
||||
)}
|
||||
```
|
||||
|
||||
**Error announcements**:
|
||||
```typescript
|
||||
{error && (
|
||||
<div role="alert" aria-live="assertive">
|
||||
{error.message}
|
||||
</div>
|
||||
)}
|
||||
```
|
||||
|
||||
#### Modal Accessibility
|
||||
|
||||
**🔴 Design Debt**: Basic modal
|
||||
```typescript
|
||||
{isOpen && (
|
||||
<div className="modal">
|
||||
<div className="content">
|
||||
<h2>Title</h2>
|
||||
<p>Content</p>
|
||||
<button onClick={onClose}>Close</button>
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
```
|
||||
|
||||
**✅ Better**: Accessible modal
|
||||
```typescript
|
||||
{isOpen && (
|
||||
<div
|
||||
role="dialog"
|
||||
aria-modal="true"
|
||||
aria-labelledby="modal-title"
|
||||
onKeyDown={(e) => e.key === 'Escape' && onClose()}
|
||||
>
|
||||
<div className="content">
|
||||
<h2 id="modal-title">Title</h2>
|
||||
<p>Content</p>
|
||||
<button onClick={onClose} aria-label="Close dialog">
|
||||
Close
|
||||
</button>
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
```
|
||||
|
||||
**With focus management**:
|
||||
```typescript
|
||||
function Modal({ isOpen, onClose, children }) {
|
||||
const modalRef = useRef<HTMLDivElement>(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
|
||||
<span style={{ color: 'red' }}>Error</span>
|
||||
<span style={{ color: 'green' }}>Success</span>
|
||||
```
|
||||
|
||||
**✅ Better**: Multiple indicators
|
||||
```typescript
|
||||
<span style={{ color: 'red' }}>
|
||||
<ErrorIcon aria-hidden="true" />
|
||||
<span>Error</span>
|
||||
</span>
|
||||
```
|
||||
|
||||
**With ARIA**:
|
||||
```typescript
|
||||
<span
|
||||
style={{ color: 'red' }}
|
||||
role="alert"
|
||||
aria-label="Error"
|
||||
>
|
||||
<ErrorIcon aria-hidden="true" />
|
||||
<span>Invalid email address</span>
|
||||
</span>
|
||||
```
|
||||
|
||||
### 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 <UI state={state} actions={actions} />
|
||||
}
|
||||
```
|
||||
|
||||
### 8. Error Handling
|
||||
|
||||
#### Missing Error Boundaries
|
||||
|
||||
**🔴 Design Debt**: No error handling
|
||||
```typescript
|
||||
function AsyncComponent() {
|
||||
const data = useAsyncData() // Can throw
|
||||
return <Display data={data} />
|
||||
}
|
||||
```
|
||||
|
||||
**✅ Better**: Error boundary wrapper
|
||||
```typescript
|
||||
<ErrorBoundary fallback={<ErrorDisplay />}>
|
||||
<AsyncComponent />
|
||||
</ErrorBoundary>
|
||||
```
|
||||
|
||||
**Or component-level handling**:
|
||||
```typescript
|
||||
function AsyncComponent() {
|
||||
const { data, error, isLoading } = useAsyncData()
|
||||
|
||||
if (isLoading) return <Spinner />
|
||||
if (error) return <ErrorDisplay error={error} />
|
||||
if (!data) return <NotFound />
|
||||
|
||||
return <Display data={data} />
|
||||
}
|
||||
```
|
||||
|
||||
## 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**.
|
||||
Reference in New Issue
Block a user