Code review as a mid-level dev

Why it matters

At junior level, code review is mostly something that happens to you. At mid level you are expected to review other people's pull requests (PRs) well: catch real bugs, spot accessibility and performance problems before users do, and do it in a way that helps the author instead of blocking them. Good reviewers make the whole team faster, and reviewing is one of the quickest ways to learn a codebase.

Before you read the diff

1. Read the description and linked issue. What problem is this solving? What is out of scope? A good PR says how to test it and includes screenshots for UI changes.
2. Check CI. If lint, types or tests are red, ask for them to be fixed first; do not spend time reviewing code that will change.
3. Check the size. A 2,000-line PR cannot be reviewed well. It is fair to ask for it to be split.
4. Run it if it is UI. Pull the branch or open the preview deploy. Many front-end bugs are only visible in a browser.

What to look for, in priority order

1. Correctness. Does it do what the issue asks, in every state?

• Loading, empty, error and success states all handled.
• Edge cases: zero items, very long text, a slow or failed request, a double click on submit.
• Race conditions: an older request finishing after a newer one and overwriting it.
• Data from the network validated; no assumptions that a field always exists.

2. Accessibility. Check it with the keyboard and the browser's accessibility tools, not by reading the code alone.

• Every interactive element is a real button, a, or form control, reachable with Tab and operable with Enter and Space.
• Visible focus is not removed; focus moves sensibly when a modal opens and returns when it closes.
• Inputs have labels; icon-only buttons have accessible names; images have meaningful (or empty) alt.
• Errors are announced (for example role="alert") and not shown by colour alone.
• Headings stay in order; motion respects prefers-reduced-motion.

3. Security and privacy. No secrets in front-end code (anything in the bundle is public), no dangerouslySetInnerHTML or innerHTML with user content, no tokens in localStorage if the app uses httpOnly cookies, no personal data in logs.

4. Performance. A new heavy dependency on every page, an image without dimensions (layout shift), an effect that refetches on every render, or a list of thousands of items rendered at once.

5. Tests. Are the new behaviours tested at a sensible level? Would the tests fail if the feature broke?

6. Maintainability. Naming, structure, duplication, and fit with the existing patterns of the codebase. This matters, but it comes after the user-facing issues.

Style (quotes, spacing, import order) should be the formatter's and linter's job. If you keep commenting on it, propose a lint rule instead.

Writing comments people can act on

• Be specific and explain why. "This button has no accessible name, so screen readers announce only 'button'. Could we add aria-label=\"Close dialog\"?" beats "a11y?".
• Label severity. Many teams use prefixes: blocking (must fix before merge), suggestion, question, nit (minor, optional). Authors then know what actually stops the merge.
• Ask, do not command, when you are not sure: "What happens if the request fails here?"
• Offer code with GitHub's suggestion feature for small fixes, so the author can apply it in one click.
• Comment on the code, not the person. "This function" not "you".
• Praise what is good. A clear test or a nice simplification is worth saying out loud.

Finish with a clear decision: approve, comment, or request changes, and a one-line summary such as "Looks good once the error state is handled; two nits are optional."

Worked example

A PR adds a "Delete comment" button. Reading the diff and running the preview, you find:

• blocking: the button is <span onClick>; it is not keyboard reachable. Use <button type="button">.
• blocking: deletion happens immediately with no confirmation and no undo. The design spec asks for a confirm dialog.
• question: if the DELETE request fails, the comment disappears anyway because the list is updated optimistically without rollback. Is that intended?
• suggestion: the new useDeleteComment hook has no test; a component test for the success and failure paths would cover it.
• nit: handleClick2 could be handleDelete.
• Praise: nice reuse of the existing ConfirmDialog in the other screen.

Decision: request changes, with the two blocking items called out at the top.

Being reviewed well

The same skills apply in reverse: keep PRs small and focused, write a description with testing steps and screenshots, review your own diff first, respond to every comment (fix it, or explain why not), and do not take feedback personally.

Common mistakes

• Approving without running UI changes.
• Fifty nit comments and no mention of the missing error state.
• Vague comments ("this is confusing") with no reason or suggestion.
• Blocking on personal preference that the codebase does not follow.
• Letting reviews sit for days; aim to respond within a working day.

Quick recap

• Read the context, check CI and run the change before reading the diff in detail.
• Review in priority order: correctness, accessibility, security, performance, tests, then maintainability. Leave style to tools.
• Write specific, kind, labelled comments and end with a clear decision.

Try it

In the Do tab you do a full review of a real pull request using this checklist.