Code Reviewer

Comprehensive code review skill for the Ishkul codebase (React Native/Expo + Go).

Tech Stack Context

• Frontend: React Native/Expo, TypeScript 5.9, Zustand, React Navigation 7
• Backend: Go 1.24, Cloud Run, Firestore
• Auth: Firebase + Custom JWT
• Testing: Jest + React Testing Library (frontend), Go testing + testify (backend)

Review Checklist

Frontend (TypeScript/React Native)

Critical Issues

• React Rules of Hooks - All hooks at top, before conditionals (see hooks-validator skill)
• No any types - Use unknown or proper types
• No hardcoded secrets - Use environment variables
• Proper error handling - Try-catch with typed ApiError

TypeScript Quality

• Interfaces for public APIs (props, return types)
• Types for internal structures
• No type assertions without validation (as keyword)
• Proper null/undefined handling with optional chaining

Component Patterns

// GOOD - Follows Ishkul patterns
interface ComponentProps {
  title: string;
  onPress: () => void;
  variant?: 'primary' | 'secondary';
}

export const Component: React.FC<ComponentProps> = ({
  title,
  onPress,
  variant = 'primary',
}) => {
  const { colors } = useTheme();  // Theme hook
  // ...
};

// BAD
export const Component = (props: any) => { // No type
  const { colors } = useTheme();
  if (loading) return null; // Hook after this could crash
  const [state] = useState(); // BAD!
};

State Management (Zustand)

• Store actions properly typed
• Selectors for derived state
• No state mutations (use set())
• Clear separation of concerns

// GOOD - Follows lessonStore.ts pattern
interface LessonState {
  currentLesson: Lesson | null;
  lessonLoading: boolean;
  error: string | null;

  // Actions
  fetchLesson: (courseId: string, lessonId: string) => Promise<Lesson | null>;
  clearError: () => void;
}

export const useLessonStore = create<LessonState>((set, get) => ({
  currentLesson: null,
  lessonLoading: false,
  error: null,

  fetchLesson: async (courseId, lessonId) => {
    set({ lessonLoading: true, error: null });
    try {
      const lesson = await lessonsApi.getLesson(courseId, lessonId);
      set({ currentLesson: lesson, lessonLoading: false });
      return lesson;
    } catch (error) {
      const message = error instanceof Error ? error.message : 'Unknown error';
      set({ error: message, lessonLoading: false });
      return null;
    }
  },
}));

API Client Usage

• Use ApiClient from services/api/client.ts
• Proper error handling with ApiError
• No raw fetch() calls for authenticated endpoints

// GOOD
try {
  const data = await apiClient.get<ResponseType>('/endpoint');
} catch (error) {
  if (error instanceof ApiError) {
    if (error.status === 401) { /* handle auth */ }
    console.error(error.code, error.message);
  }
}

// BAD
const response = await fetch('/endpoint'); // Missing auth, error handling

Backend (Go)

Critical Issues

• Input validation - Validate all request bodies
• Error codes - Use defined error codes (ErrCodeInvalidRequest, etc.)
• Context usage - Pass context through for cancellation
• No panics in handlers - Always return proper errors

Handler Patterns

// GOOD - Follows Ishkul handler pattern
func HandleResource(w http.ResponseWriter, r *http.Request) {
    // Method check first
    if !RequireMethod(w, r, http.MethodPost) {
        return
    }

    // Parse request
    var req RequestType
    if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
        sendErrorResponse(w, http.StatusBadRequest, ErrCodeInvalidRequest, "Invalid request format")
        return
    }

    // Validate
    if req.Field == "" {
        sendErrorResponse(w, http.StatusBadRequest, ErrCodeInvalidRequest, "Field is required")
        return
    }

    // Get user from context (set by auth middleware)
    userID := middleware.GetUserID(r.Context())
    if userID == "" {
        sendErrorResponse(w, http.StatusUnauthorized, ErrCodeUnauthorized, "User not authenticated")
        return
    }

    // Business logic
    result, err := processRequest(r.Context(), req)
    if err != nil {
        sendErrorResponse(w, http.StatusInternalServerError, ErrCodeInternalError, "Processing failed")
        return
    }

    JSONSuccess(w, result)
}

// BAD
func HandleResource(w http.ResponseWriter, r *http.Request) {
    var req RequestType
    json.NewDecoder(r.Body).Decode(&req) // Error ignored!
    result := process(req) // No validation, no error handling
    json.NewEncoder(w).Encode(result) // No content-type
}

Error Response Pattern

// Use defined error codes
const (
    ErrCodeInvalidRequest     = "INVALID_REQUEST"
    ErrCodeUnauthorized       = "UNAUTHORIZED"
    ErrCodeNotFound           = "NOT_FOUND"
    ErrCodeInternalError      = "INTERNAL_ERROR"
)

// Send structured error
sendErrorResponse(w, http.StatusBadRequest, ErrCodeInvalidRequest, "Human-readable message")

Logging Pattern

// Use structured logging
appLogger.Info("action_completed",
    slog.String("user_id", userID),
    slog.String("resource_id", resourceID),
    slog.Int("count", len(items)),
)

appLogger.Error("action_failed",
    slog.String("error", err.Error()),
    slog.String("user_id", userID),
)

Security Review

Frontend Security

• No secrets in code (use config files)
• Proper token storage (tokenStorage service)
• No sensitive data in logs
• Input sanitization for user-generated content

Backend Security

• All /api/* routes use Auth middleware
• Firebase Admin SDK for server-side auth
• No SQL injection (Firestore is NoSQL, but validate inputs)
• Rate limiting on auth endpoints
• CORS properly configured

Firebase Security Rules

• Users can only read/write own documents
• No wildcard write permissions
• Validation rules on document structure

Performance Review

Frontend Performance

• No N+1 patterns in data fetching
• Memoization where needed (useMemo, useCallback)
• Proper list virtualization for long lists
• Images use proper sizing

Backend Performance

• Firestore queries use proper indexes
• No unbounded queries (always limit)
• Batch operations for multiple writes
• Connection pooling for external services

Testing Requirements

Frontend Tests

• Test file exists for new screens/components
• Loading, error, success states tested
• State transition tests (critical for hooks safety)
• User interactions tested

Backend Tests

• Test file exists for new handlers
• Success and error cases covered
• Edge cases handled

Output Format

## Code Review Summary

**Files Reviewed**: 5
**Issues Found**: 3 Critical, 2 Medium, 5 Low

### Critical Issues

#### Issue #1: React Hooks Violation
- **File**: `src/screens/NewScreen.tsx:45`
- **Issue**: `useCallback` called after conditional return
- **Impact**: Will crash on state transitions
- **Fix**:
  ```typescript
  // Move before conditional return
  const handleSubmit = useCallback(() => {...}, [deps]);

  if (loading) return <Spinner />;

Issue #2: Missing Input Validation

• File: internal/handlers/users.go:67
• Issue: Request body parsed without validation
• Impact: Could crash or allow invalid data
• Fix:

  if req.Email == "" {
      sendErrorResponse(w, http.StatusBadRequest, ErrCodeInvalidRequest, "Email required")
      return
  }
  

Medium Issues

...

Low Issues

...

Positive Notes

• Good separation of concerns in store
• Proper TypeScript types throughout
• Comprehensive error handling

## When to Use

- After implementing a new feature
- Before creating a PR
- During PR code review
- After major refactoring