docs: engineering governance — CLAUDE.md, ARCHITECTURE.md, CODE_REVIEW_CHECKLIST.md
Complete rewrite of CLAUDE.md with full binding rules (vision, architecture layers, component limits, state management, design system, AI integration, performance, security). New ARCHITECTURE.md covers folder structure, all data flows with ASCII diagrams, scoring pipeline, AI flow, unified result feed, and workspace routing. New CODE_REVIEW_CHECKLIST.md provides a 10-section review guide and pre-commit quick checklist. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Benjamin Sutter
@@ -0,0 +1,226 @@
|
||||
# Code Review Checklist — Property Match
|
||||
|
||||
> Use this checklist before every PR review and before considering any implementation "done".
|
||||
> For architecture context see [ARCHITECTURE.md](./ARCHITECTURE.md).
|
||||
> For coding rules see [CLAUDE.md](./CLAUDE.md).
|
||||
|
||||
---
|
||||
|
||||
## A. Component Size & Structure
|
||||
|
||||
- [ ] **Is the component too large?**
|
||||
- Page component: hard limit 300 lines, target 150–200
|
||||
- Feature component: hard limit 250 lines, target 100–150
|
||||
- Atom/shared component: hard limit 150 lines
|
||||
- If over limit: extract sub-components, move constants to `*Constants.ts`, move helpers to `*Utils.ts` (`.tsx` if JSX)
|
||||
|
||||
- [ ] **Is business logic inside JSX?**
|
||||
- `.filter()`, `.sort()`, `.reduce()`, conditional rendering based on computed values — these belong in `useMemo`, not inline in the return
|
||||
- The render body should read computed values, not compute them
|
||||
|
||||
- [ ] **Are hardcoded strings present?**
|
||||
- Status labels (`'ACTIVE'`, `'Aktiv'`, `'PROPERTY_MANAGER'`) must come from `enums.ts` or `constants.ts`
|
||||
- Route paths must come from `ROUTES` constants, not string literals
|
||||
- German UI labels for enums live in `constants.ts` (e.g., `ASSET_TYPE_LABELS`)
|
||||
|
||||
- [ ] **Are hardcoded colors or thresholds present?**
|
||||
- Colors like `'#1a7a4a'`, `'#dc2626'` directly in `sx` props → use `DS_COLORS` from `lib/ds.ts`
|
||||
- Score thresholds like `score >= 80` → use `SCORE_STRONG` from `constants.ts`
|
||||
- Confidence thresholds like `conf >= 0.75` → use `CONF_HIGH` from `constants.ts`
|
||||
|
||||
- [ ] **Are duplicate result type labels/colors defined locally?**
|
||||
- `RESULT_TYPE_META` in `lib/ds.ts` is the single source of truth
|
||||
- `RESULT_TYPE_LABEL` / `RESULT_TYPE_COLOR` inline objects anywhere else → delete them
|
||||
|
||||
---
|
||||
|
||||
## B. State & Data
|
||||
|
||||
- [ ] **Is server data stored in Zustand?**
|
||||
- Any `useEffect(() => setFoo(data), [data])` pattern that mirrors React Query data into local/Zustand state → remove it, read directly from the query
|
||||
- Server data lives exclusively in the React Query cache
|
||||
|
||||
- [ ] **Is derived state stored in state?**
|
||||
- `const [count, setCount] = useState(0)` + `useEffect` to keep it in sync → compute inline or use `useMemo`
|
||||
- Exception: expensive computations may use `useMemo`
|
||||
|
||||
- [ ] **Is the full Zustand store subscribed to without a selector?**
|
||||
- `const { a, b, c } = useMyStore()` — re-renders on every store mutation
|
||||
- Use `useMyStore(s => s.a)` for single fields, `useShallow` for multiple fields
|
||||
|
||||
- [ ] **Is there a side effect in the render body?**
|
||||
- `queryClient.invalidateQueries(...)` called during render → must be in `useEffect`
|
||||
- Any `console.log`, analytics call, or mutation triggered directly in render body → `useEffect`
|
||||
|
||||
- [ ] **Are all hooks called before conditional early returns?**
|
||||
- React rules of hooks: all `useState`, `useMemo`, `useCallback`, `useQuery` calls must appear before any `if (isLoading) return ...`
|
||||
|
||||
---
|
||||
|
||||
## C. Type Safety
|
||||
|
||||
- [ ] **Are there `any` casts?**
|
||||
- `as any`, `: any`, `// @ts-ignore` — fix the type or use `unknown` with a type guard
|
||||
- One exception: bridging untyped third-party code — must be isolated in a named adapter
|
||||
|
||||
- [ ] **Are domain types re-defined locally?**
|
||||
- Any interface or type that duplicates something in `src/domain/` → import from domain instead
|
||||
|
||||
- [ ] **Are response types used?**
|
||||
- Service functions should return `ListResponse<T>` or `ItemResponse<T>` from `services/types.ts`
|
||||
|
||||
- [ ] **Do `.tsx` files contain JSX and `.ts` files not?**
|
||||
- Any file with JSX (`<Component />`, `<Box>`, `<Typography>`) must have `.tsx` extension
|
||||
- Plain TypeScript utilities use `.ts`
|
||||
|
||||
---
|
||||
|
||||
## D. Layering & Architecture
|
||||
|
||||
- [ ] **Is business logic in a component?**
|
||||
- Scoring calculations, ranking, filtering rules, AI calls, data transformation → move to service or `features/` layer
|
||||
- Components only render and dispatch
|
||||
|
||||
- [ ] **Is a provider called directly from a component or hook?**
|
||||
- `MockupPropertyProvider.getAll()` called in a hook → must go through `propertyService`
|
||||
- All provider access is mediated by a service
|
||||
|
||||
- [ ] **Is an LLM API called directly?**
|
||||
- Any `fetch` to an AI API, or direct `OpenAI` / `openrouter` SDK calls outside `src/services/ai/` → move behind `IAIService`
|
||||
|
||||
- [ ] **Are there cross-workspace component imports?**
|
||||
- A supply component importing from `components/demand/` or vice versa → use `components/shared/` or `components/ui/` instead
|
||||
|
||||
- [ ] **Are the import direction rules respected?**
|
||||
- `services/` must not import React hooks
|
||||
- `domain/` must not import anything from `src/`
|
||||
- `lib/constants.ts` must not import from components or services
|
||||
- No circular imports
|
||||
|
||||
---
|
||||
|
||||
## E. Loading, Error & Empty States
|
||||
|
||||
- [ ] **Does every data-dependent view handle loading?**
|
||||
- Every `useQuery` that drives a list or table must show a skeleton or spinner while `isLoading`
|
||||
- Use the appropriate skeleton component (`FeedSkeleton`, `ReminderSkeleton`, etc.)
|
||||
|
||||
- [ ] **Does every data-dependent view handle error?**
|
||||
- `isError` from `useQuery` must show `<ErrorState message={error.message} />`
|
||||
- Never leave an error state blank or silently swallowed
|
||||
|
||||
- [ ] **Does every list handle the empty state?**
|
||||
- Empty results after filtering → `<FeedEmptyState filtered={true} />` with a reset action
|
||||
- Empty because no data exists → `<FeedEmptyState filtered={false} />`
|
||||
|
||||
---
|
||||
|
||||
## F. Performance
|
||||
|
||||
- [ ] **Are list items wrapped in `React.memo`?**
|
||||
- Components rendered inside `.map()` in feeds, tables, or grids → should be `memo()`
|
||||
- Without memo, every parent state change (e.g., filter toggle, `selectedId`) re-renders the entire list
|
||||
|
||||
- [ ] **Are callbacks to memoized children wrapped in `useCallback`?**
|
||||
- Passing `() => setSelectedId(id)` inline as a prop to a `memo()`-wrapped child → the memo is broken
|
||||
- Wrap with `useCallback` and explicit dependency array
|
||||
|
||||
- [ ] **Are multiple aggregations over the same list collapsed into one pass?**
|
||||
- Three separate `useMemo` calls that each `.filter()` the same array → collapse into one `useMemo` with a `for` loop
|
||||
|
||||
- [ ] **Is `useMemo` used for expensive filters and sorts?**
|
||||
- `.filter()` + `.sort()` over 25+ items in the render body → `useMemo`
|
||||
- Decision aggregates (matchReady count, criticalGaps count, etc.) in `Properties.tsx` → already in `useMemo`
|
||||
|
||||
- [ ] **Are routes lazy-loaded?**
|
||||
- All page imports in `App.tsx` must use `React.lazy()` — never static imports for route components
|
||||
|
||||
---
|
||||
|
||||
## G. Explainability & Trust
|
||||
|
||||
- [ ] **Does every score shown have a breakdown path?**
|
||||
- A match score badge with no way to see *why* it's that score → add `ScoreBreakdownPanel` or link to MatchDetail
|
||||
|
||||
- [ ] **Are confidence levels shown proactively?**
|
||||
- Data with `confidenceScore < CONF_MEDIUM` or `freshness === 'STALE'` → must be visually flagged
|
||||
- `ConfidenceBadge`, `FreshnessIndicator`, `DataQualityBar` exist for this purpose
|
||||
|
||||
- [ ] **Are Future Availability signals clearly labeled as probabilistic?**
|
||||
- `FUTURE_AVAILABILITY` results must always show `probabilityScore` and `signalBasis`
|
||||
- They must never look identical to confirmed availability listings
|
||||
|
||||
- [ ] **Does AI output show its confidence?**
|
||||
- AI-extracted need criteria → shown with confidence indicator
|
||||
- AI match suggestions → shown with `MAISON_WORK` badge and probability
|
||||
|
||||
---
|
||||
|
||||
## H. Security
|
||||
|
||||
- [ ] **Is the React Query cache cleared on logout?**
|
||||
- Logout flow must call `queryClient.clear()` — otherwise stale data from previous user persists
|
||||
|
||||
- [ ] **Are role checks using `permissions.ts` helpers?**
|
||||
- `user.role === 'PROPERTY_MANAGER'` inline → use `canViewMatchCenter(user)` from `lib/permissions.ts`
|
||||
|
||||
- [ ] **Are workspace route guards in place?**
|
||||
- Every supply/demand/ops route wrapped in `<ProtectedRoute workspace={...} />`
|
||||
- Never guard with `if (user.workspace !== ...)` inside a component body
|
||||
|
||||
- [ ] **Is sensitive data stored in Zustand?**
|
||||
- PII, tokens, and sensitive fields beyond current-user identity should not persist in client state
|
||||
|
||||
---
|
||||
|
||||
## I. Backend-Readiness
|
||||
|
||||
- [ ] **Is the provider interface complete?**
|
||||
- Any new entity that creates a mock shortcut (array mutation inline, no interface) → add `I[Entity]Provider` and `Mockup[Entity]Provider`
|
||||
|
||||
- [ ] **Are async patterns consistent?**
|
||||
- All provider methods are `async` — if a new provider method is synchronous, fix it before it gets called from a service
|
||||
|
||||
- [ ] **Are query keys stable and specific?**
|
||||
- Query key `['properties']` is fine for list; `['properties', id]` for single item
|
||||
- Keys that include derived or computed objects (not primitives) will break cache lookup
|
||||
|
||||
- [ ] **Are stale times centralized?**
|
||||
- No `staleTime: 60_000` or `staleTime: 5 * 60 * 1000` inline in hook files
|
||||
- Use `STALE_PROPERTIES`, `STALE_MATCHES`, etc. from `src/lib/constants.ts`
|
||||
|
||||
---
|
||||
|
||||
## J. AI & Model Governance
|
||||
|
||||
- [ ] **Is the AI call going through `IAIService`?**
|
||||
- Direct fetch to AI API anywhere outside `src/services/ai/` → move it
|
||||
|
||||
- [ ] **Is the AI response validated with Zod before use?**
|
||||
- Raw LLM string parsed inline with `JSON.parse()` without a schema → add Zod validation
|
||||
|
||||
- [ ] **Is the AI output logged via `aiMonitoringService`?**
|
||||
- Any AI call that influences a business decision (need parsing, match recommendation, market signal) → log it
|
||||
|
||||
- [ ] **Does the AI flow have a fallback?**
|
||||
- If `aiService` throws or returns `confidence < threshold`, is there a manual entry path?
|
||||
- AI failures must never block the user workflow
|
||||
|
||||
---
|
||||
|
||||
## Quick Checklist — Pre-Commit
|
||||
|
||||
Before every commit, verify:
|
||||
|
||||
```
|
||||
[ ] npx tsc --noEmit → zero errors
|
||||
[ ] No new hardcoded colors, routes, or role strings
|
||||
[ ] No business logic added to a JSX return block
|
||||
[ ] No React Query data mirrored into Zustand or useState
|
||||
[ ] All hooks called before early returns
|
||||
[ ] New list-item components wrapped in React.memo
|
||||
[ ] New provider methods are async and match the interface
|
||||
[ ] New feature goes through Provider → Service → Hook → Component
|
||||
[ ] Loading, error, and empty states handled for any new data-fetching view
|
||||
[ ] File with JSX has .tsx extension
|
||||
```
|
||||
Reference in New Issue
Block a user