Code Review: From Formality to Team Growth Engine

# .github/workflows/pull-request.yml
name: PR Checks
on: pull_request
 
jobs:
  checks:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
 
      - name: Run linter
        run: npm run lint
 
      - name: Run tests
        run: npm test
 
      - name: Check coverage
        run: npm run test:coverage
        # Fail if coverage < 80%
 
      - name: Security scan
        uses: snyk/actions/node@master
 
      - name: Check formatting
        run: npm run format:check

// ❌ Bad
function calculateDiscount(user, total) {
  if (user.isVIP) {
    return total * 0.2;
  }
  return total * 0.1;
}
 
// ✅ Review comment:
// What happens if total === 0 or user === null?
// Need input validation:
 
function calculateDiscount(user, total) {
  if (!user || total <= 0) {
    throw new Error("Invalid input");
  }
 
  const discountRate = user.isVIP ? 0.2 : 0.1;
  return total * discountRate;
}

# ❌ Bad (validation logic duplication)
class UserService:
    def create_user(self, data):
        if not data.get('email'):
            raise ValueError('Email required')
        if not re.match(r'^[\w\.-]+@[\w\.-]+\.\w+$', data['email']):
            raise ValueError('Invalid email')
        # ...
 
    def update_user(self, user_id, data):
        if not data.get('email'):
            raise ValueError('Email required')
        if not re.match(r'^[\w\.-]+@[\w\.-]+\.\w+$', data['email']):
            raise ValueError('Invalid email')
        # ...
 
# ✅ Review comment:
# Email validation logic is duplicated.
# Suggest extracting to separate validator:
 
class EmailValidator:
    @staticmethod
    def validate(email):
        if not email:
            raise ValueError('Email required')
        if not re.match(r'^[\w\.-]+@[\w\.-]+\.\w+$', email):
            raise ValueError('Invalid email')
        return email.lower()
 
class UserService:
    def create_user(self, data):
        email = EmailValidator.validate(data.get('email'))
        # ...

# ❌ Bad (N+1 query problem)
def get_users_with_orders():
    users = User.objects.all()
    result = []
    for user in users:
        # Each iteration = new DB query
        orders = Order.objects.filter(user_id=user.id).count()
        result.append({
            'user': user,
            'orders_count': orders
        })
    return result
 
# ✅ Review comment:
# Classic N+1 problem. For 1000 users = 1001 DB queries.
# Use annotate:
 
def get_users_with_orders():
    return User.objects.annotate(
        orders_count=Count('orders')
    ).all()
 
# Now: 1 query instead of 1001

# ❌ Bad (SQL injection)
def get_user_by_email(email):
    query = f"SELECT * FROM users WHERE email = '{email}'"
    return db.execute(query)
 
# Attack: email = "' OR '1'='1"
# Query becomes: SELECT * FROM users WHERE email = '' OR '1'='1'
# Result: returns ALL users
 
# ✅ Review comment:
# SQL injection vulnerability. Use parameterized queries:
 
def get_user_by_email(email):
    query = "SELECT * FROM users WHERE email = ?"
    return db.execute(query, [email])

// ❌ Bad
function p(u, a) {
  if (u.t === 1 && a > 100) {
    return a * 0.15;
  } else if (u.t === 2) {
    return a * 0.25;
  }
  return 0;
}
 
// ✅ Review comment:
// Unclear what function does. Suggest renaming:
 
enum UserTier {
  BASIC = 1,
  PREMIUM = 2,
}
 
const BASIC_DISCOUNT_THRESHOLD = 100;
const BASIC_DISCOUNT_RATE = 0.15;
const PREMIUM_DISCOUNT_RATE = 0.25;
 
function calculateDiscount(user: User, amount: number): number {
  if (user.tier === UserTier.BASIC && amount > BASIC_DISCOUNT_THRESHOLD) {
    return amount * BASIC_DISCOUNT_RATE;
  }
 
  if (user.tier === UserTier.PREMIUM) {
    return amount * PREMIUM_DISCOUNT_RATE;
  }
 
  return 0;
}

# ❌ Bad (only happy path)
def test_calculate_discount():
    user = User(is_vip=True)
    discount = calculate_discount(user, 100)
    assert discount == 20
 
# ✅ Review comment:
# Need tests for edge cases:
 
def test_calculate_discount_happy_path():
    user = User(is_vip=True)
    assert calculate_discount(user, 100) == 20
 
def test_calculate_discount_zero_amount():
    user = User(is_vip=True)
    with pytest.raises(ValueError):
        calculate_discount(user, 0)
 
def test_calculate_discount_null_user():
    with pytest.raises(ValueError):
        calculate_discount(None, 100)
 
def test_calculate_discount_non_vip():
    user = User(is_vip=False)
    assert calculate_discount(user, 100) == 10

## Description of Changes
 
<!-- What does this PR do? Why is it needed? -->
 
## Related Issues
 
<!-- Closes #123 -->
 
## Type of Change
 
- [ ] Bug fix
- [ ] New feature
- [ ] Breaking change
- [ ] Refactoring
- [ ] Documentation
 
## Author's Checklist
 
- [ ] Code follows project code style
- [ ] Self-review conducted (read my code as a reviewer)
- [ ] Comments added in complex places
- [ ] Documentation updated (if needed)
- [ ] Tests added/updated
- [ ] All tests pass locally
- [ ] No breaking changes (or they are documented)
 
## Reviewer's Checklist
 
- [ ] Functionality works correctly
- [ ] Edge cases are handled
- [ ] No architectural issues
- [ ] No performance problems
- [ ] No security vulnerabilities
- [ ] Code is readable and clear
- [ ] Tests are adequate
 
## Screenshots (if UI)
 
<!-- Add before/after screenshots -->
 
## Questions for Reviewers
 
<!-- Uncertain about a decision? Ask here -->

## Review comment:
 
Great that you added error handling — before this was a naked request without checks. 👍
 
One risk zone: currently retry logic makes 10 attempts without delay.
This can create load on the API during failures.
 
Suggest adding exponential backoff:
 
- 1st attempt: immediate
- 2nd attempt: after 1 sec
- 3rd attempt: after 2 sec
- etc.
 
Here's an implementation example: [link to lib or code]
 
What do you think?

[CRITICAL] SQL injection vulnerability on line 47.
Use parameterized queries instead of f-string.

[MAJOR] N+1 query problem. For 10k users this will be 10k DB queries.
Suggest using select_related().

[MINOR] Variable `data` is too generic.
Maybe `userData` or `profileData`?

[NITPICK] I personally prefer guard clauses instead of nested if,
but it's a matter of taste.

[QUESTION] Why is setTimeout used here instead of Promise?
Want to understand the logic.

[PRAISE] Excellent solution with memoization!
Wouldn't have thought of it myself.

[MAJOR] Potential performance issue:
 
Loop over all users (lines 23-27) will be O(n\*m),
where n = users, m = orders.
 
For 10k users with 100 orders each = 1M iterations.
 
Suggest:
 
1. Use JOIN instead of nested loops
2. Or load only needed users with filter
 
Example:
 
```python
# Instead of
for user in all_users:
    for order in user.orders:
        process(order)
 
# Do
orders = Order.objects.filter(
    user__is_active=True
).select_related('user')
 
for order in orders:
    process(order)
```

[QUESTION] Want to understand the logic:
 
Why use `setTimeout(..., 0)` instead of direct call?
 
Maybe I'm missing something, but it seems a direct call would be simpler.
Is there an edge case I'm not seeing?

[PRAISE] Excellent work! Especially liked:
 
1. Added comprehensive tests (even edge cases)
2. Error handling refactoring is much cleaner
3. Comments in complex places — very helpful
 
Small nit: could simplify line 47 with destructuring,
but that's details.
 
Approved ✅

# Installation
npm install --save-dev husky lint-staged
 
# Initialization
npx husky init

#!/bin/sh
. "$(dirname "$0")/_/husky.sh"
 
npx lint-staged

module.exports = {
  // TypeScript/JavaScript files
  "*.{ts,tsx,js,jsx}": [
    "eslint --fix", // Auto-fix issues
    "prettier --write", // Formatting
    "vitest related --run", // Test related files
  ],
 
  // Python files
  "*.py": [
    "black", // Formatting
    "isort", // Sort imports
    "flake8", // Linting
    "mypy", // Type checking
    "pytest --testmon", // Tests
  ],
 
  // CSS/SCSS
  "*.{css,scss}": ["stylelint --fix", "prettier --write"],
 
  // Markdown
  "*.md": ["markdownlint --fix", "prettier --write"],
 
  // JSON/YAML
  "*.{json,yaml,yml}": ["prettier --write"],
};

# .github/workflows/pull-request.yml
name: Pull Request Checks
 
on:
  pull_request:
    types: [opened, synchronize, reopened]
 
jobs:
  # Quick checks (should pass in < 2 min)
  quick-checks:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
 
      - name: Setup Node.js
        uses: actions/setup-node@v4
        with:
          node-version: "20"
          cache: "npm"
 
      - name: Install dependencies
        run: npm ci
 
      - name: Lint
        run: npm run lint
 
      - name: Type check
        run: npm run type-check
 
      - name: Format check
        run: npm run format:check
 
  # Tests (can take longer)
  tests:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0 # Needed for coverage diff
 
      - name: Setup Node.js
        uses: actions/setup-node@v4
        with:
          node-version: "20"
          cache: "npm"
 
      - name: Install dependencies
        run: npm ci
 
      - name: Run tests
        run: npm run test:coverage
 
      - name: Coverage report
        uses: codecov/codecov-action@v3
        with:
          files: ./coverage/coverage-final.json
          fail_ci_if_error: true
 
      - name: Check coverage threshold
        run: |
          COVERAGE=$(cat coverage/coverage-summary.json | jq '.total.lines.pct')
          if (( $(echo "$COVERAGE < 80" | bc -l) )); then
            echo "Coverage is below 80%: $COVERAGE%"
            exit 1
          fi
 
  # Security checks
  security:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
 
      - name: Run Snyk security scan
        uses: snyk/actions/node@master
        env:
          SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}
        with:
          args: --severity-threshold=high
 
      - name: Check for secrets
        uses: trufflesecurity/trufflehog@main
        with:
          path: ./
 
  # Bundle size check
  bundle-size:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
 
      - name: Setup Node.js
        uses: actions/setup-node@v4
        with:
          node-version: "20"
 
      - name: Install dependencies
        run: npm ci
 
      - name: Build
        run: npm run build
 
      - name: Check bundle size
        uses: andresz1/size-limit-action@v1
        with:
          github_token: ${{ secrets.GITHUB_TOKEN }}
 
  # Complexity analysis
  complexity:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
 
      - name: Setup Node.js
        uses: actions/setup-node@v4
        with:
          node-version: "20"
 
      - name: Install dependencies
        run: npm ci
 
      - name: Analyze complexity
        run: |
          npx eslint . \
            --format json \
            --output-file eslint-report.json \
            || true
 
          # Fail if there are functions with complexity > 15
          COMPLEX_FUNCS=$(cat eslint-report.json | \
            jq '[.[] | .messages[] | select(.ruleId == "complexity")] | length')
 
          if [ "$COMPLEX_FUNCS" -gt 0 ]; then
            echo "Found $COMPLEX_FUNCS functions with complexity > 15"
            exit 1
          fi
 
  # Auto-comments in PR
  pr-comments:
    runs-on: ubuntu-latest
    needs: [quick-checks, tests, security]
    if: always()
    steps:
      - name: Comment PR
        uses: actions/github-script@v6
        with:
          script: |
            const status = '${{ needs.tests.result }}' === 'success' ? '✅' : '❌';
            const coverage = '${{ needs.tests.outputs.coverage }}';
 
            github.rest.issues.createComment({
              issue_number: context.issue.number,
              owner: context.repo.owner,
              repo: context.repo.repo,
              body: `## Code Review Checks ${status}\n\n- Coverage: ${coverage}%\n- All checks passed!`
            });

// dangerfile.ts
import { danger, warn, fail, message } from "danger";
 
// PR must have description
if (!danger.github.pr.body || danger.github.pr.body.length < 10) {
  fail("Please add a description to the PR");
}
 
// PR shouldn't be too large
const bigPRThreshold = 500;
if (danger.github.pr.additions + danger.github.pr.deletions > bigPRThreshold) {
  warn(
    `Large PR (${danger.github.pr.additions + danger.github.pr.deletions} lines). Consider splitting into multiple PRs.`
  );
}
 
// Check that package.json changes are accompanied by package-lock.json changes
const packageChanged = danger.git.modified_files.includes("package.json");
const lockfileChanged = danger.git.modified_files.includes("package-lock.json");
 
if (packageChanged && !lockfileChanged) {
  fail("package.json changed but package-lock.json did not. Run npm install.");
}
 
// Reminder about tests
const hasAppChanges = danger.git.modified_files.some((f) =>
  f.startsWith("src/")
);
const hasTestChanges = danger.git.modified_files.some(
  (f) => f.includes(".test.") || f.includes(".spec.")
);
 
if (hasAppChanges && !hasTestChanges) {
  warn("Code changes but no test changes. Add tests?");
}
 
// Check TODO comments
const todos = danger.git.modified_files
  .filter((f) => f.endsWith(".ts") || f.endsWith(".tsx"))
  .flatMap((f) => {
    const content = fs.readFileSync(f, "utf8");
    return content
      .split("\n")
      .map((line, i) => ({ line: i + 1, text: line }))
      .filter((l) => l.text.includes("TODO") || l.text.includes("FIXME"));
  });
 
if (todos.length > 0) {
  message(
    `Found ${todos.length} TODO/FIXME comments. Don't forget to address them.`
  );
}

# Automatic PR description generation
name: AI PR Description
on:
  pull_request:
    types: [opened]
 
jobs:
  generate-description:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
 
      - name: Generate PR description
        uses: github/copilot-pr-description@v1
        with:
          github-token: ${{ secrets.GITHUB_TOKEN }}

reviews:
  auto_review: true
  request_changes_workflow: false
 
  checks:
    - name: "Security"
      enabled: true
    - name: "Performance"
      enabled: true
    - name: "Best Practices"
      enabled: true
    - name: "Code Smell"
      enabled: true
 
  filters:
    - "!**/*.test.*"
    - "!**/node_modules/**"

# GitHub CLI
gh pr list --state all --json number,createdAt,reviews \
  --jq '.[] | select(.reviews | length > 0) |
       {pr: .number,
        time_to_review: (.reviews[0].submittedAt - .createdAt)}'

// Analysis script (Node.js)
const { Octokit } = require("@octokit/rest");
 
const octokit = new Octokit({ auth: process.env.GITHUB_TOKEN });
 
async function analyzeReviewThroughput(owner, repo) {
  const prs = await octokit.pulls.list({
    owner,
    repo,
    state: "closed",
    per_page: 100,
  });
 
  const merged = prs.data.filter((pr) => pr.merged_at);
 
  const throughputs = merged.map((pr) => {
    const created = new Date(pr.created_at);
    const merged = new Date(pr.merged_at);
    const hours = (merged - created) / (1000 * 60 * 60);
    return { pr: pr.number, hours };
  });
 
  const avg =
    throughputs.reduce((sum, t) => sum + t.hours, 0) / throughputs.length;
 
  console.log(`Average throughput: ${avg.toFixed(1)} hours`);
  console.log(
    `Median: ${median(throughputs.map((t) => t.hours)).toFixed(1)} hours`
  );
}

import requests
 
def analyze_review_coverage(owner, repo, token):
    headers = {'Authorization': f'token {token}'}
 
    # Get all PRs from last month
    prs = requests.get(
        f'https://api.github.com/repos/{owner}/{repo}/pulls',
        params={'state': 'closed', 'per_page': 100},
        headers=headers
    ).json()
 
    reviewed = 0
    for pr in prs:
        # Get PR comments
        reviews = requests.get(
            pr['url'] + '/reviews',
            headers=headers
        ).json()
 
        # Count PR as "reviewed" if > 2 comments
        if len(reviews) > 2:
            reviewed += 1
 
    coverage = (reviewed / len(prs)) * 100
    print(f'Review Coverage: {coverage:.1f}%')

# GitHub Webhook → Jira update
@app.route('/webhook', methods=['POST'])
def github_webhook():
    event = request.json
 
    if event['action'] == 'closed' and event['pull_request']['merged']:
        pr = event['pull_request']
 
        # Calculate review time
        created = datetime.fromisoformat(pr['created_at'])
        merged = datetime.fromisoformat(pr['merged_at'])
        review_time = (merged - created).total_seconds() / 3600
 
        # Get number of comments
        comments = len(pr.get('comments', []))
 
        # Update Jira
        jira_issue = extract_jira_issue(pr['title'])
        if jira_issue:
            jira.update_issue(
                jira_issue,
                fields={
                    'customfield_10050': review_time,  # Review Time
                    'customfield_10051': comments       # Review Comments
                }
            )