From 22f17aa39483a2f8b64e21396d89dac1a5be5253 Mon Sep 17 00:00:00 2001 From: Alan Buscaglia Date: Wed, 27 May 2026 16:41:18 +0200 Subject: [PATCH] docs(skills): strengthen UI review rules - Clarify feature-local helper placement - Add UI error-state replacement checklist - Require negative tests for mapper predicates --- skills/prowler-ui/SKILL.md | 25 ++++++++++++++++++++++++- skills/typescript/SKILL.md | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/skills/prowler-ui/SKILL.md b/skills/prowler-ui/SKILL.md index bde8c966db..db2c2911d3 100644 --- a/skills/prowler-ui/SKILL.md +++ b/skills/prowler-ui/SKILL.md @@ -79,8 +79,10 @@ Recharts/library? → CHART_COLORS constant + var() ### Scope Rule (ABSOLUTE) -- Used 2+ places → `lib/` or `types/` or `hooks/` (components go in `components/{domain}/`) +- Used by unrelated features → `lib/` or `types/` or `hooks/` (components go in `components/{domain}/`) +- Used by 2+ files in the same feature/domain → keep it inside that feature/domain, e.g. `_lib/`, `_hooks/`, or local `types.ts` - Used 1 place → keep local in feature directory +- **Feature/domain ownership beats raw usage count**: sharing between sibling files does not automatically make a helper global - **This determines ALL folder structure decisions** ## Project Structure @@ -242,6 +244,27 @@ import { severityLabel } from "@/types/findings"; If a helper doesn't exist and will be used in 2+ places, add it to `ui/lib/` or `ui/types/` and reuse it. Keep local only if used in exactly one place. +## Error State Replacement Checklist (REQUIRED) + +When replacing UI error-state logic, remove legacy state fields and branches that the new contract can no longer produce. + +Before finishing the change, check: + +- [ ] Are there flags, union members, buttons, or handlers that are now unreachable? +- [ ] Does the UI still render an action for an error case the new API contract cannot emit? +- [ ] Does retry appear only for errors where repeating the same action can help? +- [ ] Are error mappers matching specific status/code/detail or field pointers rather than broad payload-level pointers? +- [ ] Are negative tests present for over-broad mapper/type-guard matches? + +```typescript +// ❌ BAD: old branch kept after new mapper never returns needsSignOut +if (state.needsSignOut) return ; + +// ✅ GOOD: remove unreachable state and render only reachable actions +{state.canRetry && } + +``` + ## Derived State Rule (REQUIRED) Avoid `useState` + `useEffect` patterns that mirror props or searchParams — they create sync bugs and unnecessary re-renders. Derive values directly from the source of truth. diff --git a/skills/typescript/SKILL.md b/skills/typescript/SKILL.md index 4d664d3189..4d4c43bad9 100644 --- a/skills/typescript/SKILL.md +++ b/skills/typescript/SKILL.md @@ -102,6 +102,38 @@ function isUser(value: unknown): value is User { } ``` +### Type Guard Tests (REQUIRED) + +When adding or changing a type guard or mapper predicate, test both accepted and rejected shapes. A positive test alone often hides over-broad matching. + +```typescript +const ERROR_POINTER = { + INVITATION_TOKEN: "/data/attributes/invitation_token", +} as const; + +function isInvitationTokenError(error: ApiError): boolean { + return error.source?.pointer === ERROR_POINTER.INVITATION_TOKEN; +} + +it("should identify invitation token errors", () => { + expect( + isInvitationTokenError({ + detail: "Invalid invitation code.", + source: { pointer: ERROR_POINTER.INVITATION_TOKEN }, + }), + ).toBe(true); +}); + +it("should not identify payload-level errors as invitation token errors", () => { + expect( + isInvitationTokenError({ + detail: "Invalid request data.", + source: { pointer: "/data" }, + }), + ).toBe(false); +}); +``` + ## Coupled Optional Props (REQUIRED) Do not model semantically coupled props as independent optionals — this allows invalid half-states that compile but break at runtime. Use discriminated unions with `never` to make invalid combinations impossible.