Pre Commit Review

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), types relying on upstream validation, or re-validating composed validated types

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: Consistent architecture. Design Debt (ADVISORY) - never blocks commit.

Check file patterns:

• src/{components,hooks,contexts}/ → ✅ Layer-based (most common)
• src/features/[feature]/{components,hooks,context}/ → ✅ Feature-based
• Mixed patterns → ✅ Hybrid (if intentional)
• Inconsistent patterns → 🔴 Design Debt

Advisory Categories:

1. ✅ Consistent architecture → Acknowledge pattern, ensure new code follows it
2. 🟢 Hybrid with clear boundaries → Validate shared vs feature-specific distinction is clear
3. 🔴 Inconsistent patterns (advisory) → Suggest establishing clear conventions

Report Template:

🟢 Architecture Review: Layer-Based Pattern
- Current: Code organized by technical layer (components/, hooks/, contexts/)
- Status: Consistent with existing codebase ✅
- New code follows established pattern ✅

Or if inconsistent:

🔴 Design Debt (Advisory): Inconsistent Architecture
- Issue: Mixed patterns without clear conventions
- Examples: Some auth code in src/components/, other auth code scattered
- Suggestion: Document architecture decisions and apply consistently
- Alternative: Proceed as-is (address in future refactor)

Always acknowledge: Consistency with existing codebase is the priority.

1. Analyze Commit Scope

# 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:

// 🔴 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:

// 🔴 Design Debt: Prop drilling
<Parent>
  <Middle user={user}
    <Deep user={user}
      <VeryDeep user={user} />
    </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:

// 🟡 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 (, , , )
• Proper heading hierarchy (h1 → h2 → h3, no skipping)
• Lists for list content (, )

Example violations:

// 🔴 Design Debt: Non-semantic
<div me</div>  // Should be <button>

// 🟡 Readability Debt: Wrong heading order
<h1>Title</h1>
<h3>Subtitle</h3>  // Skipped h2

// ✅ Better
<button 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:

// 🔴 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:

// 🔴 Design Debt: No keyboard support
<div

// ✅ Better
<button

// Or if div required:
<div
  role="button"
  tabIndex={0}
 
  => 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:

// 🟡 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:

// 🟢 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:

// 🔴 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:

// 🟡 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/components/). 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/components/LoginForm.tsx (+45, -20 lines)
- src/types/auth.ts (+15, -0 lines)
- src/hooks/useAuth.ts (+30, -5 lines)

🔴 DESIGN DEBT (2 findings) - Recommended to fix:

1. src/components/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/components/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/components/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/components/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/hooks/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/types/auth.ts:10 - Missing JSDoc
   Public Email type should have documentation
   Suggestion: Add JSDoc explaining validation rules

2. src/components/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/components/LoginForm.tsx:55 - Keyboard navigation
   Close button could have Escape key handler
   Suggestion: Add onKeyDown handler for Escape key

4. src/hooks/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.

Acceptance Criteria

All criteria must be met before review is considered complete.

Mandatory Requirements (Must Pass)

1. Review Scope Complete

   • All changed files reviewed
   • All new components/hooks analyzed
   • Broader context examined (entire modified files)

2. Categorization Complete

   • All findings categorized (Design Debt / Readability Debt / Polish)
   • Each finding has: location, current code, better approach, why it matters
   • Fix suggestions provided for each finding

3. Design Principles Checked

   • Primitive obsession checked
   • Component composition checked
   • Prop drilling checked
   • Custom hook extraction opportunities identified
   • Type safety validated (each type owns validation; composed Zod/branded types trusted, not re-validated)

4. Accessibility Checked

   • Semantic HTML usage verified
   • ARIA attributes checked
   • Keyboard navigation verified
   • Form labels checked
   • Color contrast considerations noted

5. Findings Reported

   • Clear output format used
   • User presented with options (commit as-is, fix debt, etc.)
   • Broader context patterns noted if found

Review Completion Checklist

✅ PRE-COMMIT REVIEW ACCEPTANCE CRITERIA

Scope:
[ ] All changed files reviewed
[ ] All new code analyzed
[ ] Broader file context examined

Categorization:
[ ] Design Debt findings identified
[ ] Readability Debt findings identified
[ ] Polish Opportunities identified
[ ] Each finding has fix suggestion

Design Principles:
[ ] No primitive obsession
[ ] No prop drilling
[ ] Proper component composition
[ ] Custom hooks where appropriate

Accessibility:
[ ] Semantic HTML
[ ] Proper ARIA
[ ] Keyboard accessible
[ ] Form labels present

Output:
[ ] Findings formatted clearly
[ ] User options presented
[ ] Recommendations clear

Review complete: All boxes checked ✅

What Blocks Completion

The following will BLOCK review completion:

• Files not reviewed
• Findings not categorized
• Missing fix suggestions
• Accessibility not checked
• No user options presented

Review Output Requirements

Review must include:

1. Files Reviewed - List of all files examined
2. Design Debt - High-impact issues with fix suggestions
3. Readability Debt - Medium-impact issues with fix suggestions
4. Polish Opportunities - Low-impact improvements
5. Broader Context - Patterns noticed in unmodified code
6. User Options - Clear choices for how to proceed

Note: This review is ADVISORY. It never blocks commits but informs decisions.

Additional Resources

• reference.md - Complete review checklist and examples
• examples.md - Specific violations to check for:
  • Design Debt: IIFE patterns, primitive obsession, prop drilling
  • Readability Debt: Empty blocks, magic numbers
  • Polish: Comment quality, EMPTY_STRING usage
  • Detection patterns and review finding formats
  • Suggested fixes for each violation type