mirror of
https://github.com/prowler-cloud/prowler.git
synced 2026-03-21 18:58:04 +00:00
docs(skills): add guardrails for review-driven UI patterns
- Add batch/instant API contract and derived-state guardrails to prowler-ui - Add coupled optional props rule to TypeScript skill - Add pre-re-review thread hygiene checklist to prowler-pr
This commit is contained in:
@@ -132,6 +132,18 @@ Follow conventional commits:
|
||||
4. ✅ Branch is up to date with main
|
||||
5. ✅ Commits are clean and descriptive
|
||||
|
||||
## Before Re-Requesting Review (REQUIRED)
|
||||
|
||||
Resolve or respond to **every** open inline review thread before re-requesting review:
|
||||
|
||||
1. **Agreed + fixed**: Commit the change. Reply with the commit hash so the reviewer can verify quickly:
|
||||
> Fixed in `abc1234`.
|
||||
2. **Agreed but deferred**: Explain why it's out of scope for this PR and where it's tracked.
|
||||
3. **Disagreed**: Reply with clear technical reasoning. Do not leave threads silently open.
|
||||
4. **Re-request review** only after all threads are in a clean state — either resolved or explicitly responded to.
|
||||
|
||||
> **Rule of thumb**: A reviewer should never have to wonder "did they see my comment?" when they re-open the PR.
|
||||
|
||||
## Resources
|
||||
|
||||
- **Documentation**: See [references/](references/) for links to local developer guide
|
||||
|
||||
@@ -186,6 +186,109 @@ cd ui && pnpm run build
|
||||
cd ui && pnpm start
|
||||
```
|
||||
|
||||
## Batch vs Instant Component API (REQUIRED)
|
||||
|
||||
When a component supports both **batch** (deferred, submit-based) and **instant** (immediate callback) behavior, model the coupling with a discriminated union — never as independent optionals. Coupled props must be all-or-nothing.
|
||||
|
||||
```typescript
|
||||
// ❌ NEVER: Independent optionals — allows invalid half-states
|
||||
interface FilterProps {
|
||||
onBatchApply?: (values: string[]) => void;
|
||||
onInstantChange?: (value: string) => void;
|
||||
isBatchMode?: boolean;
|
||||
}
|
||||
|
||||
// ✅ ALWAYS: Discriminated union — one valid shape per mode
|
||||
type BatchProps = {
|
||||
mode: "batch";
|
||||
onApply: (values: string[]) => void;
|
||||
onCancel: () => void;
|
||||
};
|
||||
|
||||
type InstantProps = {
|
||||
mode: "instant";
|
||||
onChange: (value: string) => void;
|
||||
// onApply/onCancel are forbidden here via structural exclusion
|
||||
onApply?: never;
|
||||
onCancel?: never;
|
||||
};
|
||||
|
||||
type FilterProps = BatchProps | InstantProps;
|
||||
```
|
||||
|
||||
This makes invalid prop combinations a compile error, not a runtime surprise.
|
||||
|
||||
## Reuse Shared Display Utilities First (REQUIRED)
|
||||
|
||||
Before adding **local** display maps (labels, provider names, status strings, category formatters), search `ui/types/*` and `ui/lib/*` for existing helpers.
|
||||
|
||||
```typescript
|
||||
// ✅ CHECK THESE FIRST before creating a new map:
|
||||
// ui/lib/utils.ts → general formatters
|
||||
// ui/types/providers.ts → provider display names, icons
|
||||
// ui/types/findings.ts → severity/status display maps
|
||||
// ui/types/compliance.ts → category/group formatters
|
||||
|
||||
// ❌ NEVER add a local map that already exists:
|
||||
const SEVERITY_LABELS: Record<string, string> = {
|
||||
critical: "Critical",
|
||||
high: "High",
|
||||
// ...duplicating an existing shared map
|
||||
};
|
||||
|
||||
// ✅ Import and reuse instead:
|
||||
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.
|
||||
|
||||
## 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.
|
||||
|
||||
```typescript
|
||||
// ❌ NEVER: Mirror props into state via effect
|
||||
const [localFilter, setLocalFilter] = useState(filter);
|
||||
useEffect(() => { setLocalFilter(filter); }, [filter]);
|
||||
|
||||
// ✅ ALWAYS: Derive directly
|
||||
const localFilter = filter; // or compute inline
|
||||
```
|
||||
|
||||
If local state is genuinely needed (e.g., optimistic UI, pending edits before submit), add a short comment:
|
||||
|
||||
```typescript
|
||||
// Local state needed: user edits are buffered until "Apply" is clicked
|
||||
const [pending, setPending] = useState(initialValues);
|
||||
```
|
||||
|
||||
## Strict Key Typing for Label Maps (REQUIRED)
|
||||
|
||||
Avoid `Record<string, string>` when the key set is known. Use an explicit union type or a const-key object so typos are caught at compile time.
|
||||
|
||||
```typescript
|
||||
// ❌ Loose — typos compile silently
|
||||
const STATUS_LABELS: Record<string, string> = {
|
||||
actve: "Active", // typo, no error
|
||||
};
|
||||
|
||||
// ✅ Tight — union key
|
||||
type Status = "active" | "inactive" | "pending";
|
||||
const STATUS_LABELS: Record<Status, string> = {
|
||||
active: "Active",
|
||||
inactive: "Inactive",
|
||||
pending: "Pending",
|
||||
// actve: "Active" ← compile error
|
||||
};
|
||||
|
||||
// ✅ Also fine — const satisfies
|
||||
const STATUS_LABELS = {
|
||||
active: "Active",
|
||||
inactive: "Inactive",
|
||||
pending: "Pending",
|
||||
} as const satisfies Record<Status, string>;
|
||||
```
|
||||
|
||||
## QA Checklist Before Commit
|
||||
|
||||
- [ ] `pnpm run typecheck` passes
|
||||
@@ -199,6 +302,15 @@ cd ui && pnpm start
|
||||
- [ ] Accessibility: keyboard navigation, ARIA labels
|
||||
- [ ] Mobile responsive (if applicable)
|
||||
|
||||
## Pre-Re-Review Checklist (Review Thread Hygiene)
|
||||
|
||||
Before requesting re-review from a reviewer:
|
||||
|
||||
- [ ] Every unresolved inline thread has been either fixed or explicitly answered with a rationale
|
||||
- [ ] If you agreed with a comment: the change is committed and the commit hash is mentioned in the reply
|
||||
- [ ] If you disagreed: the reply explains why with clear reasoning — do not leave threads silently open
|
||||
- [ ] Re-request review only after all threads are in a clean state
|
||||
|
||||
## Migrations Reference
|
||||
|
||||
| From | To | Key Changes |
|
||||
|
||||
@@ -102,6 +102,38 @@ function isUser(value: unknown): value is User {
|
||||
}
|
||||
```
|
||||
|
||||
## 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.
|
||||
|
||||
```typescript
|
||||
// ❌ BEFORE: Independent optionals — half-states allowed
|
||||
interface PaginationProps {
|
||||
onPageChange?: (page: number) => void;
|
||||
pageSize?: number;
|
||||
currentPage?: number;
|
||||
}
|
||||
|
||||
// ✅ AFTER: Discriminated union — shape is all-or-nothing
|
||||
type ControlledPagination = {
|
||||
controlled: true;
|
||||
currentPage: number;
|
||||
pageSize: number;
|
||||
onPageChange: (page: number) => void;
|
||||
};
|
||||
|
||||
type UncontrolledPagination = {
|
||||
controlled: false;
|
||||
currentPage?: never;
|
||||
pageSize?: never;
|
||||
onPageChange?: never;
|
||||
};
|
||||
|
||||
type PaginationProps = ControlledPagination | UncontrolledPagination;
|
||||
```
|
||||
|
||||
**Key rule:** If two or more props are only meaningful together, they belong to the same discriminated union branch. Mixing them as independent optionals shifts correctness responsibility from the type system to runtime guards.
|
||||
|
||||
## Import Types
|
||||
|
||||
```typescript
|
||||
|
||||
Reference in New Issue
Block a user