adam/shoc-frontend-new
1
0
Fork 0
mirror of https://github.com/Sea-Haven-Industries/shoc-frontend-new.git synced 2026-09-30 15:03:12 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2
shoc-frontend-new/REVIEW_AND_PR_FRAMEWORK.md
Alexandre Brandizzi 4337cc662b
Some checks are pending
CI / ci (push) Waiting to run
Details
CI / governance (push) Waiting to run
Details
Deploy / deploy (push) Waiting to run
Details
chore(governance): enforce frontend quality system (#53) 
* chore(governance): make React/TS conventions mandatory via executable gates

Add AGENTS.md, QUALITY_GATES.md, ARCHITECTURE_AND_CODE_QUALITY.md, and
REVIEW_AND_PR_FRAMEWORK.md as the binding conventions and PR review
contract for humans and all coding/review agents.

Add a single 'npm run verify' command (format + lint + build + test +
governance) and 'npm run governance', which runs a dependency-free godfile
ratchet (whole-repo, baseline in scripts/governance-baseline.json) and a
changed-file maintainability gate (complexity<=20, function<=150, params<=4,
depth<=4) via ESLint. Legacy is handled by ratchets, not relaxation: 5
godfiles over 500 lines are grandfathered debt; maintainability thresholds
apply to changed TS/TSX (72 legacy violations across ~51 files otherwise).

Add a repo-owned 'governance' CI job that runs 'npm run verify' so every
gate is guaranteed from this repository, independent of the org reusable
workflow.

* fix(governance): make frontend ratchets fail closed
2026-07-24 16:47:34 -03:00

3.9 KiB
Raw Blame History

REVIEW_AND_PR_FRAMEWORK.md — PR review contract

Every PR is reviewed against this contract, by humans and by review agents. The goal is high-signal review: catch real behavior, security, performance, and regression problems — not re-lint what the gates already enforce.

1. Review the exact head

  • MUST review the diff at the current PR head, not a stale checkout. Re-pull before reviewing if new commits landed; stale approvals are dismissed on push by branch protection.
  • MUST read the full diff of every changed file, including renames and generated/mapper code, not only the "interesting" components.

2. Board-backed regression inventory

  • MUST check the change against the Seahaven Jira SH board inventory — all visible tickets for this area, not only the current ticket. Behavior that is Done / QA-approved / released is protected scope.
  • MUST treat a plausible regression against protected behavior as a Blocker until disproven with repo evidence (the accepting tests, the linked PR/release, and a targeted check of the changed code paths).
  • MUST NOT approve if board access or the relevant inventory is missing — say so and block rather than infer a pass. A missing ticket link alone is not a blocker, but known acceptance criteria must still be traced.

The current Jira workflow has no Ready for QA transition. Merged work remains in the documented pre-QA status until QA evidence supports Done; do not invent a status or mark unverified work Done.

3. Behavior-based testing

  • MUST test through public behavior (rendered output, user interactions, query/mutation outcomes), not internal implementation details. Prefer @testing-library queries and user-event flows; assert what users observe.
  • MUST NOT add tests whose only purpose is to prove a tool (ESLint, the governance script) executes — validate tooling by running the real gates (npm run verify), not with assertion-free unit tests.
  • SHOULD cover the meaningful branches of new logic: the happy path, the error/empty state, and any boundary the change introduces.

4. Security review

  • MUST confirm no raw server error payloads, stack traces, or internal IDs leak to the UI (see ARCHITECTURE_AND_CODE_QUALITY.md §Security).
  • MUST confirm no secrets/tokens are introduced into the build, and that no VITE_* variable carries a secret (it is baked into the bundle).
  • SHOULD check untrusted input is validated (Zod) before use and that dangerously-set HTML / unescaped server strings are not introduced.

5. Performance and Big-O review

  • MUST flag algorithmic regressions in hot paths: O(n²)+ loops over server collections, re-filtering/sorting on every render, unbounded list rendering without virtualization.
  • MUST confirm TanStack Query keys are stable and that mutations invalidate the correct keys (no stale cache, no redundant refetch storms).
  • SHOULD question speculative useMemo/useCallback (add when measured) and unstable identities passed to memoized children.

6. No style-only comments

  • MUST NOT leave comments that only restate what Prettier or ESLint already enforces (formatting, naming nits the linter catches). Style is settled by the gates; review is for behavior, correctness, security, and architecture.
  • MUST make every comment actionable: tie it to a behavior, a risk, or an evidence-based convention in these docs, and offer a concrete fix or a targeted question. Use GitHub suggestion blocks when safe.

7. Review close-out

A review is complete when it records, briefly:

  1. Findings ordered by severity (Blocker / Needs-change / Suggestion), or "no findings".
  2. Open questions and their owner.
  3. Which of the above checks were run, and any that were skipped (and why).
  4. Residual risk, if approving.

Do not write a monolithic review body or a validation transcript into the PR surface; keep comments inline and high-signal.