Topic

Code review & feedback

Definition

Code review is a structured process where a change author submits a bounded diff for another person to evaluate against correctness, security, readability, and architecture before it merges, and constructive feedback frames issues as specific, actionable observations rather than judgments of the author.

In simpler words

Review is someone else checking a change against real criteria before it ships, and giving or receiving feedback well makes that process actually useful.

Use the same checklist mindset from this week when reviewing a teammate Nest PR — migrations, guards, DTOs, and contracts.

Open official example (opens in new tab)

After this you can

Write a PR description that helps a reviewer verify a change quickly
Give specific, actionable review comments instead of vague ones
Distinguish a blocking issue from a stylistic preference
Respond to review feedback without treating it as personal

Making a PR reviewable

Definition

A reviewable pull request describes intent, scope, and verification steps clearly enough that a reviewer can assess correctness without re-deriving the author reasoning from the diff alone.

In simpler words

Tell the reviewer what changed, why, and how it was checked, instead of making them reverse-engineer intent from raw code.

A good description states the problem, the approach, what was explicitly left out of scope, and how it was verified, including which requests or tests were run.

Small, single-purpose PRs get reviewed faster and more carefully than large ones.

PR description shape

## What
Add priority enum with a filter query param.

## Why
Support triage sorting.

## Verified
- Migration applied and rolled back cleanly
- POST/GET tested via Postman as member and admin
- OpenAPI reflects the new field

A reviewer can verify claims instead of guessing intent.

Giving and receiving feedback

Definition

Actionable feedback identifies a location, states a concrete consequence, and suggests a direction; receiving feedback well treats comments as evaluation of the code, not the author.

In simpler words

Say what is wrong, where, and why it matters — then respond to the technical point.

Label blocking issues (bugs, auth holes, missing migrations) vs non-blocking preferences.

Ask clarifying questions instead of guessing tone.

Keep in mind

Write the PR description a reviewer needs, not the one that is fastest to type.
Point at a specific line, name the consequence, and suggest a direction.
Label feedback as blocking or non-blocking so authors can prioritize.

Test

Check your understanding

At least 10 questions — mix of concept, syntax, practical, and logic. Score ≥80% (enforced by the API) to save progress.

Checking your session…

15 questions · concept 5 · syntax 3 · practical 4 · logic 3

1. What should reviews prioritize?

Concept

Formatting, import order, and consistent naming firstCorrectness, security, schema safety, and contract clarity with actionable evidenceTest coverage percentage above all other concernsLine count and file size to keep diffs small

2. Why call out missing guards?

Syntax

Authz bugs ship to production quicklyGuards are required for TypeScript to compile the routeMissing guards break the ValidationPipe chainGuards are what attach req.user for logging

3. Why review migrations carefully?

Practical

Migrations run only in development, so mistakes compoundDestructive or unindexed changes hurt shared databasesTypeORM cannot roll back a migration once generatedMigrations are applied in random order across instances

4. What makes feedback actionable?

Logic

A summary comment on the PR describing the overall qualitySpecific file/line behavior, risk, and a suggested fixA link to the style guide for the author to readA rating score with a short justification

5. Is compile-only approval enough?

Concept

Yes — if the type checker passes, inputs are validatedNo — missing validation/auth can still compileYes — a clean build means guards are wired correctlyYes — tsc rejects any unguarded route at build time

6. Should you rewrite the author’s PR entirely?

Practical

Prefer guiding the author; wholesale rewrites teach poorlyYes — pushing fixes directly is faster than commentingYes — rewriting guarantees the code meets the standardYes — for security bugs, always take over the branch

7. What is mass assignment risk in review?

Syntax

A DTO that rejects fields the client legitimately sendsDTOs allowing unexpected fields to hit persistenceAssigning one entity’s fields onto another entity typeSaving the same DTO to multiple tables at once

8. Why mention vague errors?

Logic

Vague errors leak stack traces to the clientUnstable error shapes break clients and debuggingVague errors always return a 200 status by mistakeVague errors bypass the global exception filter

9. What evidence helps a security comment?

Concept

Example request that bypasses UI checks or sends body.roleA screenshot of the hidden button in the UIThe TypeScript type of the DTO in questionA link to the OWASP page for the vulnerability class

10. Which review habit is worst?

Practical

Verifying that destructive routes have authorizationReviewing migrations for data-loss riskLeaving actionable comments with a suggested fixApproving any PR that compiles even if guards are missing

11. What should a reviewer require here?

Conceptadvanced

.where("t.title = '" + q + "'")

Escaping the quotes in q before concatenatingA bound parameter (:q) to prevent SQL injectionValidating that q is a non-empty string firstWrapping the query in a try/catch for bad input

12. What is the most important review comment on this route?

Syntaxadvanced

@Delete(':id')
remove(@Param('id') id: string) {
  return this.svc.remove(id);
}

The route should return 204 instead of the deleted rowThe id param needs a ParseUUIDPipe for validationThe service call should run inside a transactionThis destructive route has no authorization guard

13. How do you make this feedback actionable?

Practicalintermediate

// review comment: "this is wrong"

Ask the author to explain what they intendedTag a senior engineer to look at it insteadRequest that they add tests before you reviewCite the file/line, the risk, and a concrete suggested fix

14. Why block this migration in review?

Logicadvanced

await q.query('ALTER TABLE users DROP COLUMN email');

It is destructive and would lose data; require a safe, reviewed approachIt locks the users table for the entire migrationIt cannot be rolled back by TypeORM automaticallyIt renames the column instead of removing it

15. What review concern does dto: any raise here?

Conceptintermediate

update(@Body() dto: any) {
  return this.repo.update(id, dto);
}

It disables TypeScript’s null checks on the updateMass assignment: clients could set fields you did not intendIt forces a full entity reload after the updateIt skips the repository’s optimistic locking

Checking your session…