chore(mobile): graduate react-hooks/refs to error - #408
Conversation
Convert the useRef-lazy-init reads-during-render (and render-phase ref writes) that the rule flagged into stable useState instances / effects, then flip react-hooks/refs from warn to error. - settings-row Switch: thumbProgress lazy useRef -> useState initializer - (tabs)/index bulkBarAnim: lazy useRef -> useState initializer - today-habits-header search focusAnim: lazy useRef -> useState initializer - bottom-sheet-modal openRef: render-phase write -> useEffect mirror - use-goal-progress-form-state: render-phase ref clear -> effect on resetKey All five are behavior-equivalent (same initial Animated.Value, same ref value); this keeps future ref-read-during-render regressions out of CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
There was a problem hiding this comment.
Code Review: PR #408 — chore(mobile): graduate react-hooks/refs to error
Scope: PR #408 in thomasluizon/orbit-ui-mobile
Recommendation: APPROVE
Summary
This PR flips react-hooks/refs from warn to error in apps/mobile/eslint.config.js after converting the 5 flagged files away from the useRef-lazy-init-read-during-render anti-pattern: four useRef-lazy-init Animated.Value reads during render became useState(() => new Animated.Value(...)) lazy initializers, and two render-phase ref writes (openRef.current = open, pendingProgressDismissRef.current = null) were moved into useEffects keyed on the value that drives them. I read all five converted files plus the eslint config in full and traced each conversion against its call sites; every one preserves the original value and timing closely enough that no observable behavior changes.
Findings
Critical
None
High
None
Medium
None
Low / Info
- [Info] Minor effect-timing shift, not a bug —
apps/mobile/components/bottom-sheet-modal.tsx:66-68andapps/mobile/components/goals/goal-detail-drawer/use-goal-progress-form-state.ts:84-86: both former render-phase ref writes now land one commit later (insideuseEffect, after paint, instead of synchronously during render). In both cases the only readers of the ref (handleDidDismiss's native callback;confirmProgressDismiss/cancelProgressDismiss, gated behind a discard dialog that's already hidden by the same render pass) can't observe the stale window in practice. No fix needed; flagging only because it's the one place the conversion is not byte-for-byte behavior-identical in the strictest sense. - [Info]
useStatevs. the file's existinguseMemopattern —apps/mobile/app/(tabs)/index.tsx:215andapps/mobile/components/today/today-habits-header.tsx:81now sit next to siblingAnimated.Values created viauseMemo(() => new Animated.Value(...), [])(e.g.dateLabelAnim,refreshSpinAnim) a few lines away.useStateis arguably the more defensible choice here (React only documentsuseMemoas a performance hint, not an identity guarantee —apps/mobile/CLAUDE.md's "UseuseMemoto create stable instances" guidance is technically the weaker of the two), but the two patterns now coexist for the same purpose in the same files. Not worth blocking on; worth a follow-up note inapps/mobile/CLAUDE.mdif the team wants one canonical pattern.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — mobile-only tooling/hook-pattern refactor; no apps/web surface exists for a React-Native-only ESLint rule graduation, and no logic/feature change crossed the platform boundary |
| i18n-syncer | N/A — no user-facing strings touched |
| contract-aligner | N/A — no packages/shared/src/types/*, endpoints.ts, or orbit-api changes |
| security-reviewer | N/A — no orbit-api code touched |
Validation
| Check | Result |
|---|---|
| Lint | N/A — skipped per CI adaptation (this PR's own Build/Unit Tests/SonarCloud checks cover it); author reports npm run lint -w @orbit/mobile exit 0, zero react-hooks/refs findings |
| Type check | N/A — same as above; author reports exit 0 |
| Tests | N/A — same as above; author reports 960 tests / 185 files passing |
| Build (api) | N/A — orbit-api untouched |
Deferred — N/A dimensions & files not verdicted
- DESIGN.md / AI-slop (#8): diff touches
apps/*UI files but adds no new JSX/markup/styles/colors — only hook-internals changed — so there is nothing for the DESIGN.md token/AI-slop checks to evaluate. - Parity (#9), i18n (#10), Contract drift (#11), Security (#12), Backend hard rules (#13), FEATURES.md (#14): all N/A per the table above / gates not triggered — reasons stated inline, not silently skipped.
- Lint/type-check/test execution: not re-run in this review per the CI adaptation (separate required checks own this); relied on the author's stated verification plus static reading of every changed line.
- Every one of the 6 changed files (
app/(tabs)/index.tsx,bottom-sheet-modal.tsx,use-goal-progress-form-state.ts,today-habits-header.tsx,settings-row.tsx,eslint.config.js) was read in full and given a verdict above — nothing changed was left unexamined.
What's good
- Root-cause fix, not a suppression: the PR converts the actual anti-pattern (ref read/write during render) instead of just silencing the rule or leaving it at
warnindefinitely. - Grep across the repo post-conversion turns up zero remaining
useRef<...>(null)lazy-init patterns, consistent with the claim that all 19 warnings are cleared before the rule graduates toerror— nothing was left behind to start failing CI. - Each conversion is minimal and scoped: no drive-by refactors, no unrelated formatting churn, comment on the graduated rule updated to explain the new state.
Recommendation
Approve as-is. The two Info notes above are optional follow-ups (a one-line apps/mobile/CLAUDE.md clarification on useState vs useMemo for stable Animated.Value instances), not blockers.



What
Graduates the
react-hooks/refsESLint rule inapps/mobile/eslint.config.jsfromwarntoerror, after converting the 5 files it flagged (19 warnings) away from the useRef-lazy-init-read-during-render anti-pattern.The 5 conversions (all behavior-equivalent)
components/ui/settings-row.tsx(Switch) —thumbProgresslazyuseRefinit/read →useState(() => new Animated.Value(on ? 1 : 0)).app/(tabs)/index.tsx—bulkBarAnimlazyuseRefinit/read →useStateinitializer.components/today/today-habits-header.tsx(search bar) —focusAnimlazyuseRefinit/read →useStateinitializer.components/bottom-sheet-modal.tsx—openRef.current = openrender-phase write → mirrored in auseEffectkeyed onopen(the callback read is unchanged).components/goals/goal-detail-drawer/use-goal-progress-form-state.ts— render-phasependingProgressDismissRef.current = nullmoved out of the adjusting-state block into auseEffectkeyed onresetKey; the callback-based ref writes/reads are untouched.Each
useStatelazy initializer preserves the same initialAnimated.Value, and the effect-mirrored refs hold the same value they did before — no runtime behavior changes.Why
The stable-instance / effect pattern is what the mobile
CLAUDE.mdmandates for React 19 / React Compiler. Flipping the rule toerrorprevents future ref-read-during-render regressions from slipping through as ignorable warnings.Verification
npm run lint -w @orbit/mobile→ exit 0, zeroreact-hooks/refsfindings.npx vitest run --root apps/mobile→ 960 tests pass (185 files).npm run type-check -w @orbit/mobile→ exit 0.🤖 Generated with Claude Code