# Merge Request Review Guidelines

When reviewing merge requests in this repository, follow these guidelines systematically.

## Review Workflow

1. **Read the MR description and any linked issues first** - understand the intent and requirements before looking at code
2. **Check the diff against architecture and code quality principles below**
3. **Look at related files for consistency** - check how similar code is structured elsewhere
4. **Post findings as a structured MR comment** with severity labels

## Severity Labels

Use these labels when reporting issues:

- `🚨 CRITICAL` - Security vulnerabilities, data loss risks, breaking changes
- `⚠️ WARNING` - Violations of architecture principles, potential bugs, performance issues
- `💡 SUGGESTION` - Code style improvements, refactoring opportunities, minor enhancements
- `❓ QUESTION` - Clarification needed, unclear intent, missing context

## Architecture Principles

### Database Access

- **No direct DB calls outside service/repository layer**
  - All database operations must go through service or repository classes
  - Controllers must use injected services, never direct database access
  - Flag any direct database session usage in controllers or handlers

### Service Layer Pattern

- Services follow **Interface + Impl pattern**: `UserService` (interface) + `UserServiceImpl` (implementation)
- Service implementations should be properly annotated for transactions
- Dependencies injected via constructor or setter injection
- Flag services that don't follow the established pattern

### External API Calls

- **All external API calls need timeout and error handling**
  - HTTP clients must configure connection and read timeouts
  - Wrap external calls in try-catch with meaningful error messages
  - Flag any HTTP client calls without timeout configuration
  - Flag silent failures when calling external integrations

### Exception Handling

- Custom exceptions should extend appropriate base exception classes
- Handlers should throw meaningful exceptions with context
- Let framework interceptors handle exception propagation

## Code Quality Rules

### No Silent Catch Blocks

- **Every catch block must either:**
  - Log the exception appropriately
  - Re-throw a wrapped exception
  - Return an error response to the user
- Flag empty catch blocks or catch blocks with only comments
- Flag `catch (Exception e) {}` patterns

### No Hardcoded Secrets

- **Flag any hardcoded:**
  - API keys, tokens, passwords
  - Cloud credentials, database connection strings
  - Webhook URLs, service tokens
  - Private keys or certificates
- These must use environment variables or secret management

### Function Length

- **Functions over 40 lines need justification**
  - Check if the function can be split into smaller, focused methods
  - Long functions are acceptable if they're linear workflows (e.g., multi-step validation)
  - Flag functions with deep nesting (>3 levels) regardless of length

### Logging

- Use consistent logger instantiation pattern
- Logger should be appropriately scoped (typically private static final)
- Include context in log messages (IDs, operation names)

## Pattern Consistency

### Naming Conventions

- **Classes**: PascalCase
  - Controllers/Actions: `*Controller` or `*Action`
  - Services: `*Service` / `*ServiceImpl`
  - Entities: Direct names (e.g., `User`, `Order`, `Product`)
  - Exceptions: `*Exception`
  - Tests: `*Test`
- **Methods**: camelCase, starting with verb
- **Constants**: UPPER_SNAKE_CASE
- **Test methods**: `methodName_expectedBehavior()` pattern

### Frontend Conventions

- **Components**: PascalCase for component names
- **Hooks**: `use*` prefix (e.g., `useDebounce`)
- **Utilities**: camelCase
- Use functional components with hooks, not class components
- Minimize `any` types - prefer explicit typing

### Flag Deviations From Existing Patterns

When reviewing, check if the new code deviates from patterns already used elsewhere:

- Different error handling approach than similar files
- Different naming convention than existing code
- Different state management pattern than other components
- Different API response format than existing endpoints

### Flag Duplicate Logic

Check if the logic being added already exists in shared utilities or common components. If similar logic exists, suggest reusing or extracting to a shared location.

## Testing Requirements

### Backend

- New service methods should have corresponding tests
- Use appropriate mocking framework for dependencies
- Mock external dependencies, don't call real services

### Frontend

- New components should have tests
- Test user interactions, not implementation details

## Security Checklist

- [ ] No SQL injection vulnerabilities (use parameterized queries)
- [ ] No XSS vulnerabilities (escape user input in templates)
- [ ] Authentication required on new endpoints
- [ ] Sensitive data not logged
- [ ] File uploads validated (type, size)
- [ ] User authorization checked (user can only access their own data)

## Output Format

Output your review as JSON (IMPORTANT: This format is required for CI parsing):

```json
{
  "status": "APPROVED",
  "review": "## Code Review\n\n**Overall Assessment**: APPROVED\n\n### Issues Found\n\n(none)\n\n### What Looks Good\n- Code follows project patterns\n"
}
```

**Status values:**
- `APPROVED` - No blocking issues, ready to merge
- `CHANGES_REQUESTED` - Has CRITICAL or WARNING issues that must be fixed
- `NEEDS_DISCUSSION` - Unclear requirements or architectural questions

**Review field:** Markdown content for the comment, including:
- Summary of what was reviewed
- Issues found (with severity labels)
- What looks good
- Recommendations