refactor(lint): enable stateful react-hooks v7 rules - #171
Merged
Conversation
Enable react-hooks/set-state-in-effect and react-hooks/preserve-manual-memoization at 'error' (Group B of change 0003), completing the react-hooks v7 rollout — all five rules now enforced. Fix all set-state-in-effect violations: - app/utils/responsive.ts: useMediaQuery now subscribes via useSyncExternalStore instead of syncing state in an effect. - InputNumberCard / InputDateTimeCard: sync localValue from the entity during render (previous-value guards) instead of in an effect; behavior preserved. - KeepAlive: resolve the cached portal element during render (lazy init + render-time cacheKey sync); the effect only attaches/detaches it. - useWebRTC: tear down on disable via an effect-cleanup return rather than a synchronous cleanup() in the effect body (this violation was introduced by Group A's setPeerConnection(null) in cleanup). preserve-manual-memoization had zero violations (verified post-0001/0004). The only exhaustive-deps suppression (CardConfig.tsx) remains load-bearing. Add tests for useMediaQuery, KeepAlive, and the two input-card sync fixes; the existing useWebRTC test covers the disable-teardown path. Finalize change 0003: tick Group B + Open Question, flip status to complete in the doc and both indexes, and sync the architecture spec lint section.
📝 WalkthroughSummary
WalkthroughReact hook implementations were migrated away from effect-driven state updates, WebRTC teardown was moved into effect cleanup, tests were added for updated behaviors, and React Hooks lint-rule documentation and statuses were completed. ChangesReact Hooks rule compliance
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR completes change 0003 by enabling the remaining stateful eslint-plugin-react-hooks v7 rules (set-state-in-effect and preserve-manual-memoization) at error, and refactors affected hooks/components to comply without adding suppressions. It also updates the project’s change/spec index docs and adds targeted tests for the behavioral refactors.
Changes:
- Enable all five react-hooks v7 rules at
errorineslint.config.js. - Refactor
useMediaQuery,KeepAlive, input-* cards, anduseWebRTCteardown patterns to satisfyreact-hooks/set-state-in-effect. - Add/extend Vitest coverage for the refactored patterns and mark change 0003 as complete in docs.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/hooks/useWebRTC.ts | Moves teardown behavior into effect cleanup keyed by enabled to avoid state updates in effect bodies. |
| src/components/KeepAlive.tsx | Resolves cached portal element during render and narrows the effect to mounting/unmounting the element. |
| src/components/InputNumberCard.tsx | Reworks local edit value syncing to avoid effect-driven setState. |
| src/components/InputDateTimeCard.tsx | Reworks local edit value syncing to avoid effect-driven setState. |
| src/components/tests/KeepAlive.test.tsx | Adds tests for portal caching and moving behavior. |
| src/components/tests/InputNumberCard.test.tsx | Adds tests ensuring entity-driven updates don’t clobber in-progress edits and seed correctly. |
| src/components/tests/InputDateTimeCard.test.tsx | Adds tests ensuring edit field seeds from latest entity state. |
| eslint.config.js | Flips set-state-in-effect and preserve-manual-memoization from off to error. |
| docs/specs/architecture/index.md | Updates the linting scenario to reflect all five rules enforced at error; removes the related open question. |
| docs/index.yml | Marks change 0003 status as complete. |
| docs/index.md | Marks change 0003 status as complete. |
| docs/changes/0003-reenable-react-hooks-rules.md | Marks change 0003 as complete and checks off Group B. |
| app/utils/responsive.ts | Rewrites useMediaQuery using useSyncExternalStore to avoid effect-driven setState and keep query changes in sync. |
| app/utils/tests/responsive.test.ts | Adds tests for reactive updates, query changes, and unsubscribe behavior. |
Comments suppressed due to low confidence (1)
docs/changes/0003-reenable-react-hooks-rules.md:12
- This change doc still states that
eslint.config.jsdisables the react-hooks v7 rules, but the PR enables them aterror. Updating the Motivation bullet avoids leaving the doc in a contradictory state now that status is marked complete.
**Status:** complete
**Depends On:** —
## Motivation
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/KeepAlive.tsx (1)
13-27: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDon't mutate
portalCacheduring render. Creating and storing the portal element here can leave detached nodes behind if React abandons a render, and the cache never evicts entries. Move creation/registration to a commit-safe effect with cleanup or eviction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/KeepAlive.tsx` around lines 13 - 27, Update getOrCreatePortalElement and the KeepAlive lifecycle so portalCache is not mutated during render. Create and register each portal element from a commit-safe effect, then remove the cache entry and detach or clean up the element when the owning component unmounts or the key changes, while preserving cached reuse for committed instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/KeepAlive.tsx`:
- Around line 13-27: Update getOrCreatePortalElement and the KeepAlive lifecycle
so portalCache is not mutated during render. Create and register each portal
element from a commit-safe effect, then remove the cache entry and detach or
clean up the element when the owning component unmounts or the key changes,
while preserving cached reuse for committed instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3d921e6-5888-450f-9ae8-4f755ef6ae3b
📒 Files selected for processing (1)
src/components/KeepAlive.tsx
This was referenced Jul 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Group B of docs/changes/0003-reenable-react-hooks-rules.md — the final PR for the change. Enables
react-hooks/set-state-in-effectandreact-hooks/preserve-manual-memoizationaterror; all five react-hooks v7 rules are now enforced with zero violations and zero suppressions added.Post-merge audit found 5
set-state-in-effectviolations (the 4 inventoried in the Group A PR, plus one introduced by Group A's own teardown restructure inuseWebRTC);preserve-manual-memoizationconfirmed at 0.Fixes (underlying patterns, no blanket suppressions)
app/utils/responsive.ts—useMediaQueryrewritten onuseSyncExternalStore(no effect at all; re-syncs on query change via snapshot).InputNumberCard.tsx/InputDateTimeCard.tsx—localValuesyncs from the entity during render with previous-value guards reproducing the old effect's triggers; in-progress edits preserved.KeepAlive.tsx— cached portal element resolved during render (lazy init + render-time cache-key sync); the effect now only mounts/unmounts the element.useWebRTC.ts— teardown moved to a dedicatedenabled-keyed effect's cleanup return (setState in effect-cleanup is allowed); disable/unmount behavior unchanged, validated by the existing reactive-teardown test.exhaustive-depssweep: onlyCardConfig.tsx:434remains and is still load-bearing; left in place.Completes change 0003
error, matching the zero-warning bar), Status → complete.docs/index.yml/docs/index.md: 0003 → complete.Testing
npm test— 539 passed, 2 pre-existing skips (+8 new tests: responsive, KeepAlive, InputNumberCard, InputDateTimeCard; no existing tests modified)npm run lint— 0 errors, 0 warningsnpm run typecheck— cleannpm run build:ha:prod— passes