chore: strengthen purity and ref rules for Concurrent Mode (#694)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: orionmiz

.agents/skills/review-react/SKILL.md

@@ -64,6 +64,22 @@ Reference these guidelines when:
 - `advanced-event-handler-refs` - Store latest event handlers in refs for stable callbacks
 - `advanced-init-once` - Initialize app-level singletons once, not per mount
 
+## Review Discipline
+
+### Never downgrade CRITICAL violations
+
+When a CRITICAL rule violation is detected (e.g., `react-rules-purity`), **fix it — do not rationalize exceptions**. Common rationalizations to reject:
+
+- "It's idempotent, so Strict Mode double-render is fine" — Strict Mode is not the only concern; Concurrent Mode render abandonment is the real danger.
+- "It works in practice" — Concurrent features may not be active today but the code must be correct when they are.
+- "Adding a comment explaining the intent is sufficient" — A comment does not prevent the bug. If the rule says "don't do X", the fix is to stop doing X.
+
+If you detect a violation, move to "fix" before moving to "judge severity." The cost of a false positive (unnecessary refactor) is far lower than the cost of a false negative (shipping a Concurrent Mode bug).
+
+### Re-check after refactors
+
+When a fix for one issue changes the code structure (e.g., adding `callback` to useEffect deps), **re-run the full rule check** on the modified code. A fix for one rule can regress another — e.g., fixing `rerender-dependencies` can reintroduce a `react-rules-purity` violation if it moves code back into render phase.
+
 ## How to Use
 
 Read individual rule files for detailed explanations and code examples:

.agents/skills/review-react/rules/advanced-event-handler-refs.md

@@ -53,3 +53,18 @@ function useWindowEvent(event: string, handler: (e) => void) {
 ```
 
 `useEffectEvent` provides a cleaner API for the same pattern: it creates a stable function reference that always calls the latest version of the handler.
+
+### When external code reads your ref: useEffect update is mandatory
+
+The "Correct" pattern above (update ref in `useEffect`) is not just an optimization — it becomes a **correctness requirement** when code outside React reads the ref synchronously.
+
+Examples of external readers:
+- Store/state-manager subscribers (e.g., `store.subscribe()`, Zustand/Redux middleware)
+- Event bus listeners (e.g., `EventEmitter.on()`)
+- Framework lifecycle hooks that fire outside React's render cycle
+
+If you update the ref during render instead of in useEffect, Concurrent Mode can abandon that render. The external reader then sees a callback from a render that never committed — a value that doesn't correspond to any real UI state.
+
+**Rule of thumb:** If anything outside React's tree can call `ref.current`, the ref must only be written in `useEffect`. The one-render staleness between render and commit is the correct trade-off — a stale-but-committed callback is always safer than an uncommitted one.
+
+See also: `react-rules-purity` Rule 6 for the full rationale and examples.

.agents/skills/review-react/rules/react-rules-purity.md

@@ -118,4 +118,49 @@ function Page({ title }) {
 }
 ```
 
+### Rule 6: Never write refs during render when external code reads them synchronously
+
+Render-phase ref writes (`ref.current = value`) are dangerous when **code outside React** (store subscribers, event bus listeners, framework lifecycle hooks) can read that ref synchronously between renders.
+
+In Concurrent Mode, React may **abandon** a render and start a new one with different props. If you wrote to the ref during the abandoned render, external readers see a value from a render that never committed — a value that doesn't correspond to any committed UI state.
+
+This is **not** mitigated by the write being "idempotent" — the problem isn't double-writes from Strict Mode, it's writes from renders that are discarded entirely.
+
+**Incorrect (render-phase ref write with external reader):**
+
+```tsx
+function useEventBusHandler(bus: EventBus, event: string, handler: () => void) {
+  const handlerRef = useRef(handler)
+  handlerRef.current = handler // ← render phase write
+
+  useEffect(() => {
+    // External subscriber reads handlerRef on every event
+    return bus.on(event, () => handlerRef.current())
+  }, [bus, event])
+}
+
+// Meanwhile, outside React:
+// bus.emit() fires synchronously on state change — may read a ref
+// written by a render that React later abandoned
+```
+
+**Correct (ref updated only in useEffect):**
+
+```tsx
+function useEventBusHandler(bus: EventBus, event: string, handler: () => void) {
+  const handlerRef = useRef(handler)
+  useEffect(() => {
+    handlerRef.current = handler // ← commit phase only
+  })
+
+  useEffect(() => {
+    return bus.on(event, () => handlerRef.current())
+  }, [bus, event])
+}
+```
+
+The trade-off: between render and commit, the subscriber may read a one-step-old handler. But a stale-but-committed value is always safer than a value from a render that never committed.
+
+**Key principle:** Do not make assumptions about React's scheduling. "This render will commit before anything else happens" is never guaranteed in Concurrent Mode. Any optimization based on that assumption will break.
+
 Reference: [Components and Hooks must be pure](https://react.dev/reference/rules/components-and-hooks-must-be-pure)