Code-Review in Pull Requests: Worauf achten

1. Wozu Code-Review eigentlich dient

Ein Pull Request, der nur auf Einrückung, Namenskonventionen und fehlende Leerzeichen geprüft wird, hat sein Potenzial verschenkt. Solche Formatfragen gehören in einen Linter oder eine automatische Formatierung, nicht in die Aufmerksamkeit eines menschlichen Reviewers. Code-Review entfaltet seinen Wert erst dort, wo Werkzeuge aufhören: bei der Frage, ob die Änderung tatsächlich das tut, was sie behauptet, ob sie an der richtigen Stelle im System ansetzt und ob sie in sechs Monaten noch verständlich ist.

Ein guter Reviewer fragt sich bei jeder Zeile, warum sie so und nicht anders geschrieben wurde. Das erfordert, den Kontext der Änderung zu verstehen, nicht nur die Diff-Zeilen isoliert zu lesen. In Magento-Projekten bedeutet das zusätzlich: Passt die Änderung zum bestehenden Modul-Zuschnitt, nutzt sie Service Contracts statt direkter Repository-Zugriffe, respektiert sie die Plugin-Architektur statt Core-Klassen zu überschreiben. Diese Fragen lassen sich nicht automatisieren, sie brauchen ein Review mit Absicht.

2. Logische Korrektheit und Edge Cases: den Diff mit Absicht lesen

Der Kern jedes Reviews ist die Frage, ob die Logik für alle relevanten Eingaben korrekt bleibt, nicht nur für den Happy Path, den der Autor getestet hat. Leere Arrays, null-Werte, negative Zahlen, doppelte IDs, gleichzeitige Requests: Diese Edge Cases werden im Diff selten explizit sichtbar, sondern müssen aktiv mitgedacht werden. Wer eine Methode liest, die eine Collection filtert, sollte sich fragen, was passiert, wenn die Collection leer ist oder wenn ein erwartetes Attribut fehlt.

Bevor man einzelne Zeilen bewertet, lohnt sich ein Blick auf den vollständigen Kontext der Änderung, nicht nur auf den isolierten Diff-Ausschnitt in der PR-Oberfläche. Mit git diff gegen den Basis-Branch und git log für die Commit-Historie versteht man, warum eine Datei geändert wurde und ob frühere Commits im selben PR bereits Fragen aufwerfen.

# Inspect a PR branch before reviewing line by line
git fetch origin pull/482/head:pr-482
git checkout pr-482

# Full diff against the target branch, not just the last commit
git diff main...pr-482

# See every commit in the PR, not just the squashed result
git log main..pr-482 --oneline --stat

# Function-context diff: see which method a change belongs to
git diff main...pr-482 -- src/Model/Cart.php --function-context

Diese Kommandos zeigen den PR so, wie er tatsächlich in den Basis-Branch einfließt, inklusive Zwischenschritte, die in der Weboberfläche eines PR-Tools oft zusammengefasst dargestellt werden. Wer nur die finale Diff-Ansicht im Browser liest, übersieht leicht, dass eine Variable in einem früheren Commit eingeführt und in einem späteren wieder umbenannt wurde, was die eigentliche Absicht der Änderung verschleiert.

3. Testabdeckung: Testet der PR das behauptete Verhalten

Eine Änderung ohne begleitenden Test ist eine Behauptung, keine Garantie. Der Reviewer sollte prüfen, ob die neuen oder geänderten Tests tatsächlich das Verhalten abdecken, das die PR-Beschreibung verspricht, statt nur den Code-Pfad auszuführen, ohne die Assertion scharf genug zu formulieren. Ein Test, der nur assertNotNull() aufruft, wo eigentlich ein konkreter Wert erwartet wird, täuscht Sicherheit vor, die nicht existiert.

Bei Bugfixes ist die wichtigste Frage, ob ein Regressionstest existiert, der ohne die Korrektur fehlschlagen würde. Fehlt dieser Test, kann derselbe Fehler beim nächsten Refactoring unbemerkt zurückkehren. Für Magento-Module heißt das konkret: Unit-Tests für ViewModels und Service-Klassen, Integrationstests für Repository-Interaktionen mit der Datenbank, und bei kritischen Zahlungs- oder Bestandslogiken zusätzlich ein Blick auf API-Tests, die den gesamten Request-Response-Zyklus abdecken.

Testabdeckung heißt nicht Codeabdeckung in Prozent. Ein Modul kann 90 % Line Coverage erreichen und trotzdem die kritischen Edge Cases ungetestet lassen, weil die Prozentzahl nur misst, welche Zeilen ausgeführt wurden, nicht ob die Assertions die richtigen Fragen stellen. Reviewer, die aktiv nach fehlenden Testfällen fragen, etwa "Was passiert hier, wenn der Warenkorb leer ist?", finden Lücken, die kein Coverage-Report anzeigt.

4. Scope Creep: einen PR auf ein Anliegen fokussiert halten

Ein Pull Request sollte eine einzige, klar benennbare Änderung enthalten. Sobald ein PR nebenbei Formatierung im gesamten Ordner überarbeitet, eine unabhängige Methode umbenennt oder eine zweite Funktion einführt, die mit dem eigentlichen Ticket nichts zu tun hat, wird er für den Reviewer unlesbar. Diese Vermischung heißt Scope Creep, und sie ist einer der häufigsten Gründe, warum Reviews entweder oberflächlich durchgewunken oder wochenlang liegen bleiben.

Ein PULL_REQUEST_TEMPLATE.md mit einer klaren Checkbox-Struktur zwingt Autoren dazu, den Umfang der Änderung explizit zu benennen, bevor der Reviewer überhaupt in den Code schaut.

## What does this PR do?
<!-- One sentence. If you need more, the PR is probably too big. -->

## Scope
- [ ] This PR touches exactly one concern
- [ ] No unrelated formatting or renaming changes included
- [ ] Follow-up work is tracked in a separate ticket, not bundled here

## Testing
- [ ] Unit tests added or updated
- [ ] Manually verified against staging

## Backward compatibility
- [ ] No breaking changes to public interfaces
- [ ] If breaking, deprecation path documented below

Wenn ein Reviewer Scope Creep entdeckt, ist die richtige Reaktion nicht, den PR pauschal abzulehnen, sondern konkret zu benennen, welcher Teil ausgelagert werden sollte: "Die Umbenennung von getPrice() zu getFinalPrice() ist sinnvoll, gehört aber in einen eigenen PR, damit dieser hier fokussiert auf die Rabattlogik bleibt." Das hält die Änderungshistorie im Git-Log nachvollziehbar und macht spätere git bisect-Läufe deutlich aussagekräftiger.

5. Sicherheit und Abwärtskompatibilität im Magento-Kontext

Sicherheitsrelevante Fragen gehören in jedes Review, unabhängig davon, ob der PR explizit als sicherheitsrelevant markiert ist. Bei Magento-Modulen heißt das konkret: Wird Nutzereingabe vor der Ausgabe mit escapeHtml() oder escapeHtmlAttr() behandelt, werden SQL-Queries ausschließlich über den QueryBuilder oder mit gebundenen Parametern erstellt statt mit String-Konkatenation, und prüft ein neuer Admin-Controller die passende ACL-Ressource in der acl.xml, bevor er sensible Daten preisgibt.

Abwärtskompatibilität ist die zweite Dimension, die in reinen Funktionstests oft übersehen wird. Eine Änderung an einem öffentlichen Interface, etwa das Hinzufügen eines Pflichtparameters zu einer Methode eines Service Contracts, bricht jedes Drittmodul, das dieses Interface implementiert oder aufruft. Der Reviewer sollte gezielt prüfen, ob eine Methode public, protected oder Teil eines Api/Interfaces ist, denn nur dort gilt Magentos strenge Rückwärtskompatibilitätspolitik.

Bei Preference-basierten Änderungen an Core-Klassen ist zusätzlich zu prüfen, ob nicht besser ein Plugin verwendet werden sollte, da Preferences mit jedem Core-Update kollidieren können, während Plugins deutlich robuster gegen Magento-Versionswechsel sind. Ein Reviewer, der diese Frage stellt, verhindert stille Breaking Changes, die erst beim nächsten Composer-Update im Live-Betrieb auffallen.

6. Konkretes vs. vages Feedback: Review-Kommentare mit Vorschlägen formulieren

"Das ist falsch" oder "Das gefällt mir nicht" sind Kommentare, die den Autor zwingen, beim Reviewer nachzufragen, was eigentlich gemeint ist, statt direkt weiterzuarbeiten. Ein Review-Kommentar sollte drei Dinge enthalten: was am Code problematisch ist, warum es problematisch ist, und im Idealfall einen konkreten Alternativvorschlag, der den Punkt ohne weitere Rückfragen umsetzbar macht.

# Vague, forces a round trip
# "this looks wrong, please fix"

# Actionable: names the problem, explains the risk, suggests a fix
# "getCustomerGroup() can return null for guest checkout (see
#  CustomerSession::getGroupId()). This will throw a TypeError
#  on line 42 since getGroupId() is typed as int. Suggest:
#  $groupId = $this->customerSession->getGroupId() ?? GroupManagement::NOT_LOGGED_IN_ID;"

Konkretes Feedback spart in Summe mehr Zeit, als es beim Schreiben kostet, weil es die Anzahl der Review-Runden reduziert. Ein Reviewer, der Codebeispiele statt reiner Beschreibungen liefert, macht es dem Autor leicht, den Vorschlag direkt zu übernehmen, statt die Lösung selbst zu erraten. Viele PR-Tools erlauben zusätzlich Vorschlags-Kommentare, die der Autor mit einem Klick übernehmen kann, was besonders bei kleinen, eindeutigen Korrekturen den Workflow beschleunigt.

7. Review-Etikette: nit vs. blocking, Ton und Kommentar-Präfixe

Nicht jeder Kommentar in einem Review hat dasselbe Gewicht, und diese Unterscheidung muss für den Autor sofort erkennbar sein. Konventionelle Präfixe wie nit: für kleine, optionale Stilfragen, question: für echte Verständnisfragen ohne Änderungsanspruch, und blocking: oder must-fix: für Punkte, die vor dem Merge zwingend gelöst werden müssen, schaffen diese Klarheit ohne zusätzliche Diskussion.

Der Ton eines Review-Kommentars sollte sich immer auf den Code beziehen, nie auf die Person. "Diese Methode wird an drei Stellen dupliziert" liest sich völlig anders als "Du hast diesen Code dreimal kopiert", obwohl beide dasselbe Problem benennen. Fragen statt Behauptungen ("Warum wird hier ein try-catch statt eines early return verwendet?") öffnen einen Dialog, während kategorische Aussagen ihn oft schon im Ansatz beenden.

Ebenso wichtig ist die Etikette in die andere Richtung: Ein Autor, der jeden nit-Kommentar als persönlichen Angriff wertet, macht zukünftige Reviews für das gesamte Team unangenehmer. Teams, die diese Konventionen in einer kurzen CONTRIBUTING.md dokumentieren, sparen sich wiederkehrende Missverständnisse und beschleunigen die Einarbeitung neuer Teammitglieder in die Review-Kultur.

8. PRs klein halten: Splitting, Stacked PRs und Draft PRs

Ein PR mit über 800 geänderten Zeilen wird statistisch seltener gründlich geprüft als ein PR mit 100 Zeilen, weil die kognitive Last für den Reviewer mit der Diff-Größe stark wächst. Die Konsequenz ist meist ein oberflächliches "LGTM" nach kurzem Überfliegen, statt einer echten Prüfung von Logik und Edge Cases. Die wirksamste Gegenmaßnahme ist, PRs von vornherein klein zu planen: ein Ticket, eine Änderung, ein überschaubarer Diff.

Wenn eine Funktion zwangsläufig mehrere aufeinander aufbauende Schritte braucht, etwa erst ein neues Interface, dann die Implementierung, dann die Integration in den Controller, bietet sich das Muster der Stacked PRs an: mehrere kleine, voneinander abhängige PRs, die jeweils auf dem vorherigen basieren statt alles in einem einzigen Mega-PR zu bündeln.

# Stacked PRs: branch B builds on branch A, not on main
git checkout -b feature/cart-api-interface main
# ... implement the interface, open PR #1 against main

git checkout -b feature/cart-api-implementation feature/cart-api-interface
# ... implement it, open PR #2 against feature/cart-api-interface

# After PR #1 merges into main, rebase the stack
git checkout feature/cart-api-implementation
git rebase --onto main feature/cart-api-interface feature/cart-api-implementation
git push --force-with-lease

Draft PRs sind das dritte Werkzeug gegen unnötig große oder verfrühte Reviews. Ein PR, der als Draft markiert ist, signalisiert dem Team, dass er noch nicht reviewbereit ist, aber trotzdem CI-Feedback und frühes Feedback zur Richtung ermöglicht. Sobald der Autor Änderungen nach einem Review umgesetzt hat, sollte er den Reviewer explizit erneut anfragen (re-request review), statt stillschweigend zu erwarten, dass jemand die neuen Commits von selbst bemerkt.

9. Git-Tools nutzen, um Kontext vor dem Review zu verstehen

Bevor man eine einzelne Zeile kommentiert, lohnt sich ein Blick auf die Geschichte der betroffenen Datei. git blame zeigt, wer eine Zeile zuletzt geändert hat und in welchem Commit, was oft erklärt, warum eine scheinbar seltsame Lösung tatsächlich einen guten Grund hatte, etwa einen früheren Bugfix, der genau dieses Verhalten erzwang.

# Understand why a line looks the way it does before questioning it
git blame -L 40,55 src/Model/Cart/TotalsCalculator.php

# Follow the line back through renames and refactors
git log --follow -p -- src/Model/Cart/TotalsCalculator.php

# Find the commit that introduced a specific line, with message
git log -S "roundingMode" --oneline -- src/Model/Cart/TotalsCalculator.php

Diese Werkzeuge verhindern eine häufige Review-Falle: einen Kommentar wie "Warum wird hier aufgerundet statt abgerundet?" zu schreiben, obwohl git blame in Sekunden zeigt, dass genau das vor drei Monaten in einem verlinkten Ticket zur Behebung eines Rundungsfehlers bei Steuerbeträgen entschieden wurde. Wer den Kontext zuerst prüft, stellt bessere Fragen und vermeidet, bereits diskutierte Entscheidungen erneut aufzurollen.

Die folgende Übersicht fasst die typischen Unterschiede zwischen unwirksamem und wirksamem Review-Verhalten zusammen, von der Formulierung einzelner Kommentare bis zur Größe des gesamten Pull Requests.

In der Praxis verstärken sich diese Punkte gegenseitig: Kleine PRs machen es leichter, konkretes statt vages Feedback zu geben, weil der Reviewer den vollständigen Kontext einer überschaubaren Änderung tatsächlich überblickt. Wer zusätzlich git blame und git log vor dem ersten Kommentar konsultiert, stellt bessere Fragen und beschleunigt den gesamten Review-Zyklus für das ganze Team.

10. Zusammenfassung

Code-Review in Pull Requests lohnt sich erst, wenn es über Formatierung hinausgeht. Logische Korrektheit und Edge Cases erkennt man, indem man den Diff mit Absicht liest und den vollständigen Kontext über git diff und git log versteht, statt sich auf die isolierte Ansicht im PR-Tool zu verlassen. Testabdeckung bedeutet, dass die Assertions tatsächlich das behauptete Verhalten prüfen, nicht nur eine Zeile Code ausführen. Scope Creep, Sicherheitslücken und Brüche der Abwärtskompatibilität sind die Punkte, die am häufigsten unentdeckt bleiben, wenn ein Review zu oberflächlich bleibt.

Der zweite Hebel liegt im Wie: konkretes statt vages Feedback mit Codebeispielen, klare Präfixe wie nit: und blocking: für die Gewichtung von Kommentaren, und ein sachlicher Ton, der sich immer auf den Code bezieht. Kleine, fokussierte PRs, notfalls als Stacked PRs aufgeteilt, und Draft PRs für frühes Feedback machen gründliche Reviews überhaupt erst realistisch. Wer diese Punkte konsequent kombiniert, verwandelt Code-Review von einer Formalität in einen echten Qualitätshebel.

11. FAQ: Code-Review in Pull Requests