Code Review & Working on a Team

The last lesson ended with a pull request: a branch, a diff, comments, and checks in one place. Code review is what makes that page useful. It is how a team turns "my change works on my machine" into "we understand this change well enough to own it together."

Review the change, not the person

A review comment should be about code, behavior, maintainability, tests, or risk. It should not be a performance, a dunk, or a personality note.

Imagine pull request #14 from the last lesson:

Pull request #14
Title: Add refresh button to weather widget
Base: main
Compare: add-refresh-button
 
Files changed:
  src/renderWeather.js
  src/styles.css
 
Checks:
  lint       passed
  typecheck  passed
  test       passed

The diff includes a refresh button handler:

+refreshButton.addEventListener("click", () => {
+  renderWeather(latestWeather);
+});

Weak review comments often sound decisive but do not help:

Bad:
This is messy.
 
Bad:
Why would you do it this way?
 
Bad:
I don't like this.

The problem is not that they are "mean." The problem is that they give the author no clear path to a better change. A rigorous review is specific:

Good:
What happens if the user clicks Refresh before `latestWeather` is set?
Could we disable the button while loading, or guard inside the handler?
That would prevent `renderWeather(undefined)` from reaching the UI.
 
Good:
This adds the button behavior, but I don't see a test for the empty-data path.
Can we add one case for "refresh before data loads"?

Those comments are kind because they stay on the work. They are rigorous because they name a concrete behavior and a concrete risk.

What reviewers look for

Review is not just "spot the typo." A useful reviewer reads for several layers:

• Behavior: Does the code do what the pull request says it does?
• Edge cases: What happens with empty data, slow network, repeated clicks, bad input, or missing configuration?
• Maintainability: Will the next developer understand this shape six months from now?
• Tests: Do the tests cover the behavior that could break?
• Scope: Did this pull request stay focused, or did it mix unrelated work?
• Risk: Could this touch secrets, user data, performance, accessibility, security, or deployment behavior?

That list is why "LGTM" after a ten-second skim is not really review. Sometimes the right answer is approval because the change is small and clear. But the approval should still come from reading.

Reading other people's code is one of the most underrated developer skills. You learn patterns you would not have invented, you see how real systems age, and you build the muscle of understanding code without having written it. That is the same muscle you use when debugging production issues, joining a new team, or opening a library file to learn how an API behaves.

A practical reading order helps:

1. Read the pull request title and description.
2. Look at the changed file list before individual lines.
3. Read tests first if they explain the behavior.
4. Read the code path from input to output.
5. Leave comments only where action would improve the change.

You do not need to comment on every line to prove you reviewed it. Silence on a line can simply mean "this is fine."

Ask, suggest, require

Not every review comment has the same weight. Good teams make the difference visible.

Question:
Is `latestWeather` ever null here, or does the loader always set a fallback object?
 
Suggestion:
Could we name this `refreshWeatherButton`? There are now two buttons in this file.
 
Blocking:
This can call `renderWeather(undefined)` before the first response arrives.
Please add a guard or disable the button while loading before merge.

The "blocking" comment is not rude. It explains a concrete failure and the condition for moving forward. The "suggestion" comment leaves room for the author to accept, explain, or choose a different clear name.

As the author, read review comments as information, not as a verdict on your ability. If a comment is unclear, ask for the missing detail:

Author:
I see the risk. Would you prefer disabling the button in the loading state,
or a guard inside the click handler? I can do either.

That answer keeps the conversation attached to the code. It also helps reviewers be precise instead of drifting into taste.

Conventions prevent repeat arguments

Teams do not scale by debating every naming, formatting, folder, and testing choice from scratch. They write down conventions: shared decisions that make future changes easier to read.

A small style guide might say:

# Weather Widget Style Guide
 
- Use `src/ui/` for DOM event handlers.
- Keep data formatting helpers in `src/format/`.
- Prefer names that include the UI role: `refreshButton`, `statusMessage`.
- Add tests for loading, success, empty, and error states.
- Let Prettier own whitespace; do not request manual spacing changes in review.

Now review can point to the agreement instead of becoming personal preference:

Review:
This handler lives in `src/renderWeather.js`, but the style guide keeps DOM event
handlers in `src/ui/`. Can we move it to `src/ui/refreshButton.js`?

That comment is easier to receive because it is not "my style beats yours." It is "this project already chose a pattern; let's keep the codebase predictable."

Linters and formatters from Section 19 help here too. A reviewer should not spend human attention on semicolons, indentation, or quote style if a tool can enforce it. Save review energy for design, behavior, tests, and risk.

CODEOWNERS routes review

As projects grow, not every person can review every area well. CODEOWNERS is a plain file that maps paths to people or teams who understand those parts of the codebase.

# UI behavior and styles
/src/ui/       @weather-team/frontend
/src/styles/   @weather-team/frontend
 
# API and data contracts
/src/api/      @weather-team/api
 
# Tooling and dependency changes
/package.json  @weather-team/tooling
/vite.config.js @weather-team/tooling

The point is not gatekeeping. The point is routing. If a pull request changes src/api/weatherClient.js, someone familiar with API boundaries should look at it. If it changes package.json, someone who understands tooling and dependency risk should be pulled in.

CODEOWNERS also teaches a team what areas exist. A new developer can scan the file and learn, "frontend owns UI behavior, API owns data contracts, tooling owns build and dependency changes." That is organizational knowledge written down where code changes happen.

A review is finished when the risk is understood

Approval does not mean "this code is perfect." It means the reviewer understands the change, the important risks are handled, and the remaining tradeoffs are acceptable for this project.

A clean final review might look like this:

Reviewer:
Thanks for adding the loading guard and the empty-data test.
The refresh path is easier to follow now, and the style-guide move to `src/ui/`
keeps this consistent with the rest of the app.
 
Approved.

That is the real craft: specific feedback, clear resolution, no theater.

Next lesson adds automation to this human workflow. Reviewers should still think, but continuous integration can run linting, type checks, and tests on every push so humans do not have to catch the same mechanical failures again and again.