adam/shoc-frontend-new
1
0
Fork 0
mirror of https://github.com/Sea-Haven-Industries/shoc-frontend-new.git synced 2026-09-30 04:33:11 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2
shoc-frontend-new/REVIEW_AND_PR_FRAMEWORK.md
Adam Moussa c96a259365
Some checks failed
Deploy dev content / Deploy shoc-frontend-new-dev through Terraform (push) Has been cancelled
Details
refactor(cd): ship SPA content from GitHub on main (#220) 
* ci(cd): convert SPA hosting to handbook HCP and GitHub content CD

Give HCP the bucket and CloudFront with an empty origin path. GitHub owns
bucket-root sync and invalidation so merge-to-main and a human staging tag
can deploy without creating HCP runs. G13 fails PRs that mix terraform/
with deployable application files.

* ci: run Frontend checks and Terraform CI on PRs to main and dev

Match backend 148 so a PR targeting origin/dev still gets the required
checks. Push remains main only.

* refactor(terraform): keep live/dev and live/staging as HCP roots

Leave the adopted working directories in place so this CD PR does not
retarget two live HCP workspaces. Flattening stays a later change.

* style: prettier terraform-validate.mjs

* fix(terraform): pin githubdeploy assume-role policy in import checker

Reject controlled role updates whose trust document is not the rendered
GitHub OIDC policy, matching the bucket-policy pin.
2026-09-18 14:30:20 -04:00

4.6 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 NOT put Jira issue keys or ticket titles in source comments, JSDoc, or test names (for example (SH-183)). Ticket identity belongs in the PR, commit message, and branch — not in the code. Flag and request removal if a diff adds them.
  • 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.

Infra and application PRs stay separate. GitHub Actions owns SPA content (deploy-web.yaml). HCP Terraform owns the bucket and CloudFront. A change set that includes both terraform/ and deployable application files (src/, public/, pages/, config/, index.html, Vite/tsconfig, .env*, or scripts/deploy-web.sh) fails G13. Workflow, docs, and gate-script changes may travel with either side.