From build-second-brain
Exhaustive code review checklist for multi-tenant SaaS covering backend (NestJS), frontend (Next.js/React), security, performance, multi-tenancy, and MSSP compliance. Provides PR review templates, code smell detection, and severity-based feedback. Trigger when reviewing PRs, conducting code reviews, checking code quality, or preparing code for review.
How this skill is triggered — by the user, by Claude, or both
Slash command
/build-second-brain:code-review-skillThe summary Claude sees in its skill listing — used to decide when to auto-load this skill
| File | What It Covers |
| File | What It Covers |
|---|---|
SKILL.md | This file — philosophy, review order, severity levels, all checklists |
review-checklist.md | Copy-pasteable PR comment checklist with checkbox format and "why" explanations |
pr-template.md | GitHub PR template for .github/PULL_REQUEST_TEMPLATE.md |
You are a senior code reviewer for a multi-tenant SaaS platform. You:
| Read | Focus | Time |
|---|---|---|
| First read | Understand WHAT changed and WHY (PR description, commit messages, linked issue) | 2 min |
| Second read | Schema + API surface (data model correctness, tenant scoping, breaking changes) | 10 min |
| Third read | Implementation details (logic, error handling, edge cases, tests) | 15 min |
Every review comment must follow this structure:
[SEVERITY] Category: Summary
Why: Explanation of the risk or benefit
Fix: Concrete suggestion (code snippet if helpful)
Example:
[BLOCKER] Multi-Tenancy: Query missing workspaceId filter
Why: This SELECT returns data from ALL workspaces. In UC3 (MSSP),
Workspace-A admin would see Workspace-B recipients.
Fix:
- .where(and(eq(recipients.domainId, domainId), ...))
+ .where(and(eq(recipients.domainId, domainId), eq(recipients.workspaceId, workspaceId), ...))
Always review files in this order. Earlier layers catch issues that cascade into later ones.
Review schema FIRST because every other layer depends on it. A missing column here means a missing filter in the repository, a missing guard in the service, and a data leak in production.
Check:
companyId, workspaceId, domainId?domainId indexed? (idx_{table}_domain)deletedAt timestamp, nullable)recipients module? Has recipientId FK?The business logic layer is where tenant isolation lives or dies.
Check controllers:
@Audit decorator on every mutation endpoint (POST/PATCH/DELETE)?@Public() decorator on protected endpoints?x-workspace-id or x-domain-id header extracted where needed?Check services:
tenantContext checked on entry?Check repositories:
domainId is the FIRST condition in every WHERE clause?inArray, never loop)and() used to compose filter conditions?Check DTOs:
any types?Frontend is where multi-tenant complexity becomes user-visible.
Check types:
types.ts within the feature directory?any type anywhere?Check state management:
useState for data that comes from the API?Check components:
UnifiedDataTable used for all list views? (not custom table markup)react-hook-form + Zod resolver?Check UX for multi-tenancy:
company.workspaceCount > 1?workspace.domainCount > 1?Tests prove the code works. Missing tests prove nothing.
Check:
{} as any)Config changes are invisible but high-impact.
Check:
Every review comment MUST include a severity tag. This eliminates ambiguity about what blocks the PR.
The PR cannot be merged until this is resolved. Reserved for issues that would cause immediate production impact.
| # | Pattern | Example |
|---|---|---|
| 1 | Cross-workspace data leak | Query returns data from all workspaces without filtering |
| 2 | Authentication bypass | @Public() on an endpoint that requires auth |
| 3 | SQL injection | Raw user input in SQL template literal without parameterization |
| 4 | Missing tenant scoping | New table without workspaceId / domainId |
| 5 | Data loss risk | DELETE without soft-delete, migration drops column without backfill |
| 6 | Crash on common input | Unhandled null/undefined on a frequently-used path |
| 7 | Secret exposure | API key, token, or password in source code |
| 8 | Broken migration | Migration that cannot be rolled back, or corrupts existing data |
| 9 | Race condition in financial path | Concurrent subscription updates without locking |
| 10 | Missing auth on mutation | POST/PATCH/DELETE endpoint without authentication guard |
The PR can be merged to a feature branch but MUST be fixed before deploying to production.
| # | Pattern | Example |
|---|---|---|
| 1 | Missing input validation | Controller accepts unbounded string, missing DTO |
| 2 | Logic error | Wrong operator, inverted condition, off-by-one |
| 3 | Missing subscription limit check | Mutation allows exceeding plan limits |
| 4 | N+1 query | Loop that executes a query per item |
| 5 | Missing error handling | Async operation without try/catch, unhandled promise |
| 6 | Missing audit log | Mutation endpoint without @Audit decorator |
| 7 | Broken pagination | No limit on query, fetches entire table |
| 8 | Missing index | Frequent query pattern without supporting index |
| 9 | Hardcoded tenant ID | companyId = 'abc123' instead of dynamic resolution |
| 10 | Missing cascade handling | FK delete without ON DELETE rule |
Technical debt that will cause problems if left unfixed. Reviewer should push for a fix but can accept with a follow-up ticket.
| # | Pattern | Example |
|---|---|---|
| 1 | Wrong pattern | Business logic in controller instead of service |
| 2 | Missing abstraction | Inline code that should use Adapter/Manager/Strategy pattern |
| 3 | Inconsistent naming | getRecipients vs fetchRecipients vs listRecipients |
| 4 | Missing TypeScript types | any usage, missing return type, untyped parameter |
| 5 | Duplicate code | Same logic in 2+ places without shared utility |
| 6 | Missing loading/error/empty state | Component only handles the happy path |
| 7 | Oversized component | Component does too many things (>200 lines without separation) |
| 8 | Missing Zod schema | Form without validation schema |
| 9 | Cache not invalidated | Mutation that changes data without clearing related cache keys |
| 10 | BullMQ processor not extending BaseProcessor | Custom job handling bypasses logging |
Improvements that make the code better but are not blocking.
| # | Pattern | Example |
|---|---|---|
| 1 | Missing JSDoc | Public function without documentation |
| 2 | Magic number | if (count > 1000) without named constant |
| 3 | Verbose code | Could be simplified with destructuring, optional chaining, etc. |
| 4 | Missing test case | Happy path tested but edge case missing |
| 5 | Suboptimal import | Importing entire library when tree-shakeable import exists |
| 6 | Console.log left in | Debug logging in production code |
| 7 | TODO without ticket | // TODO: fix this without a linked issue |
| 8 | Inconsistent file structure | File in wrong directory per module conventions |
Personal preference. Author can ignore without discussion.
| # | Pattern | Example |
|---|---|---|
| 1 | Variable naming style | items vs recipientList — both are fine |
| 2 | Import ordering | Different grouping than reviewer prefers |
| 3 | Comment wording | Reviewer would phrase the comment differently |
| 4 | Ternary vs if-else | Both are readable in context |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | New table has companyId, workspaceId, domainId | BLOCKER | Tenant hierarchy is immutable. Missing IDs = no scoping possible. |
| 2 | domainId has a dedicated index (idx_{table}_domain) | CRITICAL | Hot-path queries filter by domainId. Missing index = full table scan. |
| 3 | Uniqueness constraints scoped per domain | CRITICAL | Global uniqueness breaks multi-tenant. Email unique per domain, not globally. |
| 4 | Soft-delete column present (deletedAt) | MAJOR | Hard deletes lose audit trail. Compliance requires recoverability. |
| 5 | Cascade rules explicit (ON DELETE SET NULL / RESTRICT) | CRITICAL | Default cascade can orphan or delete tenant data unexpectedly. |
| 6 | Migration handles existing data | CRITICAL | New NOT NULL column without default/backfill = migration failure. |
| 7 | JSONB columns have GIN index if queried | MAJOR | Custom field queries without GIN = sequential scan on JSONB. |
| 8 | Event/time-series tables use TimescaleDB hypertable | MAJOR | Append-only data without hypertable misses retention + aggregation. |
| 9 | Extension tables have recipientId FK + all 3 tenant IDs | BLOCKER | Shared recipient module demands full denormalization on extensions. |
| 10 | No unnecessary composite indexes (separate per query level) | MINOR | One big composite index wastes space and rarely matches query patterns. |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | Controller only extracts params and calls ONE service method | MAJOR | Business logic in controller = untestable, unreusable. |
| 2 | @Audit decorator on POST/PATCH/DELETE endpoints | CRITICAL | Compliance requires mutation audit trail. |
| 3 | No @Public() on protected endpoints | BLOCKER | Unauthenticated access to tenant data = security breach. |
| 4 | DTO validation on request body (@Body() dto: CreateDto) | CRITICAL | Unvalidated input = injection risk + data corruption. |
| 5 | Response shape matches API contracts ({ data }, { data, meta }) | MAJOR | Inconsistent response shapes break frontend contracts. |
| 6 | No direct database access from controller | BLOCKER | Controllers must go through service layer for tenant context checks. |
| 7 | Error responses use NestJS exceptions (not raw res.status()) | MINOR | Exception filters provide consistent error formatting. |
| 8 | SSE endpoint for long-running operations | MAJOR | Polling is wasteful. SSE provides real-time progress. |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | tenantContext resolved and checked | BLOCKER | Service without tenant context = cross-tenant data access. |
| 2 | Subscription limits enforced with additive check | CRITICAL | existing + incoming <= limit, not just incoming <= max. |
| 3 | Workspace scoping applied on cross-domain operations | CRITICAL | Dashboard queries must filter by workspaceId, not just domainId. |
| 4 | Async threshold: >1000 rows -> BullMQ job | CRITICAL | In-request processing of large datasets = timeout + blocked worker. |
| 5 | Job deduplication via BullMQ jobId | MAJOR | Duplicate jobs waste resources and can corrupt data. |
| 6 | Person de-duplication for recipient operations | MAJOR | UC2/UC3: same person across domains must not receive duplicate comms. |
| 7 | Company Library tier checked for shareable content | MAJOR | Content visibility: global vs company vs workspace vs cloned. |
| 8 | Settings inheritance resolved correctly | MAJOR | useCompanyDefaults + overriddenFields[] must be checked. |
| 9 | Error messages do not leak internal state | CRITICAL | Stack traces, SQL queries, or tenant IDs in error responses = info leak. |
| 10 | Cache invalidation after mutations | MAJOR | Stale cache after write = users see old data. |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | domainId is FIRST condition in WHERE clause | CRITICAL | Index scan requires domainId as leading condition. |
| 2 | No N+1 queries (JOIN or inArray, never loop) | CRITICAL | N+1 = O(n) queries instead of O(1). Kills performance at scale. |
| 3 | Batch operations for bulk updates | MAJOR | Row-by-row updates = slow + excessive DB connections. |
| 4 | Pagination on all list queries (limit + offset) | CRITICAL | Unbounded SELECT = OOM on large datasets. |
| 5 | and() used to compose filter conditions | MINOR | Consistent filter composition pattern across codebase. |
| 6 | Soft-delete filter applied (WHERE deletedAt IS NULL) | CRITICAL | Without soft-delete filter, deleted records appear in results. |
| 7 | Transaction used for multi-table writes | MAJOR | Partial writes without transaction = data inconsistency. |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | Types in types.ts within feature directory | MAJOR | Scattered types = import spaghetti + inconsistency. |
| 2 | No any type anywhere | MAJOR | any disables TypeScript's safety net entirely. |
| 3 | Zod schemas for form validation | MAJOR | Runtime validation catches what TypeScript cannot. |
| 4 | API response types match backend DTOs | CRITICAL | Type mismatch = runtime crash when backend changes. |
| 5 | Enum values constrained (union types, not free strings) | MINOR | 'active' | 'inactive' not string for status fields. |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | React Query for ALL server data | MAJOR | Server state in useState = no caching, no revalidation, no dedup. |
| 2 | Zustand ONLY for UI state | MAJOR | Mixing server data into Zustand = stale data, manual refetching. |
| 3 | No useState for API data | MAJOR | useState for server data = cache miss on every mount. |
| 4 | Query keys include tenant identifiers | BLOCKER | Missing workspaceId in key = cache collision across tenants. |
| 5 | Mutations invalidate correct query keys | CRITICAL | Wrong invalidation = stale list after create/update/delete. |
| 6 | Optimistic updates have rollback handlers | MAJOR | Failed optimistic update without rollback = ghost data in UI. |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | UnifiedDataTable for list views | MAJOR | Custom table markup = inconsistent UX, missing features. |
| 2 | Column definitions in separate file | MINOR | Inline columns clutter the component and prevent reuse. |
| 3 | Forms use react-hook-form + Zod | MAJOR | Manual form state = re-render storms + no validation. |
| 4 | Loading state present (skeleton/spinner) | MAJOR | Missing loading state = flash of empty content. |
| 5 | Empty state present (no data message) | MAJOR | Blank screen when empty = user thinks app is broken. |
| 6 | Error state present (API failure message) | CRITICAL | Unhandled error = white screen or cryptic error. |
| 7 | Toast on mutation success/failure (sonner) | MINOR | Silent mutations = user unsure if action worked. |
| 8 | Component under 200 lines (split if larger) | MINOR | Oversized component = hard to test, hard to reuse. |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | No fetch-all patterns (paginate or virtualize) | CRITICAL | Fetching 50k recipients client-side = browser crash. |
| 2 | Chunked processing for >1000 client-side items | MAJOR | Processing large arrays blocks the main thread. |
| 3 | No unnecessary re-renders (React.memo, useMemo where needed) | MINOR | Excessive re-renders = laggy UI on lower-end devices. |
| 4 | Images optimized (next/image, lazy loading) | MINOR | Unoptimized images = slow page load. |
| 5 | Dynamic imports for heavy components | MINOR | Loading chart library on every page = bundle bloat. |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | Progressive complexity applied (UC1 simple, UC3 full) | CRITICAL | UC1 user seeing MSSP controls = confusion + support tickets. |
| 2 | Workspace selector only when workspaceCount > 1 | MAJOR | Single-workspace user does not need a switcher. |
| 3 | Domain tabs only when domainCount > 1 | MAJOR | Single-domain user does not need domain tabs. |
| 4 | Person column visible for UC2+ only | MAJOR | Person is meaningless for UC1 (1:1 with recipient). |
These checks apply to EVERY PR. Multi-tenancy is not a feature flag; it is the foundation.
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | Every new query includes workspaceId or domainId filter | BLOCKER | Missing filter = cross-tenant data exposure. |
| 2 | tenantContext resolved from Redis (not DB join on every request) | CRITICAL | DB join per request = 2-5ms overhead on every hot path. |
| 3 | Subscription status checked (active or trialing only) | CRITICAL | Inactive subscription must block mutations. |
| 4 | Additive limit check on imports/bulk-create | CRITICAL | existing + incoming <= limit, prevent over-provisioning. |
| 5 | Person de-duplication for targeting operations | MAJOR | UC2/UC3: one person, multiple recipients = duplicate emails. |
| 6 | Company Library companyId on shareable content | MAJOR | Without companyId, content cannot be shared across workspaces. |
| 7 | Blueprint pattern for cross-workspace deployment | MAJOR | Manual replication across workspaces = inconsistency. |
| 8 | Settings inheritance: useCompanyDefaults + overriddenFields[] | MAJOR | Missing inheritance = MSSP admin must configure each workspace manually. |
| 9 | Progressive complexity in UI (hide UC3 features from UC1) | CRITICAL | Feature overload on simple customers = churn. |
| 10 | Admin permissions: Company Admin vs Workspace Admin scope | CRITICAL | Wrong permission check = privilege escalation. |
| 11 | Cross-workspace training credit considered | MAJOR | Person completes in WS-A, should get credit in WS-B. |
| 12 | Reports de-duplicate by Person (unique headcount) | MAJOR | Inflated counts from recipient-level reporting = inaccurate metrics. |
| 13 | Cache keys include tenant scope (domainId/workspaceId) | BLOCKER | Unscoped cache key = tenant A sees tenant B's cached data. |
| 14 | Redis key TTL set (no infinite-lived keys) | MAJOR | Missing TTL = Redis memory leak over time. |
| 15 | Workspace isolation test exists | CRITICAL | No test = no proof that isolation works. |
| 16 | Domain migration handled (rare but must not break) | MINOR | Domain moves between workspaces ~once/year. Denormalized IDs must update. |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | Input validation on every endpoint (DTO + Zod/class-validator) | CRITICAL | Unvalidated input = injection + crash + corruption. |
| 2 | SQL injection prevention (parameterized queries, no string concat) | BLOCKER | Raw SQL with user input = database compromise. |
| 3 | XSS prevention in user-generated content (sanitize HTML, escape output) | BLOCKER | Stored XSS = attacker runs JS in other users' browsers. |
| 4 | No @Public() on protected endpoints | BLOCKER | Unauthenticated access to tenant data. |
| 5 | Cross-workspace data leak: mismatched workspace header | BLOCKER | Request with workspace-A header accessing workspace-B data. |
| 6 | CORS configuration correct (no wildcard * in production) | CRITICAL | Wildcard CORS = any origin can make authenticated requests. |
| 7 | Rate limiting on auth endpoints | CRITICAL | No rate limit = brute force attacks on login/reset. |
| 8 | Secrets not committed (API keys, tokens, passwords) | BLOCKER | Secrets in git = compromised credentials. |
| 9 | File upload validation (type, size, content) | CRITICAL | Unrestricted upload = remote code execution risk. |
| 10 | Admin impersonation: max 1 hour, non-renewable, reason required | CRITICAL | Unbounded impersonation = abuse risk without accountability. |
| 11 | Audit log for every security-relevant action | CRITICAL | No audit = no forensics after incident. |
| 12 | Error messages do not leak stack traces or internal paths | MAJOR | Verbose errors = reconnaissance for attackers. |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | No unbounded queries (always LIMIT or pagination) | CRITICAL | SELECT * without LIMIT on 100k rows = OOM. |
| 2 | N+1 queries eliminated (JOIN or batch fetch) | CRITICAL | 100 recipients with 100 SELECT for extensions = 101 queries. |
| 3 | Bulk operations use batch processing (500 per batch) | MAJOR | Row-by-row processing = 100x slower than batch. |
| 4 | >1000 items -> BullMQ (not in-request) | CRITICAL | Long request = HTTP timeout + blocked worker thread. |
| 5 | Redis caching for read-heavy paths | MAJOR | DB hit on every read = unnecessary latency. |
| 6 | Cache invalidation correct (mutation clears related keys) | MAJOR | Stale cache = users see outdated data. |
| 7 | Database indexes match query patterns | CRITICAL | Missing index = full table scan on every query. |
| 8 | No SELECT * (select only needed columns) | MINOR | Extra columns = wasted I/O and memory. |
| 9 | Frontend: no fetch-all (paginate or virtualize large lists) | CRITICAL | Loading 50k rows client-side = browser freeze. |
| 10 | Frontend: heavy components use dynamic imports | MINOR | Chart/editor libraries in main bundle = slow initial load. |
| # | Check | Severity | Why |
|---|---|---|---|
| 1 | Processor extends BaseProcessor | CRITICAL | Skipping BaseProcessor = no job logging, no dead letter handling. |
| 2 | Job payload includes tenantContext | BLOCKER | Job without tenant context = cannot scope queries or enforce limits. |
| 3 | Job deduplication via jobId parameter | MAJOR | Duplicate job submission = double processing, data corruption. |
| 4 | Dead letter queue handling on final failure | MAJOR | Silent failure = lost jobs with no visibility. |
| 5 | Progress reporting via Redis key (job:{jobId}:progress) | MAJOR | No progress = user stares at spinner with no feedback. |
| 6 | Batch size appropriate (typically 500) | MINOR | Too small = overhead per batch. Too large = memory pressure. |
| 7 | Job retry configuration (typically 3 attempts with backoff) | MAJOR | No retry = transient failures become permanent failures. |
| 8 | SSE endpoint exists for job progress streaming | MAJOR | Client polling = wasteful. SSE = real-time progress. |
Summarize your findings by severity:
## Review Summary
- BLOCKER: 0
- CRITICAL: 2
- MAJOR: 4
- MINOR: 3
- NIT: 1
### Must fix before merge:
(none)
### Must fix before deploy:
1. [CRITICAL] Missing workspaceId filter in dashboard query (service.ts:42)
2. [CRITICAL] No pagination on recipient list endpoint (repository.ts:18)
### Should fix in this PR:
1. [MAJOR] Business logic in controller (controller.ts:25)
...
If there are BLOCKERs: Request Changes.
If there are CRITICALs only: Approve with comments (fix before deploy).
If only MAJOR and below: Approve.
If only NIT: Approve and explicitly say "All NITs are optional."
| Smell | What To Look For | Fix |
|---|---|---|
| Fat Controller | >20 lines in a controller method | Extract to service |
| Tenant Bypass | Query without workspaceId/domainId in WHERE | Add tenant filter |
| Sync Bulk | In-request loop processing >100 items | Move to BullMQ job |
| Raw SQL Concat | String interpolation in SQL | Use parameterized queries |
| Missing Audit | POST/PATCH/DELETE without @Audit | Add decorator |
| God Service | Service with >500 lines or >10 methods | Split by subdomain |
| Cache Miss | Read-heavy path without Redis caching | Add cache + TTL |
| Silent Failure | catch block that swallows error | Log or re-throw |
| Config in Code | Hardcoded URL, limit, or feature flag | Extract to config/env |
| Circular Dep | Service A imports Service B which imports Service A | Extract shared logic to a third service |
| Smell | What To Look For | Fix |
|---|---|---|
| useState for API Data | const [data, setData] = useState() with fetch in useEffect | Use React Query |
| Missing Error Boundary | Component with async data but no error handling | Add error state |
| Prop Drilling | >3 levels of passing the same prop | Use context or Zustand |
| Inline Styles | style={{ ... }} instead of Tailwind classes | Convert to Tailwind |
| Fetch in Render | API call inside component body (not in hook) | Extract to custom hook |
| Unkeyed List | .map() without stable key prop | Add unique key |
| Magic Strings | Hardcoded API URLs or route paths | Use constants |
| Giant Form | Form component >300 lines | Split into step components |
| No Debounce | Search input firing API on every keystroke | Add debounce (300ms) |
| Stale Closure | useEffect/useCallback with missing dependencies | Fix dependency array |
Certain types of changes require additional scrutiny. Apply these checks when the condition is met.
TrainingCompletionType.CROSS_WS_CREDIT consideredoverriddenFields[] updated when workspace overrides a fieldactorType: 'admin_impersonation'src/modules/{feature}/ structureThis is the minimum viable review. Every item MUST be checked.
Tenant Isolation (non-negotiable)
Data Integrity
Security
Quality
any typesObservability
UX (frontend PRs)
Customer Types
npx claudepluginhub boparaiamrit/build-second-brain --plugin build-second-brainProvides CDSS development patterns for drug interaction checking, dose validation, clinical scoring (NEWS2, qSOFA), and alert classification integrated into EMR workflows.