Code Review: от формальности к инструменту роста команды

# .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

// ❌ Плохо
function calculateDiscount(user, total) {
  if (user.isVIP) {
    return total * 0.2;
  }
  return total * 0.1;
}
 
// ✅ Комментарий на ревью:
// Что происходит, если total === 0 или user === null?
// Нужна валидация входных параметров:
 
function calculateDiscount(user, total) {
  if (!user || total <= 0) {
    throw new Error("Invalid input");
  }
 
  const discountRate = user.isVIP ? 0.2 : 0.1;
  return total * discountRate;
}

# ❌ Плохо (дублирование логики валидации)
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')
        # ...
 
# ✅ Комментарий на ревью:
# Логика валидации email дублируется.
# Предлагаю вынести в отдельный валидатор:
 
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'))
        # ...

# ❌ Плохо (N+1 query problem)
def get_users_with_orders():
    users = User.objects.all()
    result = []
    for user in users:
        # Каждая итерация = новый запрос к БД
        orders = Order.objects.filter(user_id=user.id).count()
        result.append({
            'user': user,
            'orders_count': orders
        })
    return result
 
# ✅ Комментарий на ревью:
# Классическая N+1 проблема. Для 1000 юзеров = 1001 запрос к БД.
# Используй annotate:
 
def get_users_with_orders():
    return User.objects.annotate(
        orders_count=Count('orders')
    ).all()
 
# Теперь: 1 запрос вместо 1001

# ❌ Плохо (SQL injection)
def get_user_by_email(email):
    query = f"SELECT * FROM users WHERE email = '{email}'"
    return db.execute(query)
 
# Атака: email = "' OR '1'='1"
# Запрос станет: SELECT * FROM users WHERE email = '' OR '1'='1'
# Результат: вернутся ВСЕ пользователи
 
# ✅ Комментарий на ревью:
# SQL injection уязвимость. Используй параметризованные запросы:
 
def get_user_by_email(email):
    query = "SELECT * FROM users WHERE email = ?"
    return db.execute(query, [email])

// ❌ Плохо
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;
}
 
// ✅ Комментарий на ревью:
// Непонятно, что делает функция. Предлагаю переименовать:
 
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;
}

# ❌ Плохо (только happy path)
def test_calculate_discount():
    user = User(is_vip=True)
    discount = calculate_discount(user, 100)
    assert discount == 20
 
# ✅ Комментарий на ревью:
# Нужны тесты на 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

## Описание изменений
 
<!-- Что делает этот PR? Зачем нужен? -->
 
## Связанные задачи
 
<!-- Closes #123 -->
 
## Тип изменений
 
- [ ] Bug fix
- [ ] New feature
- [ ] Breaking change
- [ ] Refactoring
- [ ] Documentation
 
## Чек-лист автора PR
 
- [ ] Код соответствует code style проекта
- [ ] Самоpревью проведено (прочитал свой код как ревьюер)
- [ ] Комментарии добавлены в сложных местах
- [ ] Документация обновлена (если нужно)
- [ ] Тесты добавлены/обновлены
- [ ] Все тесты проходят локально
- [ ] Нет breaking changes (или они задокументированы)
 
## Чек-лист для ревьюера
 
- [ ] Функциональность работает корректно
- [ ] Обработаны граничные случаи
- [ ] Нет архитектурных проблем
- [ ] Нет проблем с производительностью
- [ ] Нет security уязвимостей
- [ ] Код читаем и понятен
- [ ] Тесты адекватные
 
## Screenshots (если UI)
 
<!-- Добавьте скриншоты до/после -->
 
## Вопросы к ревьюерам
 
<!-- Есть неуверенность в каком-то решении? Спросите здесь -->

## Review comment:
 
Круто, что добавил обработку ошибок — раньше тут был голый запрос без проверок. 👍
 
Одна зона риска: сейчас retry logic делает 10 попыток без задержки.
Это может создать нагрузку на API при сбое.
 
Предлагаю добавить exponential backoff:
 
- 1я попытка: сразу
- 2я попытка: через 1 сек
- 3я попытка: через 2 сек
- и т.д.
 
Вот пример реализации: [ссылка на либу или код]
 
Как думаешь?

[CRITICAL] SQL injection уязвимость в строке 47.
Используй параметризованные запросы вместо f-string.

[MAJOR] N+1 query problem. Для 10k пользователей это будет 10k запросов к БД.
Предлагаю использовать select_related().

[MINOR] Переменная `data` слишком общее название.
Может быть `userData` или `profileData`?

[NITPICK] Лично мне больше нравится guard clause вместо вложенного if,
но это на вкус.

[QUESTION] Почему здесь используется setTimeout, а не Promise?
Хочу понять логику.

[PRAISE] Отличное решение с мемоизацией!
Не подумал бы про это сам.

[MAJOR] Потенциальная проблема с производительностью:
 
Цикл по всем пользователям (строка 23-27) будет работать O(n\*m),
где n = пользователи, m = заказы.
 
Для 10k пользователей с 100 заказами у каждого = 1M итераций.
 
Предлагаю:
 
1. Использовать JOIN вместо вложенных циклов
2. Или загружать только нужных пользователей с фильтром
 
Пример:
 
```python
# Вместо
for user in all_users:
    for order in user.orders:
        process(order)
 
# Сделать
orders = Order.objects.filter(
    user__is_active=True
).select_related('user')
 
for order in orders:
    process(order)
```

[QUESTION] Хочу понять логику:
 
Почему используется `setTimeout(..., 0)` вместо прямого вызова?
 
Возможно, я что-то упускаю, но кажется, что прямой вызов был бы проще.
Есть какой-то edge case, который я не вижу?

[PRAISE] Отлично сделано! Особенно понравилось:
 
1. Добавил comprehensive тесты (даже edge cases)
2. Рефакторинг error handling стал намного чище
3. Комментарии в сложных местах — очень помогают
 
Маленький нит: можно упростить строку 47 через destructuring,
но это детали.
 
Approved ✅

# Установка
npm install --save-dev husky lint-staged
 
# Инициализация
npx husky init

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

module.exports = {
  // TypeScript/JavaScript файлы
  "*.{ts,tsx,js,jsx}": [
    "eslint --fix", // Автофикс проблем
    "prettier --write", // Форматирование
    "vitest related --run", // Тесты связанных файлов
  ],
 
  // Python файлы
  "*.py": [
    "black", // Форматирование
    "isort", // Сортировка импортов
    "flake8", // Линтинг
    "mypy", // Type checking
    "pytest --testmon", // Тесты
  ],
 
  // 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:
  # Быстрые проверки (должны пройти за < 2 мин)
  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:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0 # Нужно для 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 проверки
  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 проверка
  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 анализ
  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 если есть функции с 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
 
  # Автокомментарии в 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 должен иметь описание
if (!danger.github.pr.body || danger.github.pr.body.length < 10) {
  fail("Пожалуйста, добавьте описание к PR");
}
 
// PR не должен быть слишком большим
const bigPRThreshold = 500;
if (danger.github.pr.additions + danger.github.pr.deletions > bigPRThreshold) {
  warn(
    `Большой PR (${danger.github.pr.additions + danger.github.pr.deletions} строк). Рассмотрите разбиение на несколько PR.`
  );
}
 
// Проверка, что изменения в package.json сопровождаются изменениями в package-lock.json
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 изменён, но package-lock.json нет. Запустите npm install."
  );
}
 
// Напоминание о тестах
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("Изменения в коде, но нет изменений в тестах. Добавьте тесты?");
}
 
// Проверка TODO комментариев
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(
    `Найдено ${todos.length} TODO/FIXME комментариев. Не забудьте их решить.`
  );
}

# Автоматическое генерирование описания PR
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)}'

// Скрипт для анализа (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}'}
 
    # Получить все PR за последний месяц
    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:
        # Получить комментарии к PR
        reviews = requests.get(
            pr['url'] + '/reviews',
            headers=headers
        ).json()
 
        # Считаем PR "отревьюенным", если > 2 комментариев
        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']
 
        # Рассчитать время на ревью
        created = datetime.fromisoformat(pr['created_at'])
        merged = datetime.fromisoformat(pr['merged_at'])
        review_time = (merged - created).total_seconds() / 3600
 
        # Получить количество комментариев
        comments = len(pr.get('comments', []))
 
        # Обновить 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
                }
            )