Pull requests and reviews
What a pull request must contain, how big it may be, the CI checks that gate it in each repository, who reviews what in a team of three, how fast, and how a PR is merged and cleaned up.
The PR template
hoco ships .github/PULL_REQUEST_TEMPLATE.md. Every field is filled in or deleted on purpose:
| Field | What to write | Example |
|---|---|---|
| Title | A Conventional Commit with the task id — it becomes the squash commit on dev | feat(server): reseller lifecycle transitions [SRV-14] |
| Summary | What and why in two to five lines, with the requirement ids | "Implements the reseller status machine of CRM-04…" |
| Task ID | The plan task(s) | SRV-14 |
| Related issues | Closes #n for every issue this fixes | Closes #42 |
| Changes | A bullet list a reviewer can follow file group by file group | — |
| Screenshots / recordings | For UI: light and dark; desktop and tablet widths for the back-office; a phone for mobile; RTL where relevant; a recording of at most 60 s for a flow | — |
| API contract changed? | No, or Yes: pnpm openapi:export run, label contract, Imad notified | — |
| Testing | Tests added; how you verified it by hand (environment, test account, steps) | "Staging, account commercial.admin@…, validated CMD-2026-000158" |
| Checklist | Conventions, pnpm check-types, pnpm lint, pnpm test, translations fr / ar / en, docs updated, no secrets | — |
hoco-mobile uses the same template (MOB-01), with API contract changed? replaced by Contract version used (the pinned hoco-v1.json version) and screenshots taken on a real device.
Labels on PRs (both repositories where they apply):
| Label | Meaning | Effect |
|---|---|---|
contract | Changes packages/contracts | Imad's approval required; changelog entry required |
mobile-impact | Changes the behaviour of an operation the apps call, without touching the contract (status codes, validation, idempotency, sync semantics) | Imad's approval required |
ui | Implements or changes a screen | Akram's design review |
migration | Contains a Prisma (or drift) migration | Reviewer reads the SQL; release notes mention it |
security | Auth, permissions, scopes, payments and credit, file access, secrets handling | Security checklist; walkthrough at the Wednesday sync |
release / hotfix | Release or hotfix PRs | Repository owner only |
Size limits
- Target 400 changed lines, hard limit 800, not counting generated files (the OpenAPI document,
hoco_api, lockfiles, Prisma-generated SQL,build_runneroutputs, snapshots). - One task per PR. A task may take several PRs; a PR never mixes two tasks' features.
- Over the limit: split it (contract → schema → endpoints → screens), or explain in the Summary why it cannot be split (a mechanical rename, a generated scaffold) and get the reviewer's agreement before the review starts.
- Open draft PRs early for contract and migration changes: the reviewer sees the shape before the code exists.
Required checks
A PR merges only with every required check green. A red check is never merged "because it is flaky": the test is fixed, or quarantined in the same PR with an issue number.
hoco (.github/workflows/ci.yml, extended in OPS-02):
| Check | Runs | Since |
|---|---|---|
Lint · Types · Contracts · Build | biome ci ., pnpm check-types, contract coverage (check-coverage), OpenAPI drift (re-runs pnpm openapi:export and fails if hoco-v1.json differs from the committed one), docs check-mdx, pnpm build | PRJ-01, OPS-02 |
Tests (PostgreSQL · Redis · MinIO) | Applies every migration to an empty PostgreSQL 17, fails if the schema has changed without a migration, seeds the reference and demo data, then runs the whole Vitest suite with Redis and MinIO, so no integration suite skips | OPS-02 |
Secret scan (gitleaks) | gitleaks over the full history | PRJ-01 |
| Contract breaking changes | oasdiff against the last contracts-v* tag, failing on an undeclared breaking change — not wired yet | SRV-03 |
The three named checks are the required ones (deployment runbook).
hoco-mobile (set up in MOB-01):
| Check | Runs | Since |
|---|---|---|
analyze | dart format --set-exit-if-changed, flutter analyze on every package | MOB-01 |
test | Unit and widget tests (melos run test) | MOB-01 |
build | Staging-flavor APKs of both apps | MOB-01 |
| API client | Regenerates hoco_api from the pinned document and fails on a diff | MOB-03 |
| gitleaks | Same as hoco | MOB-01 |
| Integration and golden tests | Ordering and visit flows; design-system goldens | MOB-15 |
Who reviews what
Each repository has one main developer, so most PRs are self-reviewed with the checklist below. A second person is required only where a mistake would cross a boundary: the contract, what the apps consume, security-sensitive mobile code, and design conformance.
| PR by | Touches | Reviewer | Blocking? |
|---|---|---|---|
| Anis | Anything not listed below | Self-review checklist | Anis merges once CI is green |
| Anis | packages/contracts (label contract), or the behaviour of an operation the apps call (label mobile-impact) | Imad | Yes — Imad's approval is required |
| Anis | Back-office or reseller web screens (label ui) | Akram, on the screenshots or the preview link | Yes, for design conformance: Akram comments "Design OK" (or "Design OK, follow-ups #n") |
| Anis | Label security | Self-review with the security checklist, then a 5-minute walkthrough with Imad at the Wednesday sync | Not blocking the merge; findings become issues |
| Imad | UI-only changes in hoco-mobile | Self-review checklist | Imad merges once CI is green |
| Imad | hoco_api, hoco_core (HTTP, auth, storage), hoco_offline, Android or iOS permissions and manifests, signing, CI | Anis | Yes |
| Imad | App screens (label ui) | Akram | Yes, as above |
| Imad | hoco — docs pages, contract proposals | Anis | Yes |
| Dependabot | Dependency updates | Anis (hoco), Imad (hoco-mobile) | Yes — merged within a week; a major version gets its own PR and its changelog is read |
If Akram cannot review a ui PR within one working day, the author merges and opens a design-qa issue for the review, so the flow never waits on one person.
Self-review checklist — the author ticks it in the PR before merging:
- I read the whole diff on GitHub, top to bottom, as a reviewer would.
- The PR title, task id and
Closes #nare right; the size is within limits. - Every item of what a reviewer checks that applies is satisfied.
- CI is green; I did not skip or weaken a check.
- The change still matches the contract, the requirement and the design it implements.
What a reviewer checks
| Area | Checks |
|---|---|
| Correctness | Meets the linked requirements and their acceptance criteria; edge cases and empty values; statuses and transitions exactly as in lifecycles and statuses |
| Contract | Matches packages/contracts; error codes from the registry; cursor pagination for apps, offset for the back-office; Idempotency-Key handled on mutations, required where flagged |
| Security | Permission key <module>.<action> on every operation; self and delegate scopes applied as query filters; no personal data or secrets in logs; private files only through signed URLs; input validated by the Zod schema |
| Data | Migration safety (expand then contract); indexes for new filters and foreign keys; the change and its OutboxEvent in the same transaction; money in integer centimes; timestamps UTC, business days Africa/Algiers |
| UX | Loading, empty, error, offline and no-permission states; French copy from the message files, no hard-coded strings; logical CSS properties (RTL-ready); keyboard and screen-reader basics |
| Tests | New rules have unit tests; endpoints have integration tests for the happy path, permission denied, out of scope and the main business errors |
| Docs | Specs, endpoint catalogue, API page or plan updated in the same PR when the change touches them |
Comments start with a prefix: blocking: (must change before approval), question:, suggestion:, nit:. Only blocking: prevents an approval; an approval with nits is normal.
Response times
Working hours, Sunday to Thursday (hours).
| Event | Expected within |
|---|---|
| First review of a normal PR | 1 working day |
First review of a contract PR, a hotfix or a fix on a release branch | 4 working hours |
| Re-review after changes | 4 working hours |
| The author answers review comments | 1 working day |
Akram's design review of a ui PR | 1 working day |
| A PR with no activity | 2 working days → raised at the stand-up; 5 → closed or re-planned |
Previews
| Surface | Preview | Set up in |
|---|---|---|
| Docs | check-mdx and the docs build in CI; reviewers run the site locally | OPS-02 |
| Back-office and reseller web | Dokploy preview deployment per PR into dev, against the staging API, link posted on the PR, removed when the PR closes | OPS-02 |
| Mobile | Staging-flavor APK attached to the PR as a CI artifact, plus screenshots | MOB-01 |
| Contract | The diff of hoco-v1.json in the PR, served locally by Prism (mock server) | SRV-03 |
Assumption — until OPS-02 sets up the web previews, ui PRs are reviewed on screenshots and recordings, and Akram checks the result on staging after the merge.
Merging and cleanup
- CI green, required approvals given, every conversation resolved.
- Squash-merge into
devwith the PR title as subject. The branch is deleted automatically. - Linked issues close through
Closes #n(devis the default branch). - The author checks the change on staging the same working day (automatic deploy); mobile changes in the next App Distribution build.
- A merged contract PR is announced in the team group: "Contract 1.N.0 merged — operations … — PR #…". Imad regenerates the client (the loop).
- The task is reported done in Thursday's report; Anis sets
statusanddoneOnintasks.json(progress reporting).
Git workflow
Branch model, naming, merge policy, Conventional Commits with the scopes of both repositories, hooks, short-lived branches and Prisma migrations — the same rules in hoco and hoco-mobile.
API contract workflow
Contract first: how an operation goes from a Zod schema in packages/contracts to the OpenAPI document, a Dart client, a mock server and an implementation — and how the contract changes without breaking the apps.