From 09d65d79f76a30806fa12e9bae4c56ef1f519c87 Mon Sep 17 00:00:00 2001 From: nrslib <38722970+nrslib@users.noreply.github.com> Date: Mon, 2 Feb 2026 06:59:28 +0900 Subject: [PATCH] =?UTF-8?q?=E3=83=95=E3=83=AD=E3=83=B3=E3=83=88=E3=82=A8?= =?UTF-8?q?=E3=83=B3=E3=83=89=E3=82=A8=E3=83=BC=E3=82=B8=E3=82=A7=E3=83=B3?= =?UTF-8?q?=E3=83=88=E3=81=AB=E3=81=8A=E3=81=84=E3=81=A6SmartUI=E3=82=92?= =?UTF-8?q?=E6=8E=92=E9=99=A4=E3=81=99=E3=82=8B=E3=82=88=E3=81=86=E3=81=AB?= =?UTF-8?q?=E6=8C=87=E7=A4=BA=E5=87=BA=E3=81=97?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../en/agents/expert/frontend-reviewer.md | 428 ++++++++++++++++-- .../ja/agents/expert/frontend-reviewer.md | 95 +++- 2 files changed, 489 insertions(+), 34 deletions(-) diff --git a/resources/global/en/agents/expert/frontend-reviewer.md b/resources/global/en/agents/expert/frontend-reviewer.md index b3d4365..c9396b3 100644 --- a/resources/global/en/agents/expert/frontend-reviewer.md +++ b/resources/global/en/agents/expert/frontend-reviewer.md @@ -36,6 +36,15 @@ The user interface is the only point of contact between the system and users. No ### 1. Component Design +**Principle: Do not write everything in one file. Always split components.** + +**Required splits:** +- Has its own state → Must split +- JSX over 50 lines → Split +- Reusable → Split +- Multiple responsibilities → Split +- Independent section within page → Split + **Required Checks:** | Criteria | Judgment | @@ -60,8 +69,44 @@ The user interface is the only point of contact between the system and users. No | Layout | Arrangement, structure | `PageLayout`, `Grid` | | Utility | Common functionality | `ErrorBoundary`, `Portal` | +**Directory Structure:** +``` +features/{feature-name}/ +├── components/ +│ ├── {feature}-view.tsx # Main view (composes children) +│ ├── {sub-component}.tsx # Sub-components +│ └── index.ts +├── hooks/ +├── types.ts +└── index.ts +``` + ### 2. State Management +**Principle: Child components do not modify their own state. They bubble events to parent, and parent manipulates state.** + +```tsx +// ❌ Child modifies its own state +const ChildBad = ({ initialValue }: { initialValue: string }) => { + const [value, setValue] = useState(initialValue) + return setValue(e.target.value)} /> +} + +// ✅ Parent manages state, child notifies via callback +const ChildGood = ({ value, onChange }: { value: string; onChange: (v: string) => void }) => { + return onChange(e.target.value)} /> +} + +const Parent = () => { + const [value, setValue] = useState('') + return +} +``` + +**Exception (OK for child to have local state):** +- UI-only temporary state (hover, focus, animation) +- Completely local state that doesn't need to be communicated to parent + **Required Checks:** | Criteria | Judgment | @@ -81,7 +126,345 @@ The user interface is the only point of contact between the system and users. No | Shared across multiple components | Context or state management library | | Server data cache | Data fetching library (TanStack Query, etc.) | -### 3. Performance +### 3. Data Fetching + +**Principle: API calls are made in root (View) components and passed to children via props.** + +```tsx +// ✅ CORRECT - Fetch at root, pass to children +const OrderDetailView = () => { + const { data: order, isLoading, error } = useGetOrder(orderId) + const { data: items } = useListOrderItems(orderId) + + if (isLoading) return + if (error) return + + return ( + + ) +} + +// ❌ WRONG - Child fetches its own data +const OrderSummary = ({ orderId }) => { + const { data: order } = useGetOrder(orderId) + // ... +} +``` + +**Reasons:** +- Data flow is explicit and traceable +- Child components are pure presentation (easier to test) +- No hidden dependencies in child components + +**When UI state changes affect parameters (week switching, filters, etc.):** + +Manage state at View level and pass callbacks to components. + +```tsx +// ✅ CORRECT - State managed at View level +const ScheduleView = () => { + const [currentWeek, setCurrentWeek] = useState(startOfWeek(new Date())) + const { data } = useListSchedules({ + from: format(currentWeek, 'yyyy-MM-dd'), + to: format(endOfWeek(currentWeek), 'yyyy-MM-dd'), + }) + + return ( + + ) +} + +// ❌ WRONG - Component manages state + data fetching +const WeeklyCalendar = ({ facilityId }) => { + const [currentWeek, setCurrentWeek] = useState(...) + const { data } = useListSchedules({ facilityId, from, to }) + // ... +} +``` + +**Exceptions (component-level fetching allowed):** + +| Case | Reason | +|------|--------| +| Infinite scroll | Depends on scroll position (internal UI state) | +| Search autocomplete | Real-time search based on input value | +| Independent widget | Notification badge, weather, etc. Completely unrelated to parent data | +| Real-time updates | WebSocket/Polling auto-updates | +| Modal detail fetch | Fetch additional data only when opened | + +**Decision criteria: "Is there no point in parent managing this / Does it not affect parent?"** + +**Required Checks:** + +| Criteria | Judgment | +|----------|----------| +| Direct fetch in component | Separate to Container layer | +| No error handling | REJECT | +| Loading state not handled | REJECT | +| No cancellation handling | Warning | +| N+1 query-like fetching | REJECT | + +### 4. Shared Components and Abstraction + +**Principle: Common UI patterns should be shared components. Copy-paste of inline styles is prohibited.** + +```tsx +// ❌ WRONG - Copy-pasted inline styles + + +// ✅ CORRECT - Use shared component + + + +``` + +**Patterns to make shared components:** +- Icon buttons (close, edit, delete, etc.) +- Loading/error displays +- Status badges +- Tab switching +- Label + value display (detail screens) +- Search input +- Color legends + +**Avoid over-generalization:** + +```tsx +// ❌ WRONG - Forcing stepper variant into IconButton +export const iconButtonVariants = cva('...', { + variants: { + variant: { + default: '...', + outlined: '...', // ← Stepper-specific, not used elsewhere + }, + size: { + medium: 'p-2', + stepper: 'w-8 h-8', // ← Only used with outlined + }, + }, +}) + +// ✅ CORRECT - Purpose-specific component +export function StepperButton(props) { + return ( + + ) +} +``` + +**Signs to make separate components:** +- Implicit constraints like "this variant is always with this size" +- Added variant is clearly different from original component's purpose +- Props specification becomes complex on the usage side + +### 5. Abstraction Level Evaluation + +**Conditional branch bloat detection:** + +| Pattern | Judgment | +|---------|----------| +| Same conditional in 3+ places | Extract to shared component → **REJECT** | +| Props-based branching with 5+ types | Consider component split | +| Nested ternaries in render | Early return or component separation → **REJECT** | +| Type-based render branching | Consider polymorphic components | + +**Abstraction level mismatch detection:** + +| Pattern | Problem | Fix | +|---------|---------|-----| +| Data fetching logic mixed in JSX | Hard to read | Extract to custom hook | +| Business logic mixed in component | Responsibility violation | Separate to hooks/utils | +| Style calculation logic scattered | Hard to maintain | Extract to utility function | +| Same transformation in multiple places | DRY violation | Extract to common function | + +**Good abstraction examples:** +```tsx +// ❌ Conditional bloat +function UserBadge({ user }) { + if (user.role === 'admin') { + return Admin + } else if (user.role === 'moderator') { + return Moderator + } else if (user.role === 'premium') { + return Premium + } else { + return User + } +} + +// ✅ Abstracted with Map +const ROLE_CONFIG = { + admin: { label: 'Admin', className: 'bg-red-500' }, + moderator: { label: 'Moderator', className: 'bg-yellow-500' }, + premium: { label: 'Premium', className: 'bg-purple-500' }, + default: { label: 'User', className: 'bg-gray-500' }, +} + +function UserBadge({ user }) { + const config = ROLE_CONFIG[user.role] ?? ROLE_CONFIG.default + return {config.label} +} +``` + +```tsx +// ❌ Mixed abstraction levels +function OrderList() { + const [orders, setOrders] = useState([]) + useEffect(() => { + fetch('/api/orders') + .then(res => res.json()) + .then(data => setOrders(data)) + }, []) + + return orders.map(order => ( +
{order.total.toLocaleString()} USD
+ )) +} + +// ✅ Aligned abstraction levels +function OrderList() { + const { data: orders } = useOrders() // Hide data fetching + + return orders.map(order => ( + + )) +} +``` + +### 6. Frontend and Backend Separation of Concerns + +#### 6.1 Display Format Responsibility + +**Principle: Backend returns "data", frontend converts to "display format".** + +```tsx +// ✅ Frontend: Convert to display format +export function formatPrice(amount: number): string { + return `$${amount.toLocaleString()}` +} + +export function formatDate(date: Date): string { + return format(date, 'MMM d, yyyy') +} +``` + +**Reasons:** +- Display format is a UI concern, not backend responsibility +- Easy to support internationalization +- Frontend can flexibly change display + +**Required Checks:** + +| Criteria | Judgment | +|----------|----------| +| Backend returns display strings | Suggest design review | +| Same format logic copy-pasted | Unify to utility function | +| Inline formatting in component | Extract to function | + +#### 6.2 Domain Logic Placement (Smart UI Elimination) + +**Principle: Domain logic (business rules) belongs in the backend. Frontend only displays and edits state.** + +**What is domain logic:** +- Aggregate business rules (stock validation, price calculation, status transitions) +- Business constraint validation +- Invariant enforcement + +**Frontend responsibilities:** +- Display state received from server +- Collect user input and send commands to backend +- Manage UI-only temporary state (focus, hover, modal open/close) +- Display format conversion (formatting, sorting, filtering) + +**Required Checks:** + +| Criteria | Judgment | +|----------|----------| +| Price calculation/stock validation in frontend | Move to backend → **REJECT** | +| Status transition rules in frontend | Move to backend → **REJECT** | +| Business validation in frontend | Move to backend → **REJECT** | +| Recalculating server-computable values in frontend | Redundant → **REJECT** | + +**Good vs Bad Examples:** + +```tsx +// ❌ BAD - Business rules in frontend +function OrderForm({ order }: { order: Order }) { + const totalPrice = order.items.reduce((sum, item) => + sum + item.price * item.quantity, 0 + ) + const canCheckout = totalPrice >= 100 && order.items.every(i => i.stock > 0) + + return +} + +// ✅ GOOD - Display state received from server +function OrderForm({ order }: { order: Order }) { + // totalPrice, canCheckout are received from server + return ( + <> +
{formatPrice(order.totalPrice)}
+ + + ) +} +``` + +```tsx +// ❌ BAD - Status transition logic in frontend +function TaskCard({ task }: { task: Task }) { + const canStart = task.status === 'pending' && task.assignee !== null + const canComplete = task.status === 'in_progress' && /* complex conditions... */ + + return ( + <> + + + + ) +} + +// ✅ GOOD - Server returns allowed actions +function TaskCard({ task }: { task: Task }) { + // task.allowedActions = ['start', 'cancel'], etc., calculated by server + const canStart = task.allowedActions.includes('start') + const canComplete = task.allowedActions.includes('complete') + + return ( + <> + + + + ) +} +``` + +**Exceptions (OK to have logic in frontend):** + +| Case | Reason | +|------|--------| +| UI-only validation | UX feedback like "required field", "max length" (must also validate on server) | +| Client-side filter/sort | Changing display order of lists received from server | +| Display condition branching | UI control like "show details if logged in" | +| Real-time feedback | Preview display during input | + +**Decision criteria: "Would the business break if this calculation differs from the server?"** +- YES → Place in backend (domain logic) +- NO → OK in frontend (display logic) + +### 7. Performance **Required Checks:** @@ -102,40 +485,15 @@ The user interface is the only point of contact between the system and users. No **Anti-patterns:** ```tsx -// Bad: New object every render +// ❌ New object every render -// Good: Constant or useMemo +// ✅ Constant or useMemo const style = useMemo(() => ({ color: 'red' }), []); ``` -### 4. Data Fetching - -**Required Checks:** - -| Criteria | Judgment | -|----------|----------| -| Direct fetch in component | Separate to Container layer | -| No error handling | REJECT | -| Loading state not handled | REJECT | -| No cancellation handling | Warning | -| N+1 query-like fetching | REJECT | - -**Recommended Pattern:** -```tsx -// Good: Data fetching at root -function UserPage() { - const { data, isLoading, error } = useQuery(['user', id], fetchUser); - - if (isLoading) return ; - if (error) return ; - - return ; -} -``` - -### 5. Accessibility +### 8. Accessibility **Required Checks:** @@ -154,7 +512,7 @@ function UserPage() { - [ ] Does it make sense with a screen reader? - [ ] Is color contrast sufficient? -### 6. TypeScript/Type Safety +### 9. TypeScript/Type Safety **Required Checks:** @@ -165,7 +523,7 @@ function UserPage() { | No Props type definition | REJECT | | Inappropriate event handler types | Needs fix | -### 7. Frontend Security +### 10. Frontend Security **Required Checks:** @@ -176,7 +534,7 @@ function UserPage() { | Sensitive data stored in frontend | REJECT | | CSRF token not used | Needs verification | -### 8. Testability +### 11. Testability **Required Checks:** @@ -186,7 +544,7 @@ function UserPage() { | Structure difficult to test | Consider separation | | Business logic embedded in UI | REJECT | -### 9. Anti-pattern Detection +### 12. Anti-pattern Detection **REJECT** if found: @@ -198,6 +556,8 @@ function UserPage() { | useEffect hell | Dependencies too complex | | Premature Optimization | Unnecessary memoization | | Magic Strings | Hardcoded strings | +| Hidden Dependencies | Child components with hidden API calls | +| Over-generalization | Components forced to be generic | ## Important @@ -206,3 +566,5 @@ function UserPage() { - **Accessibility is hard to retrofit**: Build in from the start - **Beware excessive abstraction**: Keep it simple - **Follow framework conventions**: Standard approaches over custom patterns +- **Data fetching at root**: Don't create hidden dependencies in children +- **Controlled components**: Data flow is unidirectional diff --git a/resources/global/ja/agents/expert/frontend-reviewer.md b/resources/global/ja/agents/expert/frontend-reviewer.md index 5e656da..020afe0 100644 --- a/resources/global/ja/agents/expert/frontend-reviewer.md +++ b/resources/global/ja/agents/expert/frontend-reviewer.md @@ -343,7 +343,9 @@ function OrderList() { } ``` -### 6. データと表示形式の責務分離 +### 6. フロントエンドとバックエンドの責務分離 + +#### 6.1 表示形式の責務 **原則: バックエンドは「データ」を返し、フロントエンドが「表示形式」に変換する。** @@ -371,6 +373,97 @@ export function formatDate(date: Date): string { | 同じフォーマット処理が複数箇所にコピペ | ユーティリティ関数に統一 | | コンポーネント内でインラインフォーマット | 関数に抽出 | +#### 6.2 ドメインロジックの配置(SmartUI排除) + +**原則: ドメインロジック(ビジネスルール)はバックエンドに配置。フロントエンドは状態の表示・編集のみ。** + +**ドメインロジックとは:** +- 集約のビジネスルール(在庫判定、価格計算、ステータス遷移) +- バリデーション(業務制約の検証) +- 不変条件の保証 + +**フロントエンドの責務:** +- サーバーから受け取った状態を表示 +- ユーザー入力を収集し、コマンドとしてバックエンドに送信 +- UI専用の一時状態管理(フォーカス、ホバー、モーダル開閉) +- 表示形式の変換(フォーマット、ソート、フィルタ) + +**必須チェック:** + +| 基準 | 判定 | +|------|------| +| フロントエンドで価格計算・在庫判定 | バックエンドに移動 → **REJECT** | +| フロントエンドでステータス遷移ルール | バックエンドに移動 → **REJECT** | +| フロントエンドでビジネスバリデーション | バックエンドに移動 → **REJECT** | +| サーバー側で計算可能な値をフロントで再計算 | 冗長 → **REJECT** | + +**良い例 vs 悪い例:** + +```tsx +// ❌ BAD - フロントエンドでビジネスルール +function OrderForm({ order }: { order: Order }) { + const totalPrice = order.items.reduce((sum, item) => + sum + item.price * item.quantity, 0 + ) + const canCheckout = totalPrice >= 1000 && order.items.every(i => i.stock > 0) + + return +} + +// ✅ GOOD - バックエンドから受け取った状態を表示 +function OrderForm({ order }: { order: Order }) { + // totalPrice, canCheckout はサーバーから受け取る + return ( + <> +
{formatPrice(order.totalPrice)}
+ + + ) +} +``` + +```tsx +// ❌ BAD - フロントエンドでステータス遷移判定 +function TaskCard({ task }: { task: Task }) { + const canStart = task.status === 'pending' && task.assignee !== null + const canComplete = task.status === 'in_progress' && /* 複雑な条件... */ + + return ( + <> + + + + ) +} + +// ✅ GOOD - サーバーが許可するアクションを返す +function TaskCard({ task }: { task: Task }) { + // task.allowedActions = ['start', 'cancel'] など、サーバーが計算 + const canStart = task.allowedActions.includes('start') + const canComplete = task.allowedActions.includes('complete') + + return ( + <> + + + + ) +} +``` + +**例外(フロントエンドにロジックを置いてもOK):** + +| ケース | 理由 | +|--------|------| +| UI専用バリデーション | 「必須入力」「文字数制限」等のUXフィードバック(サーバー側でも検証必須) | +| クライアント側フィルタ/ソート | サーバーから受け取ったリストの表示順序変更 | +| 表示条件の分岐 | 「ログイン済みなら詳細表示」等のUI制御 | +| リアルタイムフィードバック | 入力中のプレビュー表示 | + +**判断基準: 「この計算結果がサーバーとズレたら業務が壊れるか?」** +- YES → バックエンドに配置(ドメインロジック) +- NO → フロントエンドでもOK(表示ロジック) + ### 7. パフォーマンス **必須チェック:**