Питання на співбесіді: Pull request і code review

Pull request (у GitLab - merge request) - це пропозиція влити зміни з однієї гілки в іншу. Навколо неї відбувається все обговорення: рев'ю коду, коментарі до рядків, результати CI, затвердження. Сам Git про pull request нічого не знає - це функція платформи (GitHub, GitLab, Bitbucket).

Навіщо потрібен опис: рев'юер бачить диф, але не бачить, навіщо зміна зроблена, які варіанти відкинуто і як її перевірити. Добрий опис економить кілька раундів запитань.

Шаблон опису:

## Що і навіщо
Додає експорт вакансій у CSV для адмінки. Менеджери зараз копіюють
таблицю вручну.

Closes #412

## Як зроблено
- `VacancyExport` формує файл потоково, щоб не тримати 50k рядків у пам'яті
- експорт іде в черзі, посилання приходить листом

## Як перевірити
1. Адмінка → Вакансії → «Експорт»
2. Дочекатися листа, відкрити файл

## На що звернути увагу
Не впевнений щодо формату дат - зараз ISO 8601.

Що робить опис добрим:

• проблема, а не перелік файлів - «що змінилося» видно в дифі, а «чому» - ні;
• посилання на задачу (Closes #412) - контекст і автоматичне закриття задачі;
• інструкція з перевірки - рев'юер може відтворити поведінку;
• скріншоти чи відео для змін інтерфейсу;
• ризики й відкриті питання - куди дивитися уважніше;
• заголовок як у доброго коміту - коротко про суть: «Експорт вакансій у CSV», а не «fix» чи «updates».

Шаблон для всієї команди - файл .github/pull_request_template.md: GitHub підставляє його в кожен новий pull request, і структура опису стає однаковою.

Докладніше в документації: GitHub: pull requests

Великий pull request рев'ю не проходить - він проходить «апрув». На 50 рядках рев'юер знаходить проблеми, на 2000 - гортає й пише «LGTM». Якість рев'ю падає разом з розміром.

Чому маленькі PR кращі:

• швидше рев'ю: 15 хвилин легко знайти між задачами, дві години - ні. Великі PR лежать днями;
• якісніше рев'ю: у фокусі одна ідея, помилки помітніші;
• менше конфліктів: гілка живе годину чи день, а не тиждень;
• простіший відкат: якщо щось зламалося, відкочується одна невелика зміна;
• зрозуміла історія: кожен PR - окремий логічний крок.

Орієнтир: до кількох сотень рядків змістовних змін. Згенеровані файли, lock-файли й міграції з даними рахуються окремо.

Як розбивати велику задачу:

• підготовчий рефакторинг окремо: перейменування, винесення класу, зміна сигнатури - без зміни поведінки, тому рев'ю швидке;
• шарами: спершу міграція й модель, далі сервіс з тестами, потім контролер і інтерфейс;
• за прапорцем функції (feature flag): незавершена функціональність потрапляє в main вимкненою, і можна вливати частинами;
• механічні зміни окремо від логіки: масове форматування чи перейменування в одному PR, нова поведінка - в іншому. Інакше справжня зміна губиться серед сотень рядків шуму.

Чого уникати:

• змішувати кілька непов'язаних змін «раз уже я тут» - кожна ускладнює рев'ю й відкат;
• розбивати так, що окремий PR не має сенсу й не проходить тести - кожен крок має лишати main робочим.

Якщо PR все ж великий - напишіть в описі, з чого почати читання, і проведіть рев'юера за ключовими файлами.

Докладніше в документації: GitHub: як допомогти рев'юерам

Draft (чернетка) - стан pull request, який означає «ще не готово до рев'ю». Чернетку видно команді, на неї працює CI, її можна коментувати, але:

• влити її не можна, доки автор не позначить її готовою (Ready for review);
• власників коду (CODEOWNERS) не запитують на рев'ю автоматично - запит надходить, коли PR стає готовим.

gh pr create --draft --title "Експорт вакансій у CSV"
gh pr ready 412        # позначити готовим до рев'ю

Коли відкривати draft:

• ранній відгук щодо підходу: «я збираюся зробити так - чи це правильний напрямок?» до того, як написано тисячу рядків;
• прогнати CI на реальному середовищі, поки робота триває;
• показати прогрес і позначити, що задача в роботі, - інші бачать гілку й не дублюють роботу;
• обговорити дизайн на конкретному коді, а не абстрактно.

Етикет:

• в описі чернетки варто написати, що саме хочете отримати: «подивіться лише на структуру сервісу, тести ще не готові»;
• не просити повного рев'ю чернетки - рев'юер витратить час на код, який ще зміниться;
• перед переведенням у Ready - самостійно переглянути диф, прибрати налагоджувальний код, оновити опис, переконатися, що CI зелений.

Альтернатива у старих процесах - префікс WIP: у заголовку. Draft кращий: це стан, який платформа розуміє й поважає (блокує злиття, не тривожить рев'юерів), а не домовленість, яку легко пропустити.

Докладніше в документації: GitHub: draft pull requests

GitHub розпізнає ключові слова в описі pull request (і в повідомленнях комітів) і пов'язує PR із задачею:

Closes #412
Fixes #418, resolves #420
Fixes acme/api#77

Ключові слова: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved. Регістр не важливий, двокрапка після слова допускається.

Що відбувається:

• задача показує, що над нею працюють, і містить посилання на PR;
• після злиття PR задача закривається автоматично;
• можна вказати задачу з іншого репозиторію: owner/repo#номер.

Важливий нюанс: ключові слова працюють, лише якщо PR спрямовано в гілку за замовчуванням (зазвичай main). PR у develop чи релізну гілку з Closes #412 не створить зв'язку й не закриє задачу. Для таких випадків зв'язок створюють вручну на бічній панелі PR (розділ Development).

Згадка без закриття: просто #412 чи «див. #412» створює перехресне посилання, але задачу не закриває. Це доречно, коли PR лише частина роботи:

Частина #412: міграція й модель. Інтерфейс - окремим PR.

Практичні поради:

• одна задача - одна причина PR: якщо PR закриває п'ять задач, це, мабуть, кілька різних змін;
• для задач з трекера поза GitHub (Jira, Linear) - номер задачі в назві гілки чи заголовку (ABC-123), а інтеграція трекера підхопить зв'язок;
• автопосилання (Settings → Autolink references) перетворюють ABC-123 у тексті на посилання на зовнішній трекер.

Докладніше в документації: GitHub: зв'язок pull request із задачею

Рев'ю на GitHub - це набір коментарів, які відправляються разом з підсумковим рішенням. Поки рев'ю не відправлене, коментарі видно лише вам.

Три варіанти завершення рев'ю:

Якщо в репозиторії налаштовано обов'язкове схвалення, Request changes блокує злиття, доки рецензент не змінить рішення чи рев'ю не відхилять (dismiss).

Коментарі до рядків прив'язуються до конкретного місця дифу - можна виділити кілька рядків і прокоментувати весь фрагмент.

Suggestion - запропонувати готову правку:

```suggestion
$vacancies = Vacancy::query()->with('company')->latest()->paginate(20);
```

Автор бачить диф пропозиції й натискає Commit suggestion - правка стає комітом у гілці PR. Кілька пропозицій можна застосувати одним комітом через Add suggestion to batch.

Коли suggestion доречний: друкарська помилка, перейменування, очевидний однорядковий фікс. Для складніших змін краще пояснити ідею словами - автор знає контекст краще.

Корисні звички:

• збирати коментарі в одне рев'ю (Start a review), а не відправляти по одному - автор отримує одне сповіщення, а не двадцять;
• позначати Viewed переглянуті файли - при оновленні PR GitHub покаже, які з них змінилися;
• Resolve conversation - коли питання закрите; зазвичай це робить той, хто його відкрив, або автор після виправлення;
• повторний запит рев'ю (Re-request review) після виправлень - рецензент отримає сповіщення.

Докладніше в документації: GitHub: рев'ю pull request

Головна мета рев'ю - покращити здоров'я кодової бази, а не знайти ідеальне рішення. Корисно йти від загального до деталей: немає сенсу обговорювати назву змінної в коді, який узагалі не варто вливати.

Порядок перегляду:

1. Опис і мета - чи зрозуміло, яку проблему розв'язує PR, і чи потрібна ця зміна взагалі;
2. Дизайн - чи правильне місце для коду, чи вписується в архітектуру, чи не надто складно;
3. Тести - чи покривають нову поведінку й крайні випадки, чи падали б вони без зміни;
4. Логіка й коректність - граничні умови, null, порожні колекції, конкурентний доступ, обробка помилок;
5. Безпека й продуктивність - авторизація, валідація вводу, N+1, запити в циклі, великі вибірки без пагінації;
6. Читабельність - назви, зрозумілість, коментарі там, де «чому» неочевидне;
7. Стиль - лише те, що не перевіряє автоматика.

На що звертати особливу увагу в Laravel:

• міграції - чи безпечні на великій таблиці, чи є відкат, чи сумісні зі старою версією коду під час деплою;
• масове присвоєння і $fillable, перевірка прав (Gate, політики) у контролерах і Livewire-діях;
• черги - ідемпотентність задач, що буде при повторі;
• запити - with() для зв'язків, індекси під нові where.

Що віддати автоматиці:

• форматування - Pint;
• типові помилки й типи - PHPStan/Larastan;
• тести - CI.

Людина не повинна писати «тут пробіл» - на це є інструменти, а увага рецензента дорожча.

Чого не робити:

• вимагати переписати під свій смак, якщо рішення автора теж нормальне;
• рев'ювити диф без контексту - іноді треба відкрити весь файл чи запустити гілку локально (gh pr checkout 412);
• затягувати: швидкий відгук важливіший за ідеальний.

Докладніше в документації: Google: що шукати під час code review

Текстовий коментар не передає інтонації: те, що рецензент вважав дрібною порадою, автор може прочитати як різку вимогу. Тому важливо явно позначати вагу коментаря і писати про код, а не про людину.

Conventional Comments - домовленість про формат коментарів: мітка на початку показує, чого саме чекає рецензент.

issue: тут N+1 - для кожної вакансії окремий запит до company.
Додай ->with('company') у запит вище.

suggestion (non-blocking): можна винести умову в scope `published()`,
вона повторюється в трьох місцях.

nitpick: `$data` -> `$vacancyAttributes`, так зрозуміліше.

question: чи може тут прийти порожній масив? Якщо так - впаде на array_key_first.

praise: дуже зручно, що експорт іде потоково, - пам'ять не росте.

Мітки й значення:

Принципи доброго коментаря:

• пояснювати чому: не «зроби інакше», а «так буде N+1, бо...»;
• питати, а не стверджувати, коли не впевнені: можливо, автор знає те, чого не знаєте ви;
• «ми» і «код», а не «ти»: «тут можна спростити», а не «ти ускладнив»;
• пропонувати рішення чи напрямок, а не лише вказувати на проблему;
• хвалити вдалі рішення - це теж інформація: що варто повторювати;
• не дублювати: однакова проблема в десяти місцях - один коментар «і так само нижче».

Автору: відповідати на кожен коментар (виправлено, не згоден - ось чому, винесу в окрему задачу) і не сприймати рев'ю особисто - рецензують код, а не людину.

Докладніше в документації: Conventional Comments

Кнопка злиття pull request на GitHub має три варіанти, і вони по-різному формують історію main.

Create a merge commit (git merge --no-ff):

*   Merge pull request #412 from acme/csv-export
|\
| * add tests
| * fix typo
| * export service
|/
* previous commit

• зберігаються всі коміти гілки й сам факт злиття;
• легко відкотити весь PR одним git revert -m 1 <merge>;
• історія з «fix typo» і «wip» потрапляє в main.

Squash and merge:

* Експорт вакансій у CSV (#412)
* previous commit

• усі коміти PR стискаються в один новий коміт;
• лінійна чиста історія: один PR - один коміт, легко шукати й відкочувати;
• проміжні коміти автора губляться (лишаються на сторінці PR);
• автор після злиття не може просто продовжити роботу в тій самій гілці - коміт у main має інший SHA, і наступний PR покаже старі зміни ще раз.

Rebase and merge:

• кожен коміт PR переноситься на main окремо, без коміту злиття - лінійна історія з усіма комітами;
• GitHub завжди створює нові SHA й оновлює дані комітера, навіть якщо можна було б просто перемотати гілку, і відкидає порожні коміти;
• доречно, коли автор ретельно впорядкував коміти й кожен має сенс сам по собі.

Як обрати:

Політика команди: у налаштуваннях репозиторію можна залишити лише один дозволений спосіб, щоб історія була однорідною. Для squash варто налаштувати, що заголовок коміту береться із заголовка PR, - тоді якість історії залежить від якості заголовків PR.

Докладніше в документації: GitHub: способи злиття

CODEOWNERS - файл, що визначає людей чи команди, відповідальні за частини репозиторію. Коли PR змінює їхні файли, GitHub автоматично запитує в них рев'ю.

Де лежить: .github/CODEOWNERS, корінь репозиторію чи docs/ - GitHub шукає в такому порядку і бере перший знайдений. Файл діє для тієї гілки, в якій лежить.

# За замовчуванням - уся команда бекенду
*                         @acme/backend

# Міграції - обов'язково команда даних
/database/migrations/     @acme/data

# Платежі - конкретні люди
/app/Billing/             @olena @acme/payments

# Фронтенд
*.vue                     @acme/frontend
/resources/js/            @acme/frontend

# Захистити сам файл і конфіги CI
/.github/                 @acme/leads

Синтаксис: шаблони як у .gitignore, останній збіг має пріоритет. Тому загальне правило * іде першим, а конкретніші - нижче. Якщо для файла останнє правило не містить власників, у нього їх немає.

Вимоги: власники мають мати права на запис у репозиторій; команда має бути видимою й теж мати права на запис.

Справжня сила - разом із захистом гілки: правило Require review from Code Owners (у branch protection чи ruleset) не дає злити PR, доки його не схвалить власник змінених файлів. Без цього CODEOWNERS - лише автоматичне запрошення.

Практичні поради:

• захистити .github/: інакше будь-хто може в PR змінити CODEOWNERS чи workflow CI і обійти правила;
• команди, а не люди, де можливо: людина йде у відпустку - рев'ю блокується;
• не робити власником усього одну людину - вона стане вузьким місцем;
• GitHub показує помилки синтаксису CODEOWNERS прямо в інтерфейсі файла - варто перевіряти після змін.

Докладніше в документації: GitHub: про власників коду

Перший рецензент PR - його автор. Перегляд власного дифу на GitHub (а не в редакторі) за кілька хвилин знаходить те, на що інакше пішов би раунд рев'ю.

Чеклист перед запитом рев'ю:

• переглянути весь диф у вкладці Files changed - так, як побачить рецензент;
• прибрати dd(), dump(), console.log, закоментований код, випадкові файли;
• CI зелений, тести на нову поведінку є;
• опис оновлено: що, навіщо, як перевірити;
• залишити власні коментарі до неочевидних місць: «тут свідомо без транзакції, бо...» - це зменшує кількість запитань;
• PR не містить сторонніх змін, що потрапили «заодно».

Як оновлювати PR після коментарів:

• нові коміти, а не переписування історії під час рев'ю: рецензент бачить лише те, що змінилося з останнього перегляду. Після force push GitHub губить зв'язок частини коментарів з кодом, і доводиться переглядати все заново;
• fixup-коміти зручні, якщо історію треба буде впорядкувати перед злиттям:

git commit --fixup=a1b2c3d
# перед злиттям, коли рев'ю завершене:
git rebase -i --autosquash main

А якщо репозиторій вливає через squash - впорядковувати взагалі не потрібно;

• відповідати на кожен коментар: «виправлено в 4f2e1a0», «не згоден, бо...», «винесу в #430»;
• не закривати чужі обговорення без відповіді - рецензент має бачити, що його почули;
• повторно запросити рев'ю (Re-request review), коли все виправлено, - інакше рецензент не дізнається, що черга знову за ним.

Якщо коментарів дуже багато - часто це сигнал, що варто поговорити голосом чи переглянути підхід, а не виправляти по одному.

Докладніше в документації: GitHub: запит на рев'ю

Захист гілки перетворює домовленості («не пушимо в main», «зливаємо лише після рев'ю й зелених тестів») на правила, які платформа примусово виконує.

Типовий набір правил для main:

• заборонити прямий push - зміни лише через PR;
• обов'язкове схвалення (1-2 рецензенти) і рев'ю власників коду (CODEOWNERS);
• скидати схвалення при нових комітах (dismiss stale approvals) - інакше після апруву можна дописати що завгодно;
• обов'язкові перевірки статусу (required status checks): тести, Pint, PHPStan мають бути зеленими;
• гілка має бути актуальною перед злиттям - або merge queue замість цього;
• заборонити force push і видалення гілки;
• розв'язані обговорення перед злиттям, за потреби - підписані коміти і лінійна історія.

Branch protection rules проти rulesets:

Обидва механізми працюють разом: застосовуються всі правила, що підходять.

Пастки:

• назва обов'язкової перевірки має збігатися з назвою job у CI. Перейменували job - PR зависає в очікуванні перевірки, яка ніколи не прийде;
• перевірка, що запускається не завжди (через paths у workflow), лишається «очікуваною» - PR не можна злити. Рішення - job, що завжди звітує успіхом, якщо змін немає;
• адміністратори за замовчуванням можуть обходити правила - варто вирішити свідомо;
• bypass для ботів (релізний бот) - лише конкретним GitHub Apps, не всім.

Правила як код: rulesets можна експортувати й імпортувати в JSON чи керувати ними через API й Terraform - однакові правила для десятків репозиторіїв.

Докладніше в документації: GitHub: про rulesets

Проблема «зелені окремо, червоні разом». PR A і PR B пройшли CI окремо, кожен відносно старого main. Їх злили один за одним - і main зламався: A перейменував метод, а B додав новий виклик старої назви. Конфлікту в Git немає, а код не працює.

Вимога «гілка має бути актуальною» це лікує, але в активному репозиторії перетворюється на гонку: кожне злиття робить усі інші PR застарілими, їх оновлюють, CI перезапускається, і хтось інший знову зливає першим.

Merge queue:

1. PR схвалений і зелений - автор додає його в чергу замість натискання Merge;
2. GitHub створює тимчасову гілку gh-readonly-queue/main/... з main + усіма PR, що стоять перед ним у черзі, + цим PR;
3. на цій комбінації запускаються обов'язкові перевірки;
4. перевірки пройшли - PR вливається; впали - PR вилучається з черги, а черга за ним перебудовується без нього.

Черга може перевіряти кілька PR групою паралельно - це прискорює потік, коли PR багато.

Що треба налаштувати в CI:

on:
  pull_request:
  merge_group:

Подія merge_group - окрема від pull_request і push. Якщо її не додати, обов'язкові перевірки для черги не запустяться, і злиття впаде через відсутній статус. Сторонні CI мають реагувати на push у гілки з префіксом gh-readonly-queue/.

Що змінюється для команди:

• спосіб злиття визначає черга, а не автор;
• main завжди зелений: те, що в нього потрапляє, перевірено саме в такій комбінації;
• час до злиття зростає на тривалість CI - тому швидкий CI стає ще важливішим.

Коли не потрібна: невелика команда, кілька злиттів на день - достатньо вимоги актуальності гілки. Merge queue окупається там, де PR вливаються десятками на день і зламаний main блокує всіх.

Докладніше в документації: GitHub: merge queue

Проблема: велику фічу розбили на маленькі PR, але кожен наступний залежить від попереднього. Чекати злиття першого, щоб почати другий, - повільно; складати все в один PR - втрачаємо переваги маленьких рев'ю.

Stacked PRs - ланцюжок залежних pull request-ів, де кожен спрямований не в main, а в гілку попереднього:

feat/ui      → PR #3 (base: feat/api)    ← верх
feat/api     → PR #2 (base: feat/schema)
feat/schema  → PR #1 (base: main)        ← низ
main

Кожен PR показує лише свій шар змін: рецензент дивиться міграцію окремо від API і окремо від інтерфейсу. Принцип: якщо код залежить від іншого коду, залежність має бути в тій самій гілці чи нижче.

Головна складність - rebase. Після виправлення в нижній гілці всі верхні треба перебазувати. Вручну це втомливо, але Git уміє оновлювати весь ланцюжок одразу:

git switch feat/ui
git rebase --update-refs main

--update-refs під час rebase верхньої гілки пересуває й проміжні гілки (feat/schema, feat/api) на переписані коміти. Можна увімкнути за замовчуванням: git config rebase.updateRefs true.

Після злиття нижнього PR наступний треба переспрямувати на main і перебазувати. Якщо низ вливали через squash, у верхніх гілках лишаються «старі» коміти, яких у main вже немає за SHA, - тут допомагає git rebase --onto main feat/schema feat/api.

Інструменти: GitHub має нативну підтримку стеків (на момент написання - у публічному попередньому перегляді) з каскадним rebase на сервері і розширенням gh stack; є також сторонні інструменти на кшталт Graphite чи ghstack.

Коли варто: довга фіча, яку природно ділити на шари, і швидкий потік рев'ю. Коли ні: незалежні зміни - їм не потрібен стек, достатньо окремих PR від main; повільне рев'ю - стек з п'яти PR, що висять тиждень, перетворюється на постійний rebase.

Докладніше в документації: GitHub: stacked pull requests

Незгоди - нормальна частина рев'ю. Проблема не в них, а в тому, що вони тягнуться днями в коментарях і псують стосунки.

Як розв'язувати незгоду:

1. розділити факти й смак. Баг, вразливість, порушення домовленостей команди - аргумент. «Я б написав інакше» - ні, якщо рішення автора теж коректне;
2. спиратися на спільні правила, а не на авторитет: стайлгайд, архітектурні рішення (ADR), домовленості команди. Якщо правила немає - це привід його створити, а не виграти суперечку;
3. перейти в розмову: після двох-трьох раундів коментарів 10 хвилин дзвінка вирішують більше, ніж ще десять повідомлень. Підсумок - записати в PR;
4. ескалація до техліда чи ширшого обговорення, якщо згоди немає, - це нормальний механізм, а не поразка;
5. «не блокує, але»: рецензент може погодитися злити зараз і створити задачу на покращення - якщо проблема не критична.

Принцип рецензента: схвалювати зміну, яка покращує стан кодової бази, навіть якщо вона не ідеальна. Вимога досконалості зупиняє розробку.

Як рев'ю стає вузьким місцем:

• PR чекають рев'ю по кілька днів, автори перемикаються між задачами й втрачають контекст;
• рев'ю робить одна людина;
• великі PR, які ніхто не хоче брати.

Що допомагає:

• домовленість про швидкість відгуку: перша реакція протягом робочого дня; не обов'язково повне рев'ю, але хоча б «подивлюсь після обіду»;
• рев'ю - пріоритетна робота, а не те, що роблять «коли буде час»: незлитий PR - незавершена робота;
• розподіл рев'юерів - автоматичне призначення команді за CODEOWNERS чи round-robin, щоб навантаження не падало на одних і тих самих;
• маленькі PR - їх рев'юють швидко;
• автоматика знімає з людей стиль і очевидні помилки;
• вимірювати час до першого відгуку й до злиття - без цифр проблема помітна лише відчуттям;
• парне програмування для складних змін - рев'ю відбувається під час написання.

Докладніше в документації: Google: як працювати з запереченнями в рев'ю