feat: integrate Vercel React Best Practices into review rules

[rxolve]

Summary

• React framework: 5개 카테고리(17개 규칙) → 9개 카테고리(~31개 규칙)으로 확장. waterfalls_and_async [CRITICAL], bundle_size [CRITICAL], rendering, js_performance 신규 카테고리 추가 및 기존 hooks, performance, state 확장
• Next.js framework: 6개 카테고리(14개 규칙) → 8개 카테고리(~26개 규칙)으로 확장. server_performance [CRITICAL], caching 신규 카테고리 추가 및 기존 components, data_fetching, optimization 확장
• False positive patterns: React 3개 + Next.js 3개 프레임워크별 FP 패턴 추가, 글로벌 builtin 패턴 4개 추가 (React 2, Next.js 2)
• Tests: 12개 새 테스트 케이스 추가 (164 total, all passing)

Test plan

• npm run build — TypeScript 컴파일 성공
• npm test — 전체 164 테스트 통과
• npm test -- --testPathPattern=frameworks — 프레임워크 테스트 통과
• npm test -- --testPathPattern=builtin — 빌트인 패턴 테스트 통과

🤖 Generated with Claude Code

[github-actions]

🤖 Dialectic PR Review

📊 리뷰 메트릭

• 전략: SMALL PR
• 변경 파일: 5개
• 변경 라인: +234/-0
• 영향 영역: 🧪 Tests
• Consensus: ✅ LGTM (FP Rate: 100%)
• 플래그: 🧪 테스트 포함

---

=== STEP 1: REVIEW AGENT ANALYSIS ===

As HAWK (critical reviewer), analyzing all changes in this PR:

Issue 1 (maintainability): YAML-like syntax in template literal strings uses non-standard [CRITICAL] tag notation that isn't valid YAML and could confuse consumers of these instructions.

• File: src/frameworks/react.ts, Line: 15-17
• Evidence: waterfalls_and_async: [CRITICAL] and bundle_size: [CRITICAL] — these are template literal strings formatted as pseudo-YAML, and [CRITICAL] after a key with no value is not valid YAML syntax.
• Impact: If any downstream consumer attempts to parse these instructions as YAML, it would fail. The [CRITICAL] tag is ambiguous — is it a value or an annotation?

Issue 2 (maintainability): Inconsistent category assignments across similar pattern types.

• File: src/false-positive/builtin-patterns.ts, Lines: 341, 355
• Evidence: react-component-inside-component is "performance" while nextjs-suspense-boundary is "validation" and nextjs-rsc-serialization is "performance". The Suspense boundary pattern is arguably a performance pattern (streaming/parallel loading), yet it's categorized as "validation".
• Impact: Inconsistent categorization could lead to incorrect filtering or reporting when patterns are grouped by category.

Issue 3 (maintainability): No test coverage for the new framework-specific false positive patterns added in nextjs.ts and react.ts via getFalsePositivePatterns() being tested against the builtin patterns registry.

• File: tests/unit/builtin-patterns.test.ts
• Evidence: Tests exist for react-component-inside-component and react-barrel-import (builtin patterns), and tests/unit/frameworks.test.ts tests framework-specific patterns like nextjs-react-cache-dedup. However, there's no integration test verifying that builtin patterns and framework patterns don't have ID collisions.
• Impact: If a builtin pattern and a framework pattern share the same ID, one could silently shadow the other.

Issue 4 (bug): The falsePositiveIndicators field name in builtin patterns differs from the system prompt's ignore_if_review_contains field, suggesting a potential schema mismatch.

• File: src/false-positive/builtin-patterns.ts, Line: 345
• Evidence: New patterns use falsePositiveIndicators (e.g., "nested component is fine") while the system prompt's FALSE_POSITIVE_PATTERNS use ignore_if_review_contains. These appear to be different schemas for the same concept.
• Impact: If the runtime code expects one field name but receives another, false positive detection could silently fail.

Issue 5 (maintainability): The react.ts framework instructions include a js_performance section with rules like use_set_map_for_O1_lookups and avoid_layout_thrashing that are general JavaScript best practices, not React-specific.

• File: src/frameworks/react.ts, Lines: 46-49
• Evidence: js_performance: use_set_map_for_O1_lookups: true, avoid_layout_thrashing: true, hoist_regexp_creation: true
• Impact: These rules being in the React framework could cause them to only apply when React is detected, missing them for vanilla JS or other frameworks.

Issue 6 (maintainability): New patterns lack descriptions of what the reviewer should do when encountering the pattern — they only describe what the pattern IS, not what action to take.

• File: src/frameworks/react.ts, Lines: 109-138
• Evidence: react-simple-usememo explanation says "Simple expressions do not need useMemo wrapping" but doesn't specify whether to flag or ignore when this is seen.
• Impact: Minor — the context makes it clear these are false positive patterns (i.e., "don't flag this"), but explicit directionality would improve clarity.

---

=== STEP 2: DEV AGENT CHALLENGE ===

Issue 1 Challenge: The [CRITICAL] notation in template literal strings is consumed by an LLM prompt, not a YAML parser. These strings are review instructions injected into prompts. The [CRITICAL] tag is a human/LLM-readable priority marker, not meant to be machine-parsed as YAML. This is a common pattern in prompt engineering.
→ REJECT (reason: These are prompt template strings, not YAML files. The notation is intentional for LLM consumption and has no parsing risk.)

Issue 2 Challenge: Category assignments are subjective. nextjs-suspense-boundary being "validation" could mean "this is a valid pattern" (i.e., validating that Suspense usage is correct), while "performance" means "this is a performance optimization pattern." The existing codebase already mixes these semantics — e.g., nextjs-default-export is "validation". This is consistent with the existing convention where "validation" means "this is a valid/correct pattern."
→ REJECT (reason: Follows existing convention in the codebase where "validation" means "this is a valid pattern, don't flag it." Consistent with nextjs-default-export and similar entries.)

Issue 3 Challenge: ID collision between builtin and framework patterns is a theoretical concern. The naming convention already prevents this — builtin patterns use generic prefixes (react-, nextjs-) while framework patterns use more specific IDs (nextjs-react-cache-dedup, react-simple-usememo). Additionally, this is an additive change; if collisions were a systemic risk, it would have manifested already.
→ REJECT (reason: Theoretical concern with no evidence of actual collision. Naming conventions provide sufficient separation. ROI of adding collision tests is low for this PR.)

Issue 4 Challenge: Looking more carefully at the code, falsePositiveIndicators is the field name used consistently across ALL builtin patterns in the existing codebase (visible in the diff context). The system prompt's ignore_if_review_contains is a separate configuration format used in the SYSTEM prompt itself, not in the TypeScript code. These are two different systems — the system prompt configures the reviewer's behavior, while the TypeScript code defines patterns programmatically. There's no schema mismatch; they're different representations.
→ REJECT (reason: Two different systems with different schemas. falsePositiveIndicators is the consistent TypeScript code field; ignore_if_review_contains is the system prompt format. No mismatch exists.)

Issue 5 Challenge: While use_set_map_for_O1_lookups is general JS, it's placed under React's review instructions because React re-renders make O(n) lookups in render paths particularly costly. Similarly, avoid_layout_thrashing is especially relevant in React because of how React batches DOM updates. The context of React makes these general rules more impactful. Also, this is a framework-specific prompt — it's reasonable to include JS performance tips that are especially relevant in that framework's context.
→ REJECT (reason: General JS performance rules are contextually more important in React due to re-rendering. Placing them here is a reasonable architectural choice, not a bug.)

Issue 6 Challenge: All patterns in getFalsePositivePatterns() are by definition false positive patterns — their purpose is to prevent the reviewer from flagging valid code. The explanation field describes WHY it's a false positive. The falsePositiveIndicators describe WHAT incorrect review comments look like. The action is implicit: "if you see this pattern, don't flag it." Adding explicit directionality would be redundant.
→ REJECT (reason: The method name getFalsePositivePatterns and the field names make the intent clear. Adding explicit "don't flag this" would be redundant boilerplate.)

---

=== STEP 3: OUTPUT ===

📋 Executive Summary
This PR adds comprehensive React and Next.js best practice patterns to the false positive detection system, covering modern patterns like React.cache() deduplication, Server Action authentication, Suspense boundaries, barrel imports, derived state, and RSC serialization. The changes span builtin patterns, framework-specific configurations, and corresponding test coverage. All additions are well-structured and consistent with existing conventions.

🔴 Critical Issues
✅ Critical 이슈 없음

🟡 Important Issues
✅ Important 이슈 없음

✅ 긍정적인 점

• Thorough test coverage: Every new pattern and framework rule has corresponding unit tests in both builtin-patterns.test.ts and frameworks.test.ts, ensuring regressions are caught early.
• Consistent structure: New patterns follow the exact same schema (id, category, explanation, falsePositiveIndicators) as existing patterns, making the codebase uniform and predictable.
• Practical, modern patterns: The additions cover real-world patterns that developers encounter daily (React.cache dedup, Server Action auth, barrel imports, derived state during render) — these will meaningfully reduce false positive noise in reviews.

📊 Final Verdict
결론: LGTM ✅
머지: 즉시 가능
우선순위: P2 (일반)

---

🪄 Generated by Claude Opus 4.6 · Dialectic PR Review
💡 Tip: skip-ai-review 라벨을 추가하면 자동 리뷰를 건너뜁니다