chore(ui): drive React Doctor to zero in web components/hooks (#243) - #503
Conversation
React Doctor burn-down for bucket B2 (apps/web/components/** except habits/ & ui/, plus apps/web/hooks/**): 105 findings driven to 0. Fixed properly: - effect-needs-cleanup (6): gamification celebration/toast timers now track nested timers in the same ref and return a cleanup from the timer-creating effect (removed redundant unmount-only effects). - prefer-use-effect-event (3): wrapped changing prop callbacks in useEffectEvent so effects no longer re-subscribe on parent redraws. - use-lazy-motion (7): swapped motion -> m + <LazyMotion>; domAnimation everywhere except route-transition-shell (popLayout needs domMax). - no-inline-exhaustive-style (13): hoisted static style objects to module-scope CSSProperties consts. - no-tiny-text (16 + extras): bumped sub-12px text to the DESIGN.md 12px type-role floor (truncate/ellipsis contexts, so no overflow). - only-export-components (4): removed unused exports; extracted the shared toSectionStatus/SectionStatus and trendHeadline helpers into insights-section-status.ts and insights-headline-model.ts. - no-giant-component (1): extracted NotificationRow from NotificationBell. - no-prop-callback-in-render (3): goal-drawer deep-link actions moved from render into a single guarded effect. - no-impure-state-updater (1): login resend countdown side effects moved out of the setState updater into the interval callback (ref-driven). - query-mutation-missing-invalidation (1): marketing consent mutation now reconciles via onSettled invalidate. - js-combine-iterations (3), js-hoist-intl (2), no-array-index-as-key (2), button-has-type (1), control-has-associated-label (1), rerender-lazy-state-init (1), rerender-state-only-in-handlers (1), prefer-module-scope-static-value (1), client-localstorage-no-version (1), no-locale-format-in-render (1, via shared useDateFormat). Justified suppressions (react-doctor-disable-next-line + #243 WHY URL, only where the rule is a genuine false positive or deliberate choice): - exhaustive-deps (14): deps already track query.data/profile.* via an alias react-doctor does not resolve; adding the raw member expression is semantically identical and uglier. - no-unguarded-browser-global (4): createPortal/localStorage reached only behind a client-only render gate (useIsClient / viewport / the repo's canonical 'localStorage' in globalThis guard). - query-mutation-missing-invalidation (2): report-user and suggest-tags mutate no client-cached data. - prefer-html-dialog (2), no-large-animated-blur (2): desktop-only non-modal rail / FAB; native dialog and phone GPU concerns don't apply. - no-many-boolean-props (1): four orthogonal range-picker state flags; effect-needs-cleanup (1): tour observer lifecycle is ref-managed; nextjs-no-client-side-redirect (1): tour orchestrator navigation; no-pass-data/live-state-to-parent (2): reusable code-entry primitive. Refs #243 (React Doctor burn-down: web components/hooks) 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
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
/pr-review — PR #503
chore(ui): drive React Doctor to zero in web components/hooks (#243) — web-only mechanical React Doctor burn-down (motion→m/LazyMotion, hoisted style consts, sub-12px→12px floor, stable keys, useEffectEvent, extracted helpers) across apps/web/components/** + apps/web/hooks/**.
Decision: Request changes
Two High findings — the mandatory web↔mobile parity rule (root CLAUDE.md) is violated by two of the "behavior-adjacent" fixes bundled into this otherwise-mechanical pass. Everything else in the diff is clean.
Findings
[High] Marketing-consent cache invalidation fix not mirrored to mobile
· dimension: 9 — Parity (web ↔ mobile)
· location: apps/mobile/components/marketing-consent/marketing-consent-prompt.tsx:52-74
· issue: Web added onSettled: () => invalidate() to the consent-update mutation (apps/web/components/marketing-consent/marketing-consent-prompt.tsx:57-62), fixing a real query-mutation-missing-invalidation bug — the profile cache never reconciled with the server after a consent PUT. Mobile's useMutation block has no onSettled, even though useProfile() already exposes invalidate on mobile (apps/mobile/hooks/use-profile.ts:43-59) — the fix is a one-line, zero-adapter port.
· risk: Mobile's profile cache stays stale after every consent toggle; a background refetch or navigation can revert the UI to a value that no longer matches the server.
· fix: Add invalidate to the mobile useProfile() destructure and onSettled: () => invalidate() to the mutation, mirroring the web change.
· reference: root CLAUDE.md "Cross-platform parity (MANDATORY)"
[High] Impure countdown state updater not fixed on mobile
· dimension: 9 — Parity (web ↔ mobile)
· location: apps/mobile/hooks/use-login-code-entry.ts:54-63
· issue: Web refactored the resend-countdown interval to move setCanResend(true) / clearResendTimer() out of the setResendCountdown functional updater and into a ref-backed pure update (apps/web/hooks/use-login-code-entry.ts:69-88), fixing a no-impure-state-updater violation — React may invoke a functional updater more than once (Strict Mode, concurrent features), and the old code fired side effects (setCanResend, clearResendTimer) from inside it. Mobile's use-login-code-entry.ts is otherwise byte-identical to web's pre-fix version and still has the same impure updater at lines 55-62.
· risk: Under React 19's concurrent re-invocation of updaters, clearResendTimer()/setCanResend(true) can fire more than once per tick, or the interval can be cleared prematurely — a live correctness bug on the exact platform (mobile login OTP entry) most likely to hit slow/replayed renders.
· fix: Port web's ref-driven fix verbatim (resendCountdownRef tracking the count, side effects only in the setInterval callback body, not the updater).
· reference: root CLAUDE.md "Cross-platform parity (MANDATORY)"
Both gaps are acknowledged in the PR body ("the behavior-adjacent fixes … will be matched by the mobile buckets hitting the same rules") — but root CLAUDE.md requires parity fixes to land "in the same task," with no carve-out for a tracked follow-up bucket. The third behavior-adjacent item (goal-drawer deep-link initialAction move-out-of-render, apps/web/components/goals/goal-detail-drawer/use-goal-drawer-initial-action.ts) is correctly N/A — mobile has no equivalent deep-link-driven drawer action at all, so there's nothing to mirror.
[Medium] GoalStatusBadge font-size bump breaks the locked Badge primitive spec
· dimension: 8 — DESIGN.md / AI-slop
· location: apps/web/components/goals/goal-status-badge.tsx:8,17
· issue: This PR's mechanical "sub-12px floor" pass bumped GoalStatusBadge from 10.5px→12px and rewrote its JSDoc to claim "12/600" as the spec. But DESIGN.md's Primitives table locks the Badge shape at 10.5/600 (DESIGN.md:127), and apps/web/components/ui/badge.tsx:41 still correctly renders fontSize: 10.5. The mechanical floor rule overrode a primitive with its own explicit locked dimension without checking it against the primitives table.
· risk: Two divergent badge sizes now exist for the same visual primitive (10.5 in ui/badge.tsx, 12 in goal-status-badge.tsx); goal-status pills no longer match sibling badges pixel-for-pixel anywhere else in the app. Not a layout/truncation risk (confirmed shrink-0 next to truncate/min-w-0 siblings) — the defect is the token/spec drift itself.
· fix: Revert goal-status-badge.tsx to 10.5px (and its JSDoc) to match the locked Badge primitive, or — if 12px is intentionally correct — update DESIGN.md:127's Badge spec and ui/badge.tsx to match, in the same PR.
· reference: DESIGN.md:127
Dimensions checked, no findings
- Correctness — spot-checked the non-mechanical refactors (
use-goal-drawer-initial-action.tsrender→effect consolidation,breakdown-suggestion.tsxstable-id keys,pending-operation-card.tsxstate→ref forconfirmationToken, gamification celebration effect/cleanup consolidation) — all behaviorally equivalent to their prior render paths, cleanup preserved. - Dead/stale code —
only-export-componentsremovals (goalTypeOptions,copyToClipboard) and theinsights-section-status.ts/insights-headline-model.tsextractions are correctly re-wired at all call sites. - Comment policy — all
react-doctor-disable-next-linesuppressions carry a WHY + issue-#243 URL perno-comments.cjs. - Type safety / console.log — none introduced.
- DESIGN.md (rest) — no raw
--slate-*, no hardcoded violet rgba, notransition-all/h-screen, allmotion→mmigrations correctly wrapped inLazyMotion. Em dashes found incalendar-time-grid.tsxare inside a JSDoc comment, not user-facing copy. - i18n — no new user-facing strings; diff doesn't touch either locale file. N/A.
- Contract drift / backward-compat — diff touches no
packages/sharedtypes or orbit-api. N/A (orbit-api not checked out in this job either way). - Security — no orbit-api code touched. N/A.
- Backend hard rules — N/A, no orbit-api.
- FEATURES.md parity — pure refactor/bugfix bucket, no feature surface change. N/A.
Not verifiable in CI
- Cross-repo contract-aligner / orbit-api side of backward-compat guard — sibling repo not checked out. N/A for this diff regardless (no shared-type changes).
#243) - Mirror the two behavior fixes to mobile (mandatory web/mobile parity): - marketing-consent: add `onSettled: () => invalidate()` so the profile cache reconciles after a consent PUT (was web-only). - use-login-code-entry: port the ref-driven resend countdown so the setState updater stays pure (no side effects re-run under concurrent re-invocation). - Revert the sub-12px "floor" bump on two DESIGN.md-locked primitives and justifiably suppress no-tiny-text instead: - GoalStatusBadge back to 10.5/600 (Badge primitive, DESIGN.md:127). - TabBar label back to 11 (DESIGN.md:135). Refs #243 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed all three review findings in
React Doctor: file-set still at 0 (scoped scan clean, incl. the two new suppressions). Web lint/typecheck and mobile lint/typecheck clean (the one mobile tsc error is a pre-existing missing |
Addressed in 3391af6 (mobile parity mirrors for the consent invalidate + impure-updater fixes; GoalStatusBadge/TabBar reverted to the locked DESIGN.md primitive sizes with justified no-tiny-text suppressions). The claude re-review ran against 3391af6 and passed clean with no findings; dismissing this verdict on the superseded commit 47aab54.
…ts-rest # Conflicts: # apps/mobile/hooks/use-login-code-entry.ts
|
There was a problem hiding this comment.
/pr-review — PR #503
chore(ui): drive React Doctor to zero in web components/hooks (#243) — web-only mechanical React Doctor burn-down (motion→m/LazyMotion, hoisted style consts, sub-12px→12px floor, stable keys, useEffectEvent, extracted helpers) across apps/web/components/** + apps/web/hooks/**.
Decision: Approve
Follow-up on prior review
The previous automated review (dismissed) flagged two High findings — mobile parity gaps for the marketing-consent cache invalidation and the login-countdown pure-updater fix. Both are confirmed resolved in commit 3391af6e ("fix(ui): address PR review - mobile parity + locked Badge/TabBar sizes"):
apps/mobile/components/marketing-consent/marketing-consent-prompt.tsxnow destructuresinvalidatefromuseProfile()and calls it inonSettled, matching web.apps/mobile/hooks/use-login-code-entry.tsnow uses the same ref-driven (resendCountdownRef) pure updater as web — side effects (clearResendTimer,setCanResend) moved out of the functionalsetStateupdater.
No outstanding parity gaps found in this diff.
Summary
React Doctor lint burn-down for apps/web/components/** (excluding habits/ and ui/) + apps/web/hooks/**, driving 105 findings to zero. Overwhelmingly mechanical: hoisting inline style={{...}} objects to module-scope CSSProperties consts, motion → m + <LazyMotion> code-splitting, a 12px type-role floor for sub-12px text, stable list keys, useEffectEvent wrappers, effect-cleanup fixes, and justified react-doctor-disable-next-line suppressions each carrying a WHY note linking #243. Two behavior-preserving refactors (login resend-countdown updater, goal-drawer deep-link effect) and one real bug fix (marketing-consent cache invalidation on settle) are included, and both behavior-adjacent fixes are correctly mirrored to apps/mobile (verified above). Dead-export removals (copyToClipboard, goalTypeOptions) have zero remaining importers, and the insights-section/insights-headline module split rewired every call site correctly.
Findings
Critical
None
High
None
Medium
None
Low / Info
- Info —
apps/web/components/settings/public-profile-settings.tsx:9-19: a newconst regenerateButtonStyle: CSSProperties = {...}declaration sits between twoimportstatements. ES module imports hoist regardless of lexical position so this is functionally inert, and noimport/order/import/firstlint rule is enabled in this workspace — pure code-organization nit, not blocking.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — both behavior-adjacent fixes mirrored to mobile (confirmed via commit 3391af6) |
| i18n-syncer | N/A — no new i18n keys; the one new aria-label reuses an existing habits.form.frequency key present in both locales |
| contract-aligner | N/A — no packages/shared/src/types/* or orbit-api DTO changes |
| security-reviewer | N/A — no orbit-api changes |
| design-reviewer | PASS |
Design detail: both DESIGN.md citations in suppression comments (goal-status-badge.tsx → DESIGN.md:127 Badge 10.5/600; bottom-tab-bar.tsx → DESIGN.md:135 TabBar label 11) are accurate. Font-size bumps to the 12px floor land in truncating/fixed-width contexts with no overflow risk observed. No raw/off-token colors introduced by the style-object hoists.
Validation
| Check | Result |
|---|---|
| Lint | N/A — not runnable in this review sandbox; PR author reports clean |
| Type check | N/A — same restriction; PR author reports clean tsc --noEmit |
| Tests | N/A — same restriction; PR author reports vitest run 2246 pass |
| Build (api) | N/A — orbit-api not touched, not checked out |
Manual spot-verification performed in place of the toolchain: grepped for stray importers of the two de-exported helpers (copyToClipboard, goalTypeOptions) — zero found; grepped every ./insights-section import site to confirm toSectionStatus/trendHeadline rewired all 7 consumers; confirmed both DESIGN.md line citations; diffed apps/mobile/hooks/use-login-code-entry.ts and apps/mobile/components/marketing-consent/marketing-consent-prompt.tsx against their web counterparts to confirm the parity fixes landed correctly.
Deferred — N/A dimensions & files not verdicted
- Contract drift + backward-compat (#11) — N/A, no
packages/shared/src/types/*or orbit-api DTO touched. - Backend hard rules (#13) — N/A, orbit-api not in this diff/checkout.
- FEATURES.md parity (#14) — N/A, no user-facing feature added/changed/removed.
- Validation (Phase 7) — could not execute lint/type-check/test in this sandbox (package-manager execution requires unavailable approval); relied on PR's self-reported results plus targeted manual verification.
- All changed files were read and given a verdict; nothing was skipped silently.
What's good
- Every suppression comment carries a specific, verifiable WHY (aliasing explanation, DESIGN.md line citation, or architectural rationale) plus the tracking issue URL.
- Both parity gaps from the prior review round were fixed correctly and verified byte-for-byte against the web implementation.
- Dead-export removals were verified against zero remaining call sites.
Recommendation
Merge as-is. Optionally, in a follow-up, move regenerateButtonStyle in public-profile-settings.tsx below the import block — cosmetic only.
…est + shared (#243) (#506) React Doctor burn-down for bucket B5 (apps/mobile/components/** except ui/, habits/, habit-list*; plus packages/shared/src/**). Drives the file-set to 0 findings — fixing properly where safe and using justified react-doctor-disable-next-line suppressions (each linking #243) only for genuine false positives or deliberate, coupled choices. Fixed (behavior-preserving): - shared: memoized Intl formatter cache (intl-format-cache.ts) wired into locale-format/goal-metrics/subscription-pricing (js-hoist-intl); filter+map -> flatMap single passes (js-combine-iterations); direct imports off barrels (no-barrel-import). - mobile: shadow* glow -> boxShadow on the 5 celebration overlays (parity with web's boxShadow), matching the shared primary/frozen tint; useEffectEvent for deferred timer/debounce callbacks (achievement-toast, level-up-overlay, welcome-back-toast, today-habits-header); real setTimeout cleanup in welcome-back-toast; TouchableOpacity -> Pressable (chat-input-area, goal/edit-goal deadline fields, goal-detail-sections); Set lookup for tag filter; lazy useState init (breakdown-suggestion); state -> ref for handler-only confirmationToken (pending-operation-card); module-local non-component exports (goal-type-selector, notification-row); onSettled: invalidate() on the marketing-consent-section mutation (matches #503's marketing-consent-prompt fix); precise exhaustive-deps (goal-detail-drawer, use-goal-status-actions, referral-drawer, tour-provider). Suppressed with WHY (FP / deliberate): rn-prefer-reanimated x21 (pinned worklets 0.10.0 / reanimated 4.5.0 ABI, coupled to shared lib/motion.ts); rn-prefer-expo-image x4 (expo-image not installed; transient/static images); tour-tooltip shadowsV2.shadow3 offset override; today-habits-header + chat-input-bar + pricing-section no-many-boolean-props; scrollview chip rows; tour-provider no-giant-component + rn-no-dimensions-get; welcome-back-toast effect-needs-cleanup (RD can't trace the clear through the async helper); error-utils js-set-map-lookups (String.includes, not array membership); today-habits-header prefer-explicit-variants. Refs #243 (React Doctor burn-down: mobile rest + shared) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What
React Doctor burn-down for bucket B2 —
apps/web/components/**(excepthabits/&ui/) +apps/web/hooks/**. Drives all 105 React Doctor findings in that file-set to 0 (verified by a fullapps/webre-scan).Fixed properly
motion→m+<LazyMotion>;domAnimationeverywhere exceptroute-transition-shell(mode="popLayout"needsdomMax)CSSPropertiesconststruncate/ellipsis contexts, so no overflow)useEffectEventtoSectionStatus/SectionStatus→insights-section-status.tsandtrendHeadline→insights-headline-model.ts(7 + 2 importers rewired)onSettledinvalidateNotificationRowfromNotificationBellflatMap, memoized/hoistedIntl, stable keys, explicittype, aria-label, lazy init,useRef, hoisted const, versioned key, shareduseDateFormat)Fixed: ~78 · Suppressed: ~27
Justified suppressions
react-doctor-disable-next-line+ a WHY comment linking #243, only where the rule is a genuine false positive or a deliberate design choice:query.data/profile.*through an alias (const profile = query.data) that React Doctor does not resolve; adding the raw member expression is semantically identical and uglier. Never blanket-added a dep.createPortal/localStoragereached only behind a client-only gate (useIsClient, a measured-viewport gate, or the repo's canonical'localStorage' in globalThisguard, chore(ui): fix Sonar smells S6479/S4624/S7741 (keys, nested templates, typeof-undefined) #490).useReportUser/useSuggestTagsmutate no client-cached data.hidden md:*FAB; native<dialog>semantics and the phone-GPU concern don't apply.resetSessionState.Verification
npm run lint(apps/web) — clean (3 pre-existing warnings in files I didn't touch)tsc --noEmit— cleanvitest run(apps/web) — 2246 pass (2 pre-existing server-fetch timing flakes inlib/, outside this bucket, self-heal on--retry)--scope changed --blocking error) — 0 newapps/webReact Doctor re-scan — 0 findings remain in the B2 file-setNotes
__tests__/.../marketing-consent-prompt.test.tsxupdated because theuse-lazy-motionchange requires themotion/reactmock to exposem/LazyMotion/domAnimation(+ theuseProfilemock'sinvalidatefor the newonSettled).rn-*); the behavior-adjacent fixes (consent invalidate, impure-updater, prop-callback) will be matched by the mobile buckets hitting the same rules.Refs #243 (React Doctor burn-down: web components/hooks)
🤖 Generated with Claude Code