Skip to main content

Review checklist

A short list of things to verify before a PR merges. Use as a pre-flight before clicking Approve (or as a self-review for solo PRs).

Drafted from planning · v0.1

Code​

  • No new framework dependency added without an ADR
  • Every mutation goes through withAudit (or has a deliberate-and-noted exception)
  • Every external API write uses an idempotency key
  • No console.log of PII (customer names, emails, phones, SMS bodies)
  • No raw SQL with values; parameterized only
  • No new tenant_id / shop_id column anywhere
  • Error handling: failures are loud where they need to be, quiet where they should be
  • If permissions are involved: gated on the correct screen and (where applicable) the correct role

Schema​

  • Migration is forward-only (no DROP)
  • New tables have indexes on FK columns
  • Money columns are *_cents INTEGER, not float
  • Booleans are INTEGER (0/1), not TEXT
  • Timestamps are TEXT ISO 8601, not Unix epoch
  • Foreign keys reference real columns
  • NOT NULL is the default; only nullable when truly optional

Bible​

  • Relevant entity / slice / lifecycle page updated
  • Cross-links present (added "See also" entries for any new affected page)
  • Diagrams updated if architecture changed (C4, sequence, state)
  • Frontmatter description is meaningful (it shows up in search)

Tests​

  • At least one happy-path test for new endpoint
  • At least one blocker / error-path test
  • Audit row is asserted on the test mutation (if mutating)
  • npm test passes locally
  • No skipped or xit'd tests

UI​

  • Server-rendered fallback exists (or this is genuinely JS-only and noted)
  • In-situ editing affordances follow the helm-editable chassis (dashed-underline cue; inline cell or composite-till modal)
  • No PII shown in error messages
  • Loading states are graceful (no full-page spinners)
  • Manual smoke: clicked through the affected screen in wrangler dev

Security​

  • No new secrets committed to git (check .gitignore covers .dev.vars, .env)
  • Auth required on endpoints that mutate
  • CSRF token used on state-changing forms (when slice 1 finishes wiring)
  • No eval(), new Function(), or dynamic SQL with user input

Observability​

  • New mutation endpoint emits a log line at end of request (via instrumentation, automatic)
  • Any new external call is timed and logged
  • Cost-bearing operations (AI calls, SMS sends) write to per-shop spend counters

Commit messages​

  • Subject line is imperative, ≤50 chars
  • Body explains why if not obvious
  • Co-author attribution if AI-assisted

When in doubt​

If you're not sure whether a thing is right, raise it explicitly in the PR description: "Not sure if X is right; would value a second look." That's better than self-merging and regretting.

See also​