shoc-pr-review-runner/skills/pr-review/references/frontend-review-checklist.md
Adam Moussa c3cd8f7765
feat: SHOC PR review runner, phase 1
Manually-dispatched GitHub Actions workflow that reviews SHOC pull requests in
a clean environment: exact-head checkout of shoc-frontend-new and shoc-backend,
clean build/test gates, a truthful evidence report, a single-shot Fireworks
review, deterministic output validation, and published artifacts. The runner
never writes to the product repositories or their pull requests.

The review checklists move here from the reviewers' local Cursor commands so
the instructions live outside both product repos.

Phase 1 does not provision a database, start either application, or run live
browser flows; the evidence report records those as NOT_RUN so a review cannot
claim them.

Security architecture: building a PR executes its author's code, so the
workflow is split. The gates job runs that code holding no Fireworks key and
revokes its App token first; the review job holds the key, executes no product
code, and re-checks out this repo fresh. Product checkouts live outside the
workspace, the App token is downscoped at mint time, gate results fail closed
on any duplicate key, changed files are read from git objects rather than the
filesystem, and the validator re-checks every claim against the gate table.
2026-07-29 12:05:38 -04:00

40 KiB
Raw Blame History

PR Review

Review the specified pull request using the instructions and checklist below.

Do not post, approve, comment on, dismiss, or otherwise modify anything in GitHub. Return the completed review in chat only.

Write the review from the perspective of an experienced internal reviewer. The finished review should sound natural and specific to the PR, not like a checklist was converted into a template.

Do not claim that a command, test, page, component, API request, browser flow, or runtime scenario was checked unless it was actually checked.


Required Output Format

Use the following sections in this exact order.

1. Overall Verdict

Choose one:

APPROVE | REQUEST_CHANGES | COMMENT

Follow the verdict with one short, plain-language reason.

Examples:

  • REQUEST_CHANGES - The failed save path clears the user’s draft.
  • COMMENT - The frontend delta is clean, but the backend parent PR is not ready.
  • APPROVE - The implementation, production build, and affected browser flows are clean at the reviewed head.

Do not write a paragraph in this section.


2. Overall Review Comment

Write a concise review body that the user could paste directly into GitHub.

The review comment should:

  • State that the PR was reviewed or re-reviewed at the current 7-character abbreviated head SHA.
  • Summarize the actual state of the PR in natural language.
  • Clearly explain anything preventing approval.
  • Mention lint, TypeScript compilation, production build, tests, browser startup, or runtime results when they materially support the verdict.
  • Mention ticket linkage, CI, merge conflicts, stack order, backend readiness, or parent-PR readiness only when they affect the verdict.
  • Briefly acknowledge strong implementation choices when useful, especially when approving.
  • Avoid walking through every checklist item or summarizing every changed file.
  • Avoid using the same opening and closing language in every review.

Natural wording may include phrases such as:

  • Reviewed at <short-sha>.
  • Re-reviewed at <short-sha> after the latest update.
  • I did not find a new blocker introduced by this delta.
  • The remaining issue is isolated to...
  • I am withholding approval until the backend contract is ready.
  • Lint, the production build, and the affected tests are green at this head.
  • The project compiles, but the affected flow still fails in the browser.
  • The implementation looks clean overall, but...

These are examples, not mandatory phrases.

At most one non-blocking observation may be included at the end using:

Non-blocking: <brief note>

Omit non-blocking feedback unless it is genuinely useful.


3. Inline Comments

Include only defects that must be fixed before merge and directly support a REQUEST_CHANGES verdict.

Do not include:

  • Nits
  • Style preferences
  • Optional refactors
  • General praise
  • Speculative concerns without a reachable failure mode
  • Questions that do not require a code change
  • Issues inherited entirely from the base branch
  • Governance issues that cannot be fixed in the cited code
  • Duplicate comments describing the same underlying defect

Order comments by file path and then by ascending line number.

Use this format:

path/to/File.tsx:line - blocker

<Natural, direct explanation of the defect, the reachable failure, and why it matters.>

Fix: Test: <Focused regression test, component test, or browser scenario that would have caught the issue.>

The explanation does not need to begin with the same phrase every time.

Use “Requesting changes because...” when it reads naturally, but do not repeat it mechanically across every comment.

Each inline comment should:

  • Identify one concrete defect.
  • Explain the observable failure or material risk.
  • State how the failure can be reached.
  • Request a specific fix.
  • Request focused regression coverage.
  • Be ready to paste into GitHub without editing.
  • Avoid overstating theoretical risks that are not reachable in the current implementation.

If there are no blocking inline comments, write:

None.


Review Standard

Severity Threshold

Emit an inline comment only when at least one of the following is true:

  • The defect changes the verdict.
  • TypeScript compilation fails from a clean checkout.
  • The production build fails.
  • The application cannot start or render the affected route.
  • A reachable browser flow throws an unhandled runtime exception.
  • A page crashes, becomes unusable, or enters an unrecoverable state.
  • User-entered data can be lost or silently corrupted.
  • A visible action is nonfunctional or wired to a no-op.
  • The frontend sends an invalid request to the backend.
  • The frontend misinterprets a valid backend response.
  • Expected 4xx or 5xx responses are silently swallowed or shown as success.
  • The implementation does not satisfy the owning ticket’s acceptance criteria.
  • A required lint, build, test, browser, or end-to-end path is broken.
  • The defect must reasonably be fixed before this slice can merge.

Prefer fewer, stronger comments over complete checklist coverage.

Do not turn every imperfection into a blocker.

A successful production build alone is not enough to approve the PR. The affected behavior must also be checked in the browser where practical.


Separate Code Defects From Governance

Treat findings as separate categories.

Delta Defect

A concrete problem introduced, modified, or exposed by this PR.

Examples:

  • TypeScript compilation failure
  • Production build failure
  • Route crashes when opened
  • Component throws during render
  • Failed mutation clears the user’s input
  • API errors are silently swallowed
  • Frontend calls a route the backend does not provide
  • A button is visible but has no working handler
  • User-visible data is incorrectly transformed
  • Loading state never resolves
  • A successful response is treated as an error
  • A failed response is presented as success

A delta defect may justify REQUEST_CHANGES.

Inherited-Base Issue

A problem that already exists in the target branch or parent PR and is not introduced by this delta.

Inherited issues should be identified clearly, but should not be presented as though this PR introduced them.

Governance Issue

Examples:

  • Missing SH ticket
  • Required CI is missing or failing
  • Branch is conflicting
  • Incorrect stack order
  • Base branch changed after review
  • Backend producer PR is not ready
  • Parent frontend PR is not ready
  • Required dependency-review check is missing

Governance issues normally justify COMMENT, not inline blocker comments.

Only a concrete code or behavior defect should normally produce REQUEST_CHANGES.


Frontend Review Checklist

This checklist is a local review aid for shoc-frontend-new.

Do not commit it to the repository.

Suggested local location:

~/frontend-review-checklist.md

Backend companion:

~/backend-review-checklist.md

Use this checklist to investigate the PR.

Do not reproduce the checklist in the written review.


0. Anchor the Review

  • Review the exact current head.
  • Record the 7-character abbreviated commit SHA.
  • Re-pin the SHA when performing a re-review.
  • Treat any previous approval as stale when the head or base changes.
  • Read the PR title and description.
  • Read the linked SH ticket and acceptance criteria.
  • Read issue comments, submitted reviews, and unresolved inline threads.
  • Identify whether the PR is standalone or part of a stack.
  • Identify the corresponding backend PR when the feature depends on one.
  • Distinguish delta defects from inherited-base and governance concerns.
  • Do not rely only on the GitHub diff when surrounding code is needed to understand runtime behavior.

Always use:

git rev-parse --short=7 HEAD

Never include the full 40-character commit OID in outward-facing review copy.


1. Clean Checkout and Dependency Installation

Review from a clean checkout of the exact head whenever the environment allows it.

  • Remove or avoid relying on existing build artifacts.
  • Confirm the repository does not depend on untracked local files.
  • Install dependencies using the repository’s expected package manager and lockfile.
  • Confirm the lockfile is present and consistent with package.json.
  • Confirm dependency installation succeeds without manual local changes.
  • Confirm required generated files are present or reproducible.
  • Confirm the PR does not work only because of stale node_modules, Vite cache, coverage output, or local environment files.
  • Check whether new dependencies are actually declared.
  • Check whether removed dependencies are still imported.
  • Confirm package scripts referenced by the review instructions exist.

Use the repository’s intended install command, such as:

npm ci

A clean-install failure caused by the PR is a blocker.

A project that works only with stale or undeclared local dependencies should be treated as a clean-checkout failure.

Do not modify or commit repository files solely to make the review environment pass.


2. Linting and Static Analysis

Run the project’s lint and static-analysis gates against the exact reviewed head.

  • Run npm run lint.
  • Confirm all affected files are included in linting.
  • Check whether lint scripts silently ignore errors.
  • Review newly introduced warnings.
  • Check React hook dependency warnings.
  • Check inaccessible interactive elements.
  • Check unsafe any, non-null assertions, and ignored TypeScript errors when they hide reachable defects.
  • Check unused props, state, flags, handlers, and imports.
  • Check suppression comments such as eslint-disable, @ts-ignore, and @ts-expect-error.
  • Confirm suppression comments are narrow and justified.
  • Confirm generated files are excluded intentionally rather than masking source errors.

Run:

npm run lint

A lint failure is a blocker when lint is a required merge gate.

A warning should block only when it identifies a reachable correctness, accessibility, or runtime issue.

Do not convert every lint warning into an inline blocker.


3. TypeScript Compilation and Production Build

Compilation and the production build must be checked directly.

Do not assume editor diagnostics, development mode, or CI status are enough.

  • Run the repository’s TypeScript type-check command when one exists.
  • Run the production build.
  • Confirm all affected routes and imports are included in the production bundle.
  • Confirm test files do not hide source compilation failures.
  • Check unresolved imports and incorrect path aliases.
  • Check component prop and API response type mismatches.
  • Check generated API types or clients when used.
  • Check environment-variable access during the build.
  • Check dynamic imports and lazy-loaded routes.
  • Check case-sensitive import paths that may pass on macOS but fail in Linux CI.
  • Check circular dependencies when they cause initialization failures.
  • Review bundle warnings introduced by the PR when they indicate a broken import or runtime path.
  • Confirm the build does not depend on undeclared environment variables unless they are required and documented.
  • Confirm the repository’s deployment configuration can consume the generated output.

Run the relevant commands, such as:

npm run typecheck
npm run build

When there is no separate type-check script, confirm whether the production build performs TypeScript compilation.

A TypeScript compilation failure is a blocker.

A production build failure is a blocker.

A build that succeeds only in development mode but fails under the configuration used by CI or deployment is a blocker.

Do not report “build is green” unless the command actually completed successfully.


4. Unit and Component Tests

  • Run the full relevant Vitest suite.
  • Note test file and test counts when available.
  • Confirm tests run from the clean checkout.
  • Confirm newly added tests actually execute.
  • Investigate skipped, disabled, .only, or filtered tests relevant to the change.
  • Confirm tests do not pass only because assertions are too broad.
  • Confirm asynchronous tests await the behavior they claim to verify.
  • Check for false positives caused by swallowed promises, fake timers, or unhandled rejections.
  • Confirm mocks match the actual backend contract.
  • Confirm component tests cover the state transitions changed by the PR.
  • Confirm failure paths are covered when the change handles mutations or network requests.
  • Confirm each blocker fix includes focused regression coverage when reasonably possible.

Run:

npx vitest run

Or use the repository-defined test script:

npm test -- --run

Use the project’s actual script when it differs.

Do not require a new test merely to satisfy a formula. Request one when it would meaningfully prevent recurrence.

A test suite that cannot compile or start because of the PR is a blocker.

A failing test unrelated to the PR should be identified separately and not misrepresented as a delta defect.


5. Application Startup and Route Rendering

A successful production build does not prove the application works in the browser.

Start the application when practical.

  • Start the frontend using the repository’s expected development command.
  • Confirm the development server starts without errors.
  • Confirm the root application renders.
  • Navigate directly to each affected route.
  • Refresh each affected route to catch routing and hosting issues.
  • Confirm lazy-loaded components resolve.
  • Confirm route guards do not create loops or blank screens.
  • Confirm providers and context dependencies are mounted.
  • Check configuration and environment-variable initialization.
  • Confirm the affected page does not crash before data loads.
  • Confirm loading, empty, success, and error states render.
  • Check whether development-only behavior differs from the production build.
  • Preview the production build when the repository supports it.

Run the appropriate commands, such as:

npm run dev

When available:

npm run preview

Examples of startup or rendering blockers:

  • Development server does not start.
  • Application renders a blank page.
  • A route throws during initial render.
  • A provider or hook is used outside its required context.
  • A lazy import resolves to the wrong export.
  • Direct navigation to the changed route returns an unusable page.
  • Required environment configuration is read incorrectly.
  • Route guard redirects indefinitely.

Do not claim browser runtime validation was completed if the application was never started.


6. Browser Runtime Execution

Compilation and tests are not sufficient when the changed behavior can be exercised locally.

Execute the affected user flow where practical.

  • Identify each user-visible flow changed by the PR.
  • Exercise the normal success path.
  • Exercise relevant invalid-input paths.
  • Exercise empty, loading, error, and retry states.
  • Exercise not-found, unauthorized, and conflict states where applicable.
  • Confirm buttons, menus, links, dialogs, forms, and keyboard actions work.
  • Confirm visible actions have mounted and reachable handlers.
  • Confirm forms submit the intended values.
  • Confirm failed submissions preserve the user’s work.
  • Confirm successful submissions update or invalidate the correct data.
  • Confirm repeated clicks do not create duplicate operations.
  • Confirm loading states prevent accidental duplicate actions where needed.
  • Confirm modals and drawers can be opened and closed.
  • Confirm navigation after a successful action is correct.
  • Confirm browser refresh does not lose state that should be URL-driven or persisted.
  • Confirm read-only users can still access required read paths.
  • Confirm disabled controls are actually disabled and not merely styled as disabled.
  • Confirm no unhandled exception appears while using the flow.

A reachable browser crash or unusable user flow is a blocker even when lint, build, and tests are green.

Do not approve a change solely because automated tests pass if the affected flow demonstrably fails in the browser.


7. Browser Console and Runtime Errors

Review the browser console while exercising affected flows.

  • Check for uncaught exceptions.
  • Check for unhandled promise rejections.
  • Check React error-boundary output.
  • Check repeated render-loop warnings.
  • Check state updates after component unmount.
  • Check missing key warnings when they indicate unstable list behavior.
  • Check invalid DOM nesting.
  • Check controlled and uncontrolled input warnings.
  • Check hydration warnings when server rendering is involved.
  • Check failed dynamic imports.
  • Check blocked or missing assets.
  • Check authorization or token-refresh loops.
  • Check whether errors are swallowed and replaced with misleading success states.
  • Confirm expected failures are surfaced to the user.
  • Confirm sensitive values are not logged to the console.

Runtime blockers include:

  • Uncaught exception during a changed flow.
  • Unhandled promise rejection caused by the PR.
  • Component enters an infinite render or request loop.
  • A mutation fails but the UI reports success.
  • A failed lazy import makes the page unusable.
  • A route renders only after manually clearing local storage or cached state.

Minor console noise should not block unless it represents a reachable correctness, stability, security, or accessibility issue.


8. Network Requests and API Failures

Use browser developer tools or equivalent request inspection while exercising the affected flow.

  • Confirm the frontend calls the intended host and route.
  • Confirm the HTTP method is correct.
  • Confirm path and query parameters are encoded correctly.
  • Confirm request bodies match the backend contract.
  • Confirm headers and authentication are present when required.
  • Confirm dates, times, enum values, booleans, and nulls are serialized correctly.
  • Confirm successful responses are parsed correctly.
  • Confirm expected 204 responses do not cause JSON parsing errors.
  • Confirm 400, 401, 403, 404, 409, and 500 responses are handled appropriately.
  • Confirm error messages use the agreed response field, such as message.
  • Confirm failures are not silently swallowed.
  • Confirm failed requests do not clear the user’s draft.
  • Confirm failed requests do not update local cache as though they succeeded.
  • Confirm retries do not duplicate mutations.
  • Check for repeated requests caused by unstable query keys or effect dependencies.
  • Confirm cancellation behavior when navigating away or changing filters.
  • Confirm stale responses do not overwrite newer state.
  • Confirm loading indicators resolve on both success and failure.

A frontend that sends a request the backend cannot accept contains a merge-blocking defect when that request is part of the changed flow.

A frontend that interprets an expected error as success contains a merge-blocking defect.


9. API and Backend Contract Fidelity

Do not infer the backend contract solely from frontend types or mocks.

Review the producer PR, generated API documentation, or implemented backend route when available.

  • Confirm the route exactly matches the producer backend.
  • Confirm the HTTP method matches.
  • Confirm query parameter names and casing match.
  • Confirm path parameters match.
  • Confirm request body shape and field names match.
  • Confirm enum values match.
  • Confirm date and time formats match.
  • Confirm pagination request and response fields match.
  • Confirm filter and sorting semantics match.
  • Confirm sentinel behavior matches the backend contract.
  • Confirm values such as overdue are not incorrectly sent as domain enum values.
  • Confirm response DTO fields and nesting match.
  • Confirm nullable and optional fields are handled.
  • Confirm empty collections and missing values are handled.
  • Confirm expected status codes are handled.
  • Confirm error bodies use the agreed field, such as message.
  • Confirm the frontend does not depend on undocumented response fields.
  • Confirm the frontend targets the exact backend PR or branch that will ship with it.
  • Confirm sibling PR numbers are identified when the contract spans repositories.
  • Start both frontend and backend together when practical.
  • Exercise the actual user flow end to end.

Cross-repository blockers include:

  • Frontend calls a route the backend does not provide.
  • Frontend sends a query parameter with the wrong name.
  • Frontend expects a field the backend does not return.
  • Frontend assumes a successful JSON body when the backend returns 204.
  • Frontend sends a domain enum where the backend expects a separate filter flag.
  • Frontend and backend serialize dates differently.
  • Frontend only works against a backend change contained in an unready sibling PR.

A clean frontend delta depending on an unready backend normally receives COMMENT when there is no frontend code defect.

A concrete contract mismatch in the frontend normally receives REQUEST_CHANGES.


10. Data Loss and Mutation Safety

  • Confirm user-entered values are cleared only after confirmed success.
  • Confirm failed mutations preserve the user’s draft.
  • Confirm closing and reopening a dialog behaves intentionally.
  • Confirm optimistic updates roll back on failure.
  • Confirm cache invalidation targets the correct records and lists.
  • Confirm stale cache data does not overwrite a successful update.
  • Confirm rapid repeated submission does not duplicate records.
  • Confirm retry behavior is safe.
  • Confirm canceling a request does not present an error as a completed action.
  • Confirm partial form values are not dropped during validation.
  • Confirm hidden fields are not accidentally reset.
  • Confirm read-modify-write transformations preserve stored values.
  • Confirm mutation payloads do not send undefined, empty strings, or nulls in ways that erase existing data unintentionally.
  • Confirm file uploads preserve selected files after recoverable failures where appropriate.
  • Confirm navigation does not discard unsaved work without warning when the product requires protection.

Merge-blocking examples:

  • Failed save clears the form.
  • Optimistic update remains visible after the server rejects the request.
  • Editing one field silently clears another stored field.
  • Retry creates duplicate records.
  • A stale response overwrites a newer user action.

11. Data Transformation and Provenance

  • Confirm displayed values preserve their original meaning.
  • Check formatting and parsing for phone numbers, dates, currency, percentages, and identifiers.
  • Confirm values are not normalized in a lossy way before being written back.
  • Confirm empty strings, nulls, and missing values remain distinguishable when the backend contract requires it.
  • Confirm identifiers are not converted in ways that lose precision.
  • Confirm timezone conversion is intentional.
  • Confirm sorting uses the intended raw value rather than a formatted display string.
  • Confirm exports use the same data semantics shown in the UI.
  • Confirm audit or attribution labels use the recorded actor.
  • Never attribute an action to the current viewer when the stored actor is absent.
  • Display “Unknown”, “System”, or another agreed fallback when provenance is unavailable.
  • Confirm fallback labels do not create false business records.

A lossy read-and-write transformation that can corrupt stored values is a blocker.

Incorrectly attributing a historical action to the current viewer is a blocker when it creates a false audit representation.


12. State, Query, and Cache Behavior

  • Confirm query keys include all values that affect the response.
  • Confirm filter changes trigger the correct request.
  • Confirm applied filters, not draft filter controls, drive the displayed results.
  • Confirm clearing filters resets both the UI and request state.
  • Confirm pagination resets when criteria change where appropriate.
  • Confirm cached data from one record or filter does not appear under another.
  • Confirm query invalidation is specific enough to update affected views.
  • Confirm query invalidation is broad enough to prevent stale displays.
  • Confirm enabled flags do not prevent required requests.
  • Confirm dead query flags and unused props are removed when they are part of the changed slice.
  • Confirm effects do not duplicate requests.
  • Confirm unstable objects are not used directly in dependencies or query keys without normalization.
  • Confirm stale closures do not submit outdated values.
  • Confirm race conditions between filters, pagination, and navigation do not display incorrect data.
  • Confirm loading and previous-data behavior does not misrepresent which criteria are active.

A stale-data issue should block when it can cause the user to view, edit, approve, delete, or export the wrong record or result set.


13. Filters, Search, Sorting, Pagination, and Exports

  • Confirm displayed filter controls match the request sent to the backend.
  • Confirm applied criteria are visibly distinguishable from unsubmitted draft criteria.
  • Confirm search behavior matches backend semantics.
  • Confirm date presets produce the intended start and end values.
  • Confirm timezone handling does not shift date boundaries unexpectedly.
  • Confirm sorting fields and directions match backend support.
  • Confirm pagination indexes are translated correctly between zero-based and one-based systems.
  • Confirm changing filters resets pagination when required.
  • Confirm result counts match the active criteria.
  • Confirm empty results do not incorrectly display stale prior results.
  • Confirm exports use the same applied criteria shown in the UI.
  • Confirm exports do not use stale, draft, or default filter values.
  • Confirm export filenames and formats are correct where changed.
  • Confirm special filters such as overdue or unassigned use the backend’s actual contract.

A UI that shows one filter state while exporting or requesting another contains a merge-blocking defect when it can produce materially incorrect results.


14. User Feedback and Error Presentation

  • Confirm successful actions provide appropriate feedback.
  • Confirm failed actions provide visible feedback.
  • Confirm backend error messages are surfaced where appropriate.
  • Confirm generic fallback messaging exists when no safe server message is available.
  • Confirm the UI does not display success before the server confirms success.
  • Confirm error banners, alerts, and snackbars remain visible long enough to be understood.
  • Confirm repeated failures do not create an unusable stack of notifications.
  • Confirm loading indicators represent the actual operation.
  • Confirm loading states resolve after failure.
  • Confirm retry controls retry the intended action.
  • Confirm error state does not permanently block navigation or correction.
  • Confirm field-level validation identifies the correct field.
  • Confirm server validation errors are not replaced with misleading client text.

A failed operation that appears successful to the user is a blocker.

A failed operation with no visible feedback is normally a blocker when the user cannot reasonably determine that the action did not complete.


15. Accessibility and Interaction Reliability

Check accessibility in the context of the changed behavior.

Do not turn every minor accessibility improvement into a blocker.

  • Confirm interactive elements use appropriate semantic controls.
  • Confirm controls are keyboard reachable.
  • Confirm visible buttons can be activated by keyboard.
  • Confirm dialogs manage focus appropriately.
  • Confirm focus returns to a sensible location after closing dialogs.
  • Confirm labels are associated with form controls.
  • Confirm validation messages are programmatically associated where applicable.
  • Confirm icon-only controls have accessible names.
  • Confirm loading and status feedback is available to assistive technology.
  • Confirm role="alert" and role="status" regions remain mounted reliably.
  • Confirm live regions are not created only after the message appears in a way that prevents announcement.
  • Confirm hidden content is not still keyboard focusable.
  • Confirm disabled controls communicate their state.
  • Confirm color is not the only indicator of state.
  • Confirm table and list interactions remain understandable without a mouse.

Accessibility issues should block when they make a required action unusable, hide critical feedback, or violate explicit acceptance criteria.


16. Responsive and Layout Behavior

Use this section when the PR changes layout, tables, dialogs, forms, navigation, or responsive behavior.

  • Check the affected page at representative desktop and narrow viewport sizes.
  • Confirm content does not become unreachable due to clipping.
  • Confirm dialogs fit within the viewport.
  • Confirm horizontal scrolling is intentional where used.
  • Confirm fixed headers, drawers, and action bars do not cover content.
  • Confirm tables preserve access to required actions.
  • Confirm long text, filenames, identifiers, and error messages do not break the layout.
  • Confirm zoom does not make required controls unreachable.
  • Confirm responsive changes do not hide required functionality.
  • Confirm loading and empty states remain readable.
  • Confirm mobile behavior matches the ticket when mobile support is in scope.

A cosmetic spacing issue is not normally a blocker.

A layout issue that makes a required action inaccessible or hides critical information may be a blocker.


17. Playwright and End-to-End Tests

Run Playwright or the repository’s end-to-end suite when:

  • The PR claims end-to-end coverage.
  • The PR changes an existing covered flow.
  • The PR affects routing, authentication, forms, dialogs, API integration, or multi-step workflows.
  • The ticket specifically requires end-to-end behavior.

Checks:

  • Run the relevant Playwright or end-to-end suite.
  • Confirm tests run against the intended frontend and backend configuration.
  • Confirm test setup does not depend on stale state.
  • Confirm changed selectors are stable and user-oriented.
  • Confirm tests do not pass only because assertions occur before the action completes.
  • Confirm network failures are not silently ignored.
  • Confirm screenshots, traces, or videos are inspected when a test fails.
  • Confirm newly added tests are not skipped.
  • Confirm retries are not masking a deterministic defect.
  • Confirm the primary success flow works end to end.
  • Confirm critical failure behavior is covered where practical.

Run the repository’s actual command, such as:

npx playwright test

Or:

npm run test:e2e

A required end-to-end suite that no longer starts or compiles because of the PR is a blocker.

A failed end-to-end test should be investigated before deciding whether it is a delta defect, environment problem, inherited issue, or flaky test.


18. Timezone and Locale Behavior

  • Avoid timezone-dependent test assertions unless the product is explicitly fixed to one timezone.
  • Confirm date-only values do not shift when converted through Date.
  • Confirm local and UTC timestamps are displayed intentionally.
  • Confirm date filters include the intended boundaries.
  • Confirm daylight-saving transitions do not create invalid assumptions where relevant.
  • Confirm browser locale does not break parsing.
  • Confirm formatted values are not parsed back into canonical values.
  • Confirm tests use fixed dates or explicit timezones when necessary.
  • Confirm date presets produce stable results across supported environments.
  • Confirm exported dates match the intended displayed or canonical timezone.

A timezone bug should block when it causes records to be omitted, assigned to the wrong date, or submitted with materially incorrect timestamps.


19. Authorization and Ticket Acceptance Criteria

  • Compare behavior with the linked SH ticket.
  • Confirm role and ownership rules match the acceptance criteria.
  • Check distinctions such as author-only access, administrative override, and system-admin access.
  • Confirm unauthorized controls are hidden or disabled as intended.
  • Confirm hiding a control is not treated as a substitute for backend authorization.
  • Confirm direct navigation to restricted frontend routes is handled appropriately.
  • Confirm read-only users retain required read access.
  • Confirm create, edit, delete, upload, download, approve, and administrative actions follow the ticket.
  • Exercise permitted and denied scenarios when practical.
  • Confirm the frontend does not silently broaden access beyond the acceptance criteria.
  • Confirm authorization failures from the backend are handled without misleading success feedback.

A mismatch with explicit acceptance criteria is a blocker.


20. Scope Isolation and Regression Risk

  • Confirm the PR remains within its intended slice.
  • Review changes to unrelated routes, components, hooks, stores, API clients, and shared utilities.
  • Check global providers, routing, theme, error handling, query configuration, and authentication for broader effects.
  • Confirm dependency updates do not introduce unrelated runtime changes.
  • Check environment and build configuration changes across environments.
  • Confirm a local fix does not alter global request, serialization, caching, or navigation behavior unintentionally.
  • Run targeted regression scenarios for shared code changed by the PR.
  • Confirm read paths remain available when action controls are restricted.
  • Do not block solely because a nearby cleanup could have been included.
  • Do not require unrelated refactoring to merge an otherwise correct slice.

Unrelated cleanup is not automatically a blocker.

Unrelated behavior change with a reachable regression may be a blocker.


21. Stacked PR and Base Integrity

  • Confirm the head still descends from its declared base or parent PR.
  • Check whether the branch was rewritten or force-pushed.
  • Confirm GitHub does not report CONFLICTING or DIRTY.
  • Use git merge-tree against the actual base when needed.
  • Confirm the PR diff does not unintentionally include sibling or parent work.
  • Confirm the child PR is built and tested against the correct parent head.
  • Confirm backend-dependent flows are tested against the intended backend PR.
  • If the base advanced materially, require a rebase and fresh exact-head review when appropriate.
  • Respect the intended stack merge order.
  • Do not approve a child slice that depends on an unready parent.
  • Verify browser checks are performed against the actual stacked state, not an unrelated local branch.

A clean delta riding on an unready parent normally receives COMMENT, not REQUEST_CHANGES.


22. Governance

Approval may be withheld when:

  • No explicit SH ticket is linked when one is required.
  • Required CI is missing.
  • Required CI is failing.
  • Dependency review is required but missing or failing.
  • The branch is conflicting.
  • The base changed after the review.
  • The PR depends on a parent slice that is not ready.
  • The backend producer PR is not ready.
  • The stack merge order is incorrect.

Governance findings generally belong in the Overall Review Comment, not as inline code comments.


Verdict Rules

REQUEST_CHANGES

Use when the PR contains at least one concrete defect introduced, modified, or exposed by this delta that must be fixed before merge.

Examples:

  • TypeScript does not compile.
  • The production build fails.
  • Required tests do not compile or run.
  • The application cannot start.
  • The affected route crashes.
  • A changed user flow throws an unhandled exception.
  • A failed mutation clears the user’s work.
  • A button or visible action is nonfunctional.
  • A request does not match the backend contract.
  • Expected API failures are silently swallowed.
  • The UI reports success when the operation failed.
  • A data transformation can corrupt stored values.
  • Filters or exports use criteria different from what the UI shows.
  • The implementation violates the owning ticket.
  • A required accessibility path is unusable.
  • A security-sensitive control is exposed incorrectly.

Every requested change must be supported by a specific file:line inline comment with:

  • The concrete defect
  • The reachable impact
  • A specific fix
  • A focused regression test, component test, or browser validation request

COMMENT

Use when the reviewed delta is technically clean, but approval must be withheld because of:

  • Base or parent-PR readiness
  • Backend producer readiness
  • Stack order
  • Missing or failing required CI
  • Missing ticket linkage
  • Conflicting branch state
  • A changed base that requires re-review
  • Another governance condition

Also use COMMENT when there are useful observations but nothing that reasonably requires blocking the PR.

Do not use COMMENT to avoid requesting changes for a concrete merge-blocking defect.


APPROVE

Use only when:

  • The exact 7-character head SHA is identified.
  • Dependency installation succeeds from a clean checkout.
  • Required lint and static-analysis gates pass.
  • TypeScript compilation succeeds.
  • The production build succeeds.
  • Relevant unit and component tests pass.
  • Required Playwright or end-to-end tests pass when applicable.
  • The application starts successfully.
  • Affected routes render.
  • Affected user flows were exercised where practical.
  • No relevant uncaught browser exception or unhandled promise rejection was found.
  • Network requests match the backend contract.
  • Expected API failures are handled correctly.
  • User input and stored data are preserved correctly.
  • The paired frontend and backend contracts match.
  • Required ticket and CI conditions are satisfied.
  • The branch and stack are ready.
  • No unresolved blocker remains.

Do not approve solely because the code looks correct in the diff.

Do not approve solely because npm run build succeeds.

Do not approve solely because unit tests pass.

Compilation, production build behavior, browser runtime behavior, network behavior, user-state handling, and affected integrations should all be considered where relevant.


Writing Style

  • Sound like a human reviewer who understands the change.
  • Write from the user’s point of view.
  • Be direct without being harsh.
  • Use specific language tied to the actual implementation.
  • Vary sentence openings and paragraph construction.
  • Avoid repetitive formula language.
  • Avoid converting the checklist into a narrated audit.
  • Do not summarize every file changed.
  • Do not mention being an AI, agent, bot, or automated reviewer.
  • Do not use em dashes in outward-facing review copy.
  • Use 7-character abbreviated SHAs only.
  • Reference sibling frontend and backend PRs by number where relevant.
  • Prefer one precise comment over several overlapping comments.
  • Separate code defects from inherited-base and governance issues.
  • Do not claim a command, test, page, browser flow, request, or runtime scenario was checked unless it actually was.
  • When browser or backend validation could not be completed, state that limitation plainly rather than implying the behavior was verified.
  • Prefer evidence from lint output, TypeScript compilation, production builds, test results, browser behavior, console output, network requests, and actual end-to-end execution over assumptions from static code inspection.