pr-review.md

PR Review Instructions

Please review this pull request according to Logic's engineering standards. Focus on providing actionable feedback with specific line references.

Some of this document may not apply to this PR, so use your judgment to skip sections that are not relevant.

Architecture & Design Principles

MVC + Data Mapper Pattern

• Controllers should only handle HTTP concerns (request/response, validation, authentication)
• Services must contain all business logic and orchestration
• Models follow Data Mapper pattern:
  • Entity classes (extends TenantModel/BaseModel) define fields only
  • Repository classes (extends DatabaseRepo) contain queries and CRUD operations
• Views use TypeBox schemas for type-safe API contracts

Design Quality

• Enforce single responsibility principle - each class/function does ONE thing
• Avoid monolithic entities - prefer many focused components
• Functions should be under 50 lines (extract complex logic to helpers)
• Files should generally be under 500 lines
• Maximum nesting depth of 3-4 levels
• Use early returns for validation/guards

Code Standards

Naming Conventions

• Classes: PascalCase (DocumentService, AccountController)
• Methods/Functions: camelCase (createDocument, validateInput)
• Database tables: snake_case (document_execution)
• Constants: UPPER_SNAKE_CASE (MAX_RETRY_ATTEMPTS)
• Test files: {filename}.{unit|integration|e2e}.test.ts

File Organization

src/
├── controllers/{entity}/    # API endpoints
├── models/{entity}/        # model.ts + repo.ts
├── services/{entity}/      # service.ts + helpers
├── views/                  # TypeBox schemas
└── tasks/                  # Async jobs

Testing Requirements (100% Coverage)

• CI: Our CI pipeline will verify that all code is covered by tests

Test Types Required

• Unit tests: Isolated function testing with mocked dependencies
• Integration tests: Database operations and service interactions
• E2E tests: Full API flow with real external services

Test Quality Checks

• All public methods must have tests
• Both success and error paths tested (use expect.assertions() for error cases)
• Edge cases explicitly covered
• Descriptive test names explaining the scenario
• Proper setup/teardown using test contexts
• No test interdependencies

Security Requirements

Authentication & Authorization

• Bearer token validation on all protected endpoints
• Tenant isolation via org_id checks
• Row-level security (RLS) policies enforced
• Superadmin actions require explicit permission checks

Input Validation & Sanitization

• All inputs validated against TypeBox schemas
• Email addresses lowercased and trimmed
• SQL injection prevention via Kysely parameterized queries
• Request body size limits enforced
• No user input in raw SQL queries

Data Protection

• Tokens redacted in logs (show only prefix)
• No hardcoded secrets or credentials
• Environment variables for all configuration
• The code should never be aware of the environment it is running in.
  • Any values that might change between environments should be passed in as environment variables.

Comment Standards

When to Comment

• Complex business logic that isn't self-evident
• SQL queries explaining the purpose
• Public API methods needing context
• Security-critical code sections
• Performance optimizations

Comment Quality

• Explain "why" not "what"
• Be concise and relevant to current code
• No historical notes ("we used to do X")
• Update comments when code changes
• Use JSDoc for public methods that need documentation

Bad Comment Examples

// DON'T: We deleted an import here and moved the helper
// DON'T: Increment counter by 1
// DON'T: This used to be in another file

Good Comment Examples

// Rate limit bypass for health checks to ensure uptime monitoring
// Cross-org lookup - requires superadmin permissions for security
// Soft delete to maintain referential integrity in analytics

Performance & Database

Query Optimization

• Include appropriate indexes for common queries
• Use database views for complex joins
• Batch operations where possible
• Avoid N+1 query problems
• Ensure that all database queries will use some kind of index

Migration Standards

• Include both up and down migrations
• Add indexes for foreign keys
• Use consistent naming patterns
• Document breaking changes

Error Handling

Structured Errors

• Use appropriate HTTP status codes
• Throw typed errors (HTTPException)
• Include error context without exposing internals
• Log errors with correlation IDs
• Handle all promise rejections

User-Facing Messages

• Clear, actionable error messages
• No technical jargon or stack traces
• Suggest resolution steps
• Consistent error format

Code Hygiene

Import Organization

• Group imports by type (external, internal, types)
• Use absolute imports from src/
• No circular dependencies
• Remove unused imports
• NEVER use inline/dynamic imports: All imports must be at the top of the file
  • No await import() or require() inside functions or methods
  • This makes dependencies unclear and causes performance issues
  • All module dependencies should be explicit and statically analyzable

Type Safety

• No any types without justification
• Prefer unknown over any when needed
• Use discriminated unions for variants
• Leverage TypeBox schemas for validation

Final Checklist

• No functions over 50 lines
• No hardcoded values that should be config
• No commented-out code
• No console.log statements
• All TODOs have associated tickets
• Database migrations tested up and down
• API changes reflected in TypeBox schemas
• Security considerations addressed

Provide specific, actionable feedback with line numbers. Be objective and balanced—point out both strengths and areas for improvement, even if the PR is generally solid. Avoid excessive praise; focus on clarity, maintainability, correctness, and alignment with best practices.