Compare commits

...

1 Commits

Author SHA1 Message Date
Alan Buscaglia 22f17aa394 docs(skills): strengthen UI review rules
- Clarify feature-local helper placement
- Add UI error-state replacement checklist
- Require negative tests for mapper predicates
2026-05-27 16:41:18 +02:00
2 changed files with 56 additions and 1 deletions
+24 -1
View File
@@ -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 <SignInWithDifferentAccount />;
// ✅ GOOD: remove unreachable state and render only reachable actions
{state.canRetry && <Button onClick={retry}>Retry</Button>}
<Button asChild><Link href="/sign-in">Go to Sign In</Link></Button>
```
## 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.
+32
View File
@@ -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.