# Code Review > Structured code review workflow for quality and consistency Use when this capability is needed. - Skill: `tomevault-io/code-review-258` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add tomevault-io/code-review-258` - Raw SKILL.md: https://api.skillmd.com/api/skills/tomevault-io/code-review-258/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Productivity - Author: tomevault-io (https://skillmd.com/u/tomevault-io) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/tomevault-io/code-review-258 --- # Code Review Skill Use this skill when reviewing code changes for quality, consistency, and best practices. ## When to Use - Reviewing pull requests - Pair programming sessions - Pre-merge code checks - Learning from others' code - Self-review before committing ## Review Process ### 1. Understand Context Before reviewing code: - Read the PR description/ticket - Understand the goal of the change - Check if there are related PRs ### 2. High-Level Review Ask these questions: - Does this solve the stated problem? - Is the approach reasonable? - Are there simpler alternatives? - Does it fit the existing architecture? ### 3. Detailed Review #### Code Quality ```typescript // ❌ Bad: Unclear naming const d = new Date(); const x = users.filter(u => u.a > d); // ✅ Good: Self-documenting const now = new Date(); const activeUsers = users.filter(user => user.lastActiveAt > now); ``` ```typescript // ❌ Bad: Magic numbers if (retries > 3) { ... } setTimeout(fn, 86400000); // ✅ Good: Named constants const MAX_RETRIES = 3; const ONE_DAY_MS = 24 * 60 * 60 * 1000; if (retries > MAX_RETRIES) { ... } setTimeout(fn, ONE_DAY_MS); ``` #### Function Design ```typescript // ❌ Bad: Function does too many things function processUser(user) { // Validate (20 lines) // Transform (15 lines) // Save (10 lines) // Send email (15 lines) // Log (5 lines) } // ✅ Good: Single responsibility function processUser(user) { validateUser(user); const transformed = transformUser(user); await saveUser(transformed); await notifyUser(transformed); logUserCreation(transformed); } ``` #### Error Handling ```typescript // ❌ Bad: Silent failure try { await riskyOperation(); } catch (e) { // nothing } // ✅ Good: Proper error handling try { await riskyOperation(); } catch (error) { logger.error('Operation failed', { error, context }); throw new AppError('Operation failed', { cause: error }); } ``` #### Type Safety ```typescript // ❌ Bad: Type assertions hiding issues const user = data as User; const value = (obj as any).property; // ✅ Good: Type guards and validation function isUser(data: unknown): data is User { return ( typeof data === 'object' && data !== null && 'id' in data && 'email' in data ); } if (isUser(data)) { // data is safely typed as User } ``` ### 4. Check Tests ```typescript // Tests should exist for: // - Happy path // - Edge cases // - Error conditions describe('calculateDiscount', () => { it('should apply 10% for orders over $100', () => { ... }); it('should return 0 for orders under $50', () => { ... }); it('should throw for negative amounts', () => { ... }); it('should handle zero amount', () => { ... }); }); ``` ### 5. Security Check - [ ] Input validation on user data - [ ] No secrets in code - [ ] SQL queries are parameterized - [ ] User output is escaped - [ ] Authentication/authorization verified ## Feedback Guidelines ### Be Specific ```markdown // ❌ Bad feedback "This code is confusing" // ✅ Good feedback "The variable `d` on line 45 is unclear. Consider renaming to `createdDate` to match the domain terminology." ``` ### Explain Why ```markdown // ❌ Bad feedback "Don't use `any` here" // ✅ Good feedback "Using `any` here disables type checking for `processData`. Consider using `unknown` with a type guard, or define a specific interface for the expected data shape." ``` ### Suggest Solutions ```markdown // ❌ Bad feedback "This function is too long" // ✅ Good feedback "This function is 80 lines and handles validation, transformation, and persistence. Consider extracting into smaller functions: ```typescript function processOrder(order: Order) { validateOrder(order); const enriched = enrichOrder(order); return saveOrder(enriched); } ```" ``` ### Use the Right Tone | Instead of | Use | |------------|-----| | "You should..." | "Consider..." | | "This is wrong" | "This might cause..." | | "Why did you..." | "What do you think about..." | | "You forgot to..." | "We might want to add..." | ## Comment Categories Use prefixes for clarity: | Prefix | Meaning | Action Required | |--------|---------|-----------------| | `blocking:` | Must fix before merge | Yes | | `suggestion:` | Improvement idea | No | | `question:` | Need clarification | Answer needed | | `nit:` | Minor style preference | No | | `praise:` | Good work! | None | ### Examples ```markdown blocking: This endpoint is missing authentication. Add the `authenticate` middleware before the handler. suggestion: Consider extracting this validation logic into a shared `validateEmail` function since it's used in 3 places. question: I'm not familiar with this library. What's the benefit over the standard approach we've been using? nit: Our style guide prefers `const` over `let` when the variable isn't reassigned. praise: Nice use of the builder pattern here! This makes the configuration much more readable. ``` ## Review Checklist ### Functionality - [ ] Code does what it's supposed to do - [ ] Edge cases are handled - [ ] Error handling is appropriate - [ ] No obvious bugs ### Code Quality - [ ] Code is readable and self-documenting - [ ] No unnecessary complexity - [ ] No code duplication - [ ] Follows project conventions ### Testing - [ ] Tests exist and are meaningful - [ ] Tests cover happy path and errors - [ ] Tests are not flaky - [ ] Coverage is adequate ### Performance - [ ] No obvious performance issues - [ ] Expensive operations are optimized - [ ] Database queries are efficient ### Security - [ ] Input is validated - [ ] Output is escaped - [ ] Auth is checked - [ ] No secrets exposed ## Review Output Template ```markdown ## Code Review: PR #123 ### Summary [Brief overview of what was reviewed] ### Blocking Issues 🔴 1. [Issue requiring fix before merge] ### Suggestions 💡 1. [Improvement idea] 2. [Another idea] ### Questions ❓ 1. [Clarification needed] ### Positive Notes ✅ - [What was done well] ### Verdict - [ ] Approved - [x] Request changes - [ ] Comment only ``` --- > Converted and distributed by [TomeVault](https://tomevault.io/claim/speedoa1) — claim your Tome and manage your conversions.