chore(web): drive React Doctor to zero across web habits/ui components (#243) - #505
Conversation
#243) Burn down all React Doctor findings under apps/web/components/habits/** and apps/web/components/ui/** (93 findings) to zero for this file-set. Fixed properly (34): - no-inline-exhaustive-style (13): hoisted static style objects to module scope (spreading a static base for the mixed static/dynamic cases). - js-set-map-lookups (5): O(1) Set lookups instead of array.includes() in maps. - button-has-type (5): explicit type="button" on non-submit buttons. - only-export-components (4): extracted pure helpers to sibling modules (app-bar-right-action.ts, date-group-label.ts, empty-state-message.ts) and un-exported the internal-only resolveReminderLabel. - prefer-module-scope-pure-function (2) / prefer-module-scope-static-value (1): moved stateless helpers/styles out of the component body. - no-usememo-simple-expression (1): dropped a useMemo whose ref is never relied on. - rerender-memo-with-default-value (2): hoisted [] / {} defaults to constants. - no-ref-current-in-render (1): moved the latest-ref write into a useEffect. - no-unguarded-browser-global (bulk-action-bar): added a useIsClient guard so the createPortal target is only read on the client (real SSR-crash guard). Justified react-doctor-disable-next-line suppressions (59, WHY + #243): - use-lazy-motion (7): LazyMotion migration is app-wide (shared provider + every motion.* across components/**); a partial per-file swap risks unprovided m. - no-unguarded-browser-global (9): each createPortal is behind an existing useIsClient/mounted guard the rule cannot trace; unreachable during SSR. - exhaustive-deps (10): values derived from profile/query members are recomputed and already listed; the rule unwraps them to the source member (false positive). - no-tiny-text (7): intentional captions/badges/eyebrows per DESIGN.md. - no-giant-component (4): cohesive modal/list surfaces; extraction deferred without visual QA. - dangerous-html-sink (2): DOMPurify-sanitized markdown/link sinks. - no-array-index-as-key (2): fixed-order id-less habit sub-data (checklist, reminders); index disambiguates a composite key. - prefer-html-dialog (2): intentional non-modal bottom-anchored prompts. - no-prop-callback-in-effect (2) / no-prop-callback-in-render (1) / no-pass-data-to-parent (1) / no-pass-live-state-to-parent (1): access-gate redirect and legit cross-component notifications / adjusting-state-during-render. - no-large-animated-blur (2): intentional small glass control per DESIGN.md. - nextjs-no-client-side-redirect (1): gate depends on client-fetched profile. - prefer-tag-over-role (1): row wraps nested interactive controls a <button> cannot legally contain. - rendering-hydration-mismatch-time (1): grid renders only inside a client-only overlay; no-outline-none (1): focus-within ring present; click-events-have-key-events (1): capture-phase wrapper over the real button; no-autofocus (1): search field in a user-invoked picker. Refs #243 (React Doctor burn-down: web habits/ui) 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.
/pr-review — PR #505: chore(web): drive React Doctor to zero across web habits/ui components (#243)
Recommendation: APPROVE
Mechanical React Doctor burn-down (bucket B1: apps/web/components/habits/** + apps/web/components/ui/**, 93 findings → 0) across 47 files. Reviewed the full diff against .claude/skills/pr-review/rubric.md, orchestrating design-reviewer and parity-checker.
Summary by dimension
- Correctness — Two genuine (non-lint) fixes ship alongside the suppressions/hoists, both verified safe:
bulk-action-bar-v2.tsx: adds auseIsClient()mount guard beforecreatePortal, matching the identical pattern already used inAppOverlay,CenteredOverlay,ConfirmDialog,ContextMenu,Popover. This was the one genuinely unguarded portal (real latent SSR-crash fix), not a behavior regression.habit-list.tsx: movespromptDataRef.current = {...}from the render body into auseEffect. Confirmed every read site ofpromptDataRefis a post-mutation event-handler callback, never a render path — the timing change is safe, and it matches mobile'shabit-list.tsx, which already does this inuseEffect(parity-checker verified).
- Dead code —
resolveReminderLabelwas un-exported (only-export-components fix); confirmed by grep it has zero external references and is only used insidehabit-form-fields.tsx. No orphaned exports found. - Comment policy — All 59
react-doctor-disable-next-linesuppressions carry a WHY clause plus anhttps://github.com/.../issues/243URL, satisfyingno-comments.cjs'sURL_REallowance regardless of theDIRECTIVEregex. Spot-checked several non-obvious suppression claims against the surrounding code and all hold:field-input.tsxno-outline-none—focus-within:shadow-[inset_0_0_0_2px_var(--primary)]on the wrapping div does supply the focus ring.back-to-top.tsxno-large-animated-blur— control is 48×48px, blur is static (only opacity/transform transition).trial-expired-modal.tsxno-unguarded-browser-global-in-render-or-hook-init—localStorage.getItemsits behind amounted &&short-circuit earlier in the same expression.
- Type safety / console.log — No
any/as anyescape hatches orconsole.logintroduced. - DESIGN.md / AI-slop (
design-reviewer) — All 12 hoisted style objects checked value-for-value againstDESIGN.md's canon table: semantic tokens only, no raw--slate-*/hardcoded rgba, dimensions match documented primitives (e.g.CODE_BOX_STYLEmatches the OTP spec byte-for-byte). PASS, no token/value drift. - Parity (
parity-checker) — Web-only React Doctor burn-down; React Doctor has no mobile equivalent. The 2 behavior-adjacent changes are correctly N/A: the SSR guard is web-only (mobile has no hydration), and mobile'shabit-list.tsxalready uses theuseEffectpattern. Precedented by the already-merged #501 (web routes) / #502 (mobile hooks) split for the same issue #243. No missing mirrors. - i18n — No new user-facing strings; all
t()calls reference pre-existing keys. - Contract drift / Security / Backend hard rules / FEATURES.md — N/A, no
packages/sharedtype changes, noorbit-apitouched, no new/changed feature surface.
Findings
None at Critical/High. No concretely-actionable Medium findings survived — this is a clean, well-scoped mechanical refactor with two verified-safe incidental fixes.
|
There was a problem hiding this comment.
Code Review: PR #505 — chore(web): drive React Doctor to zero across web habits/ui components (#243)
Scope: PR #505 in thomasluizon/orbit-ui-mobile (47 files, apps/web/components/habits/** + apps/web/components/ui/** + one test file)
Recommendation: APPROVE
Summary
This is a mechanical, lint-driven cleanup (React Doctor findings → 0 for this file-set): hoisting static inline style={{...}} objects to module-scope constants, array.includes() → Set.has() inside .map(), explicit type="button", three pure-helper extractions to sibling modules, one genuine SSR-crash fix (useIsClient guard added to bulk-action-bar-v2.tsx before its createPortal), one ref-write moved from render into useEffect, and ~59 react-doctor-disable-next-line <rule> -- WHY … https://…/issues/243 suppression comments. Every changed hunk across all 47 files was read directly; the highest-risk claims were independently verified rather than trusted from the PR description. No new commits since a prior identical review on this PR — this re-confirms the same conclusion after the design-reviewer and parity-checker subagents (dispatched but slow to return during the original pass) both came back with independent PASS verdicts.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
- Info: all
react-doctor-disable-next-linecomments carry a WHY +https://github.com/…/issues/243URL, satisfyingeslint-rules/no-comments.cjs'sURL_REallowance — spot-checked the rule file directly (eslint-rules/no-comments.cjs:15,22) to confirm this isn't relying on an unenforced convention. - Info: two of the more load-bearing suppression claims were verified against the actual code rather than taken at face value —
field-input.tsx'sno-outline-noneclaim (the well'sfocus-within:shadow-[...]ring) andcreate-habit-modal.tsx'sexhaustive-deps/hasProAccessclaim (derived value, listed by name in both callback deps) — both hold up. - Info: the
habit-list.tsxpromptDataRefwrite moved from synchronous render into auseEffect(a real timing change, not just a style hoist). Every read site was traced (checkAndPromptParentLog, only invoked fromuse-bulk-actions.tsinside auseCallbackfired on bulk-mutation success): all reads happen from user-triggered, post-mount callbacks, never during the same render pass that writes the ref — behavior-preserving and concurrent-safe as claimed. The mobile mirror (apps/mobile/components/habit-list.tsx) already has the identicaluseEffectpattern with matching dependencies, confirmed by the parity-checker subagent. - Info (pre-existing, not introduced by this PR):
apps/web/components/ui/settings-group.tsxPRO_BADGE_STYLEdoesn't byte-match the kit's canonical Badge primitive spec in DESIGN.md — flagged by the design-reviewer subagent as a follow-up candidate, not a regression from this diff (untouched styling, purely mechanical hoist here).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PASS. The one genuine behavior change (habit-list.tsx ref-write timing) already has an identical useEffect pattern on the mobile mirror. The three extracted helpers (date-group-label.ts, empty-state-message.ts, app-bar-right-action.ts) all have pre-existing, logically identical mobile equivalents. The bulk-action-bar-v2.tsx SSR guard is web-only (no SSR/DOM portals on React Native) — no mirror needed. |
| design-reviewer | PASS. Every hoisted style constant uses semantic tokens (var(--fg-*), var(--primary), var(--status-*-text), var(--hairline*)) — no raw hex/slate/hardcoded violet. No transition-all, no h-screen introduced. Radii/borders/status-text tokens match DESIGN.md's kit tables. A11y (aria-labels, 44px+ hit targets) intact. Two no-tiny-text suppressions in reschedule-sheet.tsx are legitimate caption/disclosure text, not shrunk body copy. One pre-existing (not-introduced-by-this-PR) PRO_BADGE_STYLE drift noted as a follow-up candidate, not a blocker. |
| i18n-syncer | N/A — no new/changed user-facing strings; all t() calls reuse existing keys. |
| contract-aligner | N/A — no packages/shared/src/types/*, endpoints.ts, or orbit-api files touched. |
| security-reviewer | N/A — no orbit-api code touched. |
Validation
| Check | Result |
|---|---|
| Lint / Type check / Tests | Not re-run locally (CI runs Build / Unit Tests / SonarCloud as separate required checks per this workflow's instructions); PR self-reports 0 ESLint errors, tsc --noEmit pass, Vitest 2268/2268 pass, React Doctor 0 new / 0 remaining for this file-set. |
Deferred — N/A dimensions
- Backend hard rules / FEATURES.md parity: N/A, no
orbit-apifiles, no user-facing feature surface change. - Cross-repo dimensions (contract-aligner's orbit-api side, backward-compat guard's orbit-api side): not verifiable in this CI job —
orbit-apiis not checked out here. No indication this diff needs it (no shared-type/endpoint changes).
What's good
- The
bulk-action-bar-v2.tsxfix is a real, previously-latent SSR/hydration bug (unguarded DOM query insidecreatePortal) fixed correctly and minimally. - Every suppression comment cites a specific, checkable reason with an issue link rather than a blanket "false positive."
- Clean extractions with zero dangling references, matching pre-existing mobile equivalents — genuine cross-platform parity, not just claimed.
Recommendation
Safe to merge. No Critical/High findings, no contract or parity concerns. Both dispatched subagents (design-reviewer, parity-checker) independently confirmed PASS.


Burn down all React Doctor findings under
apps/web/components/habits/**andapps/web/components/ui/**(bucket B1) — 93 findings to zero for this file-set. Branched off currentmainso it carries PR #500 (render-existing-reminders).Fixed properly (34)
Setlookups instead ofarray.includes()inside.map().type="button"on non-submit buttons.app-bar-right-action.ts,date-group-label.ts,empty-state-message.ts) and un-exported the internal-onlyresolveReminderLabel.useMemowhose reference is never relied on.[]/{}prop defaults to module constants.useEffect(reads only happen in post-commit callbacks; behavior-preserving, concurrent-safe).useIsClientguard so thecreatePortaltarget is only read on the client (this was the one genuinely unguarded portal — a real latent SSR-crash fix).Justified
react-doctor-disable-next-linesuppressions (59, each WHY + #243)motion.*acrosscomponents/**); a partial per-file swap yields no bundle benefit and risks unprovidedm. Consistent with chore(web): drive React Doctor to zero across web app routes (#243) #501.createPortalis behind an existinguseIsClient/mountedguard the rule cannot trace; unreachable during SSR.profile/query members are recomputed every render and already listed; the rule unwraps them to the source member (false positive).<button>cannot legally contain (div+role=button with full keyboard handling).focus-withinring present; click-events-have-key-events (1) — capture-phase wrapper over the real button; no-autofocus (1) — search field in a user-invoked picker.Verification (all green, foreground)
apps/webtypecheck (tsc --noEmit) — passapps/webESLint — 0 errors (3 pre-existing warnings, outside this file-set)apps/webVitest — 2268/2268 pass--scope changed --base origin/main --blocking error— 0 newapps/webReact Doctor scan — this file-set at 0Refs #243 (React Doctor burn-down: web habits/ui)