Код-ревью: как проверять pull request, писать комментарии и не тормозить команду

Как сделать code review полезным: цели ревью, размер PR по данным SmartBear и Google, иерархия проверок, разметка комментариев, сроки ответа, автоматические проверки, AI-ассистенты и метрики процесса.

Код-ревью легко внедрить формально и трудно — по-настоящему. Типичная картина: разработчик открывает pull request, коллега через десять минут ставит «LGTM», изменения уходят в main. Процесс есть, пользы почти нет: в лучшем случае поймана опечатка, в худшем — у команды появилась иллюзия контроля качества. Ниже — как превратить ревью в рабочий инструмент: какие цели оно решает, каким должен быть размер изменений, что проверять в первую очередь, как писать комментарии, как организовать сроки и что отдать автоматике и AI-ассистентам. Зачем нужен код-ревью: цели, которые стоит записать Прямой ответ: ревью нужно не только для поиска багов. Если команда не договорилась, чего ждёт от ревью, ревьюеры скатываются к самому простому — замечаниям по стилю — и пропускают то, что требует погружения. Полезно опереться на данные. В исследовании Microsoft Research «Expectations, Outcomes, and Challenges of Modern Code Review» (Альберто Баккелли и Кристиан Берд, ICSE 2013) разработчики называли главной целью ревью поиск дефектов, но на практике заметная доля комментариев касалась улучшения кода, альтернативных решений и передачи знаний. Авторы пришли к выводу, что эти эффекты — не побочные, а самостоятельная ценность ревью. Отсюда реалистичный список целей: Найти дефекты до продакшена — ошибки логики, уязвимости, необработанные граничные случаи. Распространить знания. Ревью — систематический способ добиться, чтобы каждый модуль понимал не один человек. Это страховка от «автобусного фактора». Сохранить архитектурную целостность. Мелкие компромиссы в каждом PR незаметно складываются в технический долг. Растить разработчиков. Хороший комментарий объясняет принцип, а не только правит строку. Держать единый стиль — наименее важная цель, и её почти целиком закрывают линтеры и форматтеры. Запишите цели в командный регламент ревью. Тогда спор «зачем ты придираешься» превращается в обсуждение по существу: соответствует ли замечание согласованным приоритетам. Размер pull request: главный фактор качества ревью Чем больше изменение, тем поверхностнее его проверяют. Это одна из самых устойчивых закономерностей в практике ревью. Самые цитируемые цифры — из исследования SmartBear на базе процесса ревью в Cisco Systems: около 2500 ревью, 3,2 млн строк кода, 50 разработчиков, 10 месяцев. Выводы опубликованы в книге SmartBear «Best Kept Secrets of Peer Code Review». Рекомендации оттуда: проверять не больше 200–400 строк за раз и не тратить на одну сессию больше 60–90 минут — дальше эффективность поиска дефектов падает. Учитывайте контекст: исследованию больше пятнадцати лет, его проводил производитель инструмента для ревью, а процесс в Cisco был достаточно формализован. Как точный норматив эти цифры использовать не стоит, как ориентир порядка величин — вполне. Современные данные показывают ту же тенденцию. В статье «Modern Code Review: A Case Study at Google» (ICSE SEIP 2018) проанализированы логи 9 млн проверенных изменений. Изменения в Google в большинстве небольшие, а медианное время ожидания первого ответа на маленькое изменение — меньше часа, на очень большое — около пяти часов. Практические правила размера: Одна задача — один PR. Не «добавил фичу и заодно отрефакторил соседний модуль». Механические изменения отдельно. Переименование, форматирование, перенос файлов — в свой PR без функциональных правок. Большую фичу — стеком. Серия последовательных PR: сначала модель данных, затем сервис, затем интерфейс. Незаконченная функциональность скрывается за флагом. Проверка одним предложением. Если суть PR не описывается одной фразой, его стоит разбить. Мягкий лимит можно автоматизировать: бот или action в CI помечает PR, превышающий заданный порог строк, и просит автора объяснить, почему его нельзя разделить. Что проверять: иерархия проблем Ревьюер должен идти по приоритетам сверху вниз, а не комментировать то, что первым бросилось в глаза. Уровень 1. Корректность и безопасность Делает ли код то, что должен? Нет ли уязвимостей? Если здесь есть проблемы, дальше можно не идти — PR возвращается автору. // Плохо: SQL-инъекция через подстановку строки const query = `SELECT * FROM users WHERE email = '${userInput}'`; // Хорошо: параметризованный запрос const result = await db.query('SELECT * FROM users WHERE email = $1', [userInput]); Что часто пропускают при беглом просмотре: гонки состояний в асинхронном коде и при параллельных запросах; незакрытые соединения, файлы, подписки; непроверенные входные данные и отсутствие проверки прав доступа на сервере; секреты в коде и в логах; граничные случаи: null, пустой массив, повторный запрос, недоступность внешнего сервиса. Уровень 2. Архитектура и дизайн Вписывается ли решение в принятую архитектуру? Не смешаны ли слои? Не появилась ли лишняя связанность? // Плохо: бизнес-логика знает об интерфейсе и сама шлёт письма async function processUserOrder(userId: string, items: Item[]) { const user = await db.getUser(userId); const total = items.reduce((sum, item) = sum + item.price, 0); if (total user.creditLimit) { showErrorModal('Превышен кредитный лимит'); // UI в бизнес-слое return; } await db.createOrder({ userId, items, total }); sendEmail(user.email, 'Ваш заказ оформлен'); // побочный эффект без абстракции analytics.track('order_created', { total }); // ещё одна ответственность } // Лучше: сервис возвращает результат и публикует событие class OrderService { constructor( private readonly users: UserRepository, private readonly orders: OrderRepository, private readonly events: EventBus, ) {} async createOrder(userId: string, items: Item[]): Promise OrderResult { const user = await this.users.findById(userId); const total = calculateTotal(items); if (total user.creditLimit) { return { success: false, error: 'CREDIT_LIMIT_EXCEEDED' }; } const order = await this.orders.create({ userId, items, total }); await this.events.publish(new OrderCreatedEvent(order)); return { success: true, order }; } } Письмо и аналитика подписываются на событие OrderCreated, а решение, как показать ошибку, принимает слой интерфейса. Уровень 3. Тесты Есть ли тесты на новое поведение? Покрывают ли они граничные случаи? Читаются ли как описание требований? Тест, который проверяет только «счастливый путь», стоит отметить так же, как баг. Подробно о том, какие тесты и на каком уровне писать, — в статье о тестировании ПО . Уровень 4. Читаемость и поддерживаемость Поймёт ли код другой разработчик через полгода? Отражают ли имена смысл? Объяснено ли в комментарии неочевидное решение — не «что делает код», а «почему так»? Принципы, по которым стоит оценивать читаемость, собраны в материале о чистом коде . Уровень 5. Производительность Проверяется, когда есть явный риск. Преждевременная оптимизация не нужна, но запрос к базе внутри цикла (N+1) или загрузка всей таблицы в память — это не оптимизация, а дефект. Уровень 6. Стиль и форматирование Последний приоритет, и в идеале — полностью автоматический: ESLint, Prettier, Stylelint, форматтеры языка. Человек не должен тратить внимание на пробелы и запятые. Как писать комментарии От качества комментариев зависит, станет ли ревью обучением или источником конфликтов. Размечайте степень важности Автор должен сразу видеть, что блокирует слияние, а что — пожелание. Готовый формат — Conventional Comments (conventionalcomments.org): комментарий начинается с метки, а при необходимости уточняется, блокирующий он или нет. issue (blocking): — проблема, без исправления мерж невозможен; suggestion: — конкретное предложение улучшить код; nitpick: — мелочь на усмотрение автора; question: — прошу объяснить, возможно, менять ничего не нужно; thought: — идея на будущее, не требует действий сейчас; praise: — удачное решение, которое стоит отметить. В руководстве Google по код-ревью (Google Engineering Practices) принята похожая практика: необязательные замечания помечаются префиксом «Nit:». Критикуйте код, а не человека Плохо: «Ты опять сделал неправильно». Хорошо: «Этот подход создаст проблему при росте числа заказов, потому что… Предлагаю рассмотреть вариант…» Объясняйте «почему» // Слабый комментарий: // Используй Map вместо объекта // Сильный комментарий: // suggestion: здесь объект используется как словарь, в который часто // добавляются и из которого удаляются ключи. Map для этого сценария // оптимизирован (так и сказано в документации MDN), хранит порядок вставки, // принимает ключи любого типа и не пересекается со свойствами прототипа. // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map Предлагайте альтернативу «Это неправильно» без варианта решения — наименее полезная обратная связь. Если лучшего решения вы не знаете, так и напишите: «Кажется, здесь проблема, но уверенного варианта у меня нет — давай обсудим». Читайте PR целиком до первого комментария Иначе получается бесконечный цикл: на втором круге ревьюер замечает то, что должен был увидеть на первом, и автор проходит три-четыре итерации вместо одной. Антипаттерны, которые разрушают процесс Нитпикинг вместо содержательного ревью. Пятнадцать замечаний об именах переменных и ни одного о синхронном HTTP-запросе в горячем пути. «LGTM» без чтения. Особенно у загруженных старших разработчиков. Junior не получает обратной связи, проблемы уходят в main. Ревью как соревнование. Снисходительный тон учит авторов избегать ревью и дробить изменения так, чтобы их было трудно понять. Проектирование архитектуры в комментариях к PR. Если нужен принципиальный спор о подходе, его выносят в дизайн-документ или созвон — и лучше до того, как код написан. Один ревьюер на всю команду. Узкое место, очередь PR и выгорание самого опытного человека. Организация процесса Кто проверяет Основной ревьюер — человек, лучше всех знающий затронутый модуль. Автоматически назначить его помогает файл CODEOWNERS в GitHub или GitLab. Второй ревьюер — по возможности тот, кто с модулем не работал: свежий взгляд и передача знаний. Для критичных изменений — безопасность, платежи, миграции данных — обязательное участие техлида. Ревью от младших к старшим стоит поощрять: junior задаёт вопросы и изучает паттерны, а свежий взгляд иногда ловит то, что автор перестал замечать. Сроки ответа Ревью «когда будет время» тормозит всю команду: готовый код ждёт, автор переключается на другие задачи и теряет контекст. В Google Engineering Practices сформулирован понятный ориентир: ответить на запрос ревью нужно максимум в течение одного рабочего дня, при этом не обязательно бросать текущую задачу — достаточно дойти до естественной паузы. Разумная схема для команды: обычный PR — первый ответ в течение рабочего дня; хотфикс — в течение пары часов, с отдельным каналом оповещения; большой PR — сроки обсуждаются заранее, а лучше его разбить. Систематическое нарушение сроков — сигнал руководителю о перегрузке или неверном распределении ревью, а не личная проблема разработчика. Асинхронно или синхронно Большинство ревью асинхронные. Переходить в созвон стоит, когда дискуссия в комментариях пошла по кругу, когда PR затрагивает архитектурное решение или когда автору нужно подробное объяснение сложных правок. Итог созвона коротко фиксируется в PR, чтобы контекст не потерялся. Автоматизация: освободите людей для важного Всё, что может проверить машина, должна проверять машина — до того, как PR увидит человек. Если хотя бы одна проверка не прошла, слияние заблокировано правилами защиты ветки. # .github/workflows/pr-checks.yml name: PR quality gates on: [pull_request] jobs: quality: runs-on: ubuntu-latest steps: - uses: actions/checkout@v6 - uses: actions/setup-node@v6 with: node-version: 24 cache: npm - run: npm ci - name: Lint run: npm run lint - name: Type check run: npm run typecheck - name: Unit tests run: npm run test:unit - name: Dependency audit run: npm audit --audit-level=high Что обычно отдают автоматике: форматирование и стиль — Prettier, ESLint; проверку типов — TypeScript, mypy; тесты и порог покрытия изменённого кода; размер бандла — Size Limit; уязвимости в зависимостях — npm audit, Dependabot, Snyk; статический анализ — SonarQube, GitHub