Skip to content

PR reviews

How we propose, review, and merge changes. This page is the canonical version of our code-review guidelines (it replaces the old root CODE_REVIEW.md).

Branch and PR naming

Every change maps to a PPL Jira ticket (see Delivery).

  • Branch: PPL-NNNN/short-description — lowercase, hyphenated.
  • PR title / commits: type(PPL-NNNN): summary — Conventional Commits. commitlint enforces the type (feat, fix, chore, docs, refactor, test, …); the PPL-NNNN scope is our convention, not enforced (commitlint allows any scope), so add it consistently.

Do not rename a branch after the PR is open

Renaming after opening breaks the PR's head ref and the Jira ↔ GitHub link. Get the name right before you push and open.

How reviews work

  • PRs target develop (see Branching & releases).
  • Merge is squash into develop; the squash title keeps the type(PPL-NNNN): summary format.
  • CI must be green before merge — see GitHub workflows.

Approval rules

  • One approval is required to merge into develop. Any teammate's approval satisfies the gate.
  • CODEOWNERS approval is advisory, not mandatory. .github/CODEOWNERS auto-requests a reviewer from the owning team — @percihealth/backend for backend paths, @percihealth/frontend for the apps and shared packages — via GitHub's automatic review assignment, so exactly one team member is asked per PR rather than the whole team. You may re-request or add reviewers manually. There is no global owner, so root, docs/, and .github/ changes need only the one approval from any reviewer.
  • CI must be green. Two aggregate status checks are the required gates — Backend checks passed and Flutter checks passed. Each turns green when its stack's lint / typecheck / build / test jobs pass, or are skipped because the PR doesn't touch that stack — so a backend-only or docs-only PR isn't blocked waiting on Flutter (see GitHub workflows).
  • main is stricter. Only release/* and hotfix/* branches merge into main, and the Release PR Guard check enforces the branch prefix and the app version bumps (see Branching & releases). The one-approval and green-CI rules still apply.

Reviewer routing (GitHub config)

Routing is configured per team under Org → Teams → team → Settings → Code review assignment (in GitHub, not in this repo). Current settings:

  • Auto-assignment on, 1 reviewer, Load balance algorithm (round-robin is an equally fine choice — switch only if assignment feels lopsided).
  • Remove the team review request once an individual is assigned, and only notify requested members, so a PR pings one person rather than the whole team.
  • Count existing requests; skip members who are Busy or who committed to the PR.

Each team needs at least two assignable members

Auto-assignment skips the PR author, so a one-person team (frontend, today) can't route a reviewer for that author's own PRs. The tech lead is therefore a member of both @percihealth/backend and @percihealth/frontend so every PR has someone to assign — keep it that way until each area has two engineers.

For authors

  • Keep PRs small — focused, reviewable changes.
  • Write clear descriptions — what, why, and how. Link the PPL ticket.
  • Self-review first before requesting review.
  • Update the docs in the same PR when you change a documented behaviour or process.
  • Respond to feedback promptly; be open to suggestions.

For reviewers

  • Be constructive — focus on the code, not the person.
  • Be timely — aim to review within 24 hours.
  • Prioritise — separate blocking issues from nits.
  • Pull and test complex changes locally.
  • Treat a missing doc update like a missing test for process/behaviour changes.

Reviewer checklist

Functionality

  • [ ] Does the code do what it's supposed to, including edge cases and error conditions?
  • [ ] Is the logic correct and efficient, with no obvious performance issues?

Code quality

  • [ ] Readable, clearly named, well organised, no needless duplication (DRY)?
  • [ ] Follows the style guide and conventions in AGENTS.md?
  • [ ] Comments only where they explain a non-obvious why (no narration comments).

Testing

  • [ ] Appropriate unit tests, covering edge and error cases?
  • [ ] Integration / widget / E2E tests where needed, and all passing?
  • [ ] Adequate coverage.

Security

  • [ ] No vulnerabilities; inputs validated and sanitised.
  • [ ] Sensitive data, secrets, and API keys handled correctly.
  • [ ] Auth and authorization correct.

Documentation

  • [ ] Public APIs documented; breaking changes called out.
  • [ ] Relevant docs updated (this site, README, architecture docs).

Frontend (if applicable)

  • [ ] Accessible (WCAG), responsive, with loading and error states handled.
  • [ ] Null safety: prefer ?. over ! in build; guard async gaps with if (!mounted) return;.

Backend (if applicable)

  • [ ] Endpoints well designed and documented; rate limiting where needed.
  • [ ] Queries optimised; logging in place for debugging.
  • [ ] Validation via Zod schemas.

Dependencies & config

  • [ ] New dependencies necessary and version-pinned appropriately.
  • [ ] Config and environment variables documented.

Getting help

Questions about the review process or standards go in #tech.