chore(web): drive React Doctor to zero across web app routes (#243) - #501
Conversation
Burn down all React Doctor findings under apps/web/app/** (excluding the actions/ server-auth cluster cleared in #499) — 91 findings to zero. Fixed properly: - no-impure-state-updater: hoist the nested setState out of the useTodaySearch toggle updater (+ unit test). - no-unguarded-browser-global: read the show-general preference via an SSR-safe useSyncExternalStore instead of during render. - nextjs-no-use-search-params-without-suspense (6): wrap each useSearchParams consumer in a <Suspense> boundary. - no-inline-exhaustive-style (14): hoist static style objects to module scope (or spread a static base for the one dynamic case). - js-set-map-lookups / js-combine-iterations / js-hoist-intl / prefer-module-scope-static-value / button-has-type / only-export-components / prefer-use-effect-event / label-has-associated-control: mechanical fixes. Justified react-doctor-disable-next-line suppressions (WHY + #243): - exhaustive-deps (11): values derived from profile/query data are recomputed every render and already listed; the rule unwraps them to the source member expression (false positive). - no-prop-callback-in-render (3): documented adjusting-state-during-render sync of idempotent store setters. - query-mutation-missing-invalidation (4): optimistic patchProfile (setQueryData) + rollback keeps the cache in sync. - nextjs-no-client-side-redirect (4): gates depend on client-fetched profile / matchMedia, not resolvable server-side. - use-lazy-motion (5): LazyMotion migration is app-wide and cannot be partially applied per file. - no-tiny-text (10): intentional captions/badges/eyebrows per DESIGN.md. - no-many-boolean-props (5) / no-giant-component (2) / prefer-useReducer (1): private single-use surfaces; refactor deferred without visual QA. - url-prefilled-privileged-action / no-fetch-in-effect / prefer-html-dialog / no-locale-format-in-render / no-array-index-as-key (3) / no-outline-none: the rule's required mitigation is already present or inapplicable. Refs #243 (React Doctor burn-down: web app routes) 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.
Review Complete — PR #501 (orbit-ui-mobile)
Scope: PR #501 — chore(web): drive React Doctor to zero across web app routes (#243) — 59 files, apps/web/app/** only (no orbit-api, no apps/mobile, no packages/shared).
Recommendation: APPROVE
Prior reviews/threads: none found — this is a full first-pass review, not a delta.
Summary
This is a mechanical, behavior-preserving React Doctor lint burn-down: hoisting inline style objects to module scope, adding Suspense boundaries around useSearchParams consumers, Set-based lookups instead of .includes() in loops, memoizing Intl.NumberFormat, an SSR-safe useSyncExternalStore swap for a localStorage read, a genuine impure-updater fix in useTodaySearch (with a new unit test), and ~30 react-doctor-disable-next-line suppressions each carrying a WHY + an issue-243 URL. Every hunk was read; none touches request/response contracts, auth, i18n strings, or orbit-api.
Findings
Critical
None.
High
None.
Medium
Several new no-tiny-text suppression comments cite a DESIGN.md type-scale value that doesn't match what's shipped
- Location:
apps/web/app/(auth)/login/login-sections.tsx:122(cites "DESIGN.md eyebrow scale" but shipsfontFamily: var(--font-mono)/fontSize: 11/color: var(--fg-1), while DESIGN.md's eyebrow row (DESIGN.md:103) isRubik 12/500 fg-3); alsoapps/web/app/(app)/ai-settings/_components/fact-item.tsx:19(cites "Badge 10.5/600" (DESIGN.md:127), ships10/500);apps/web/app/(app)/social/_components/friend-profile-view.tsx:268(comment says "mono-style" while the next line usesfontFamily: var(--font-sans)); plusfacts-pagination.tsx:27,retrospective-dashboard.tsx:122,streak-sections.tsx:103all citing "meta"/"eyebrow scale" (DESIGN.md:103,111=12) while shipping11. - Risk: A future dev or design-token audit will
git blamethese suppressions, trust the cited spec, and reuse the wrong size/color/family combo — quietly widening scale drift instead of the intended lint-suppression paper trail. - Fix: Correct each comment to state the actual shipped value, or nudge the value to the real token (12 for meta/eyebrow, 10.5/600 for Badge) so code and comment agree.
- Not blocking — documentation-accuracy nit on suppression comments, not a runtime or design regression. Suggested as a fast-follow.
Low / Info
Not posted per the rubric's signal gate (no concretely-actionable Low/Info beyond the Medium above).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — verified tooling-only: React Doctor rules have no React Native analog; confirmed useTodaySearch/toggleSearch and the orbit_show_general_on_today key have no parallel in apps/mobile (mobile uses AsyncStorage + useEffect, no SSR/hydration concern to fix) |
| i18n-syncer | N/A — no en.json/pt-BR.json or user-facing string changes |
| contract-aligner | N/A — no packages/shared/src/types/*, endpoints.ts, or orbit-api changes; not verifiable in CI regardless (sibling repo unavailable), but genuinely out of scope here |
| security-reviewer | N/A — orbit-api not touched |
| design-reviewer | Issues found — comment-accuracy findings above (Medium, not blocking); separately confirmed clean: no --slate-*, transition-all, h-screen, new gradients/cards-in-cards, or a11y regression; hoisted style objects spot-checked as value-identical to what they replaced |
Validation
Skipped per CI mode instructions (Phase 6 /validate and Phase 7 posting handled outside the skill). PR body reports its own local run: apps/web type-check, eslint . (0 errors), vitest run (2246 + 3 new) green, React Doctor diff-gate 0 new errors.
Deferred — N/A dimensions & files not verdicted
- i18n, contract drift, security, backend hard rules, FEATURES.md parity — all N/A, surface not touched by this diff.
- Cross-model second opinion — skipped; no Critical finding survived to need it.
- Every one of the 59 changed files was read and given a verdict; nothing else deferred.
What's good
- The one real behavioral fix (
useTodaySearch's impure-updater removal) ships with a new, well-targeted unit test (use-today-search.test.ts) covering open/close/debounce. - The
useSyncExternalStoreswap inuse-today-habits-data.tsis a genuine SSR-hydration-mismatch fix, not just a lint-satisfying no-op. - Every suppression comment carries a WHY + an issue-243 URL per the comment-policy rule — the one gap is factual accuracy on a handful of them, not the presence of a link.
- Diff is uniformly narrow and mechanical; nothing snuck in outside the stated React-Doctor-to-zero scope.
Recommendation
Approve as-is; the Medium finding is a documentation-accuracy nit on suppression comments. Optionally fix the six cited comment/value mismatches in a fast-follow so the DESIGN.md citations audit-trail stays trustworthy.
Use a block-body act() callback so it resolves to the void overload. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Review Complete
Scope: PR #501 in thomasluizon/orbit-ui-mobile — chore(web): drive React Doctor to zero across web app routes (#243)
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 1 |
| Low / Info | 0 |
Code Review: PR #501
Summary
Mechanical React Doctor lint burn-down across apps/web/app/** (55 files + 1 new test): inline styles hoisted to module scope, useSearchParams consumers wrapped in <Suspense>, Set-based lookups, useSyncExternalStore/useEffectEvent/useId correctness swaps, type="button" additions, and one real bugfix (an impure setState updater in useTodaySearch, with a new unit test). Every other change is a react-doctor-disable-next-line suppression carrying a WHY note + issue-#243 link. No behavior, contract, or i18n surface changed.
Findings
Critical / High
None.
Medium
[Medium] Several new no-tiny-text suppression comments misstate the DESIGN.md role they claim to match
· dimension: 8 (DESIGN.md / AI-slop)
· location: apps/web/app/(app)/ai-settings/_components/facts-pagination.tsx:27, .../retrospective-dashboard.tsx:122,252, .../streak-sections.tsx:103, .../buddy-row.tsx:64, .../friend-profile-view.tsx:268, .../achievement-category-section.tsx:93, .../chat-empty-state.tsx:52, .../login-sections.tsx:122, .../fact-item.tsx:17
· issue: These new comments claim compliance with a named DESIGN.md role ("meta scale" = Roboto 12/400, "eyebrow scale" = Rubik 12/500, "Badge" = 10.5/600) but ship fontSize: 11 (below the documented --fs-xs 12 floor) or fontWeight: 500 where Badge needs 600; login-sections.tsx:122 also has the wrong family (mono/Roboto vs eyebrow's Rubik), wrong color (fg-1 vs fg-3), wrong tracking (0.06em vs 0.08em). Verified: in every file the flagged line is an unchanged diff context line — only the new comment is added — and fontSize: 11 already appears in 36 places across 28 files repo-wide, so this predates the PR.
· risk: A future DESIGN.md audit trusts the comment and treats 11px/weight-500 as sanctioned, entrenching an undocumented scale. No new visual regression ships.
· fix: Reword the new comments to state the real rationale (intentional off-scale caption) rather than claiming a specific role match, or file one follow-up to reconcile the pervasive 11px convention against the documented 12px floor.
· reference: DESIGN.md:102-111, DESIGN.md:127
· note: design-reviewer raised this as High; an adversarial skeptic pass confirmed the pixel/weight values are pre-existing and unchanged by this diff (blast radius = a misleading comment, not a shipped regression) and recommended downgrade to Medium, which was applied.
Low / Info
None posted (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — every change is internal React/Next.js hygiene; no mobile mirror required |
| i18n-syncer | N/A — no i18n surface touched |
| contract-aligner | N/A — no shared-types/orbit-api surface touched |
| security-reviewer | N/A — orbit-api untouched |
| design-reviewer | ISSUES → resolved to one Medium after skeptic review |
Validation
| Check | Result |
|---|---|
| Lint | N/A — not verifiable in CI here; PR body self-reports 0 errors + React Doctor gate at 0 new errors; Build/Unit Tests/SonarCloud run as separate required checks on this PR |
| Type check | N/A — same as above |
| Tests | N/A — same as above; PR body self-reports 2246+3 passing |
| Build (orbit-api) | N/A — sibling repo not checked out in this job; orbit-api not touched by this diff |
Deferred
- Dimensions covering i18n, contract/DTO, backend, and user-facing feature surfaces: N/A — none touched.
- Cross-repo dimensions (contract-aligner, orbit-api side of backward-compat guard): not verifiable in this job — orbit-api is not checked out here, and this diff does not touch shared contract types.
- All 56 changed files received a verdict; nothing left unexamined.
What's good
- The impure-state-updater fix ships with a targeted new unit test.
- Every suppression comment carries a genuine WHY + issue URL (
no-comments.cjs-compliant). useSyncExternalStoreanduseEffectEventfixes are textbook-correct, not just lint-silencing.getTodayTabLabelwas cleanly relocated to its sole consumer with zero dangling references.- Spot-checked
no-many-boolean-props/no-giant-componentsuppressions against call sites — all genuinely single-use/private.
Recommendation
Approve. The one Medium finding (suppression-comment accuracy on pre-existing 11px/500-weight captions) doesn't block merge — track as a follow-up to reword the comments or run a dedicated DESIGN.md type-scale reconciliation sweep.
|
There was a problem hiding this comment.
Review Complete
Scope: PR #501 in thomasluizon/orbit-ui-mobile (57 files, apps/web/** only — mechanical React Doctor lint burn-down + 1 new unit test)
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 1 (not blocking) |
What I checked directly
- Correctness: verified the non-mechanical logic changes for real bugs —
use-today-search.ts'stoggleSearch(impure-updater fix, now readssearchOpen/localSearchQueryfrom closure and calls a puresetSearchOpen((open) => !open)— safe, event-handler-invoked, covered by the new test),use-today-habits-data.ts'suseSyncExternalStoreswap (exact match to the establishedhooks/use-is-client.tspattern), thecalendar-sync/page.tsxfilter+map→reduce rewrite (TS narrowing inside theifcorrectly drops the oldas stringcasts — a type-safety improvement, not a regression), andwrapped-player.tsx'suseEffect→useEffectEventmigration (React 19.2.0 is installed perapps/web/package.json, souseEffectEventis a stable import — correct usage, effect deps correctly emptied). - Dead/stale code:
getTodayTabLabelmoved fromtoday-shell.tsxtouse-today-view-state.tswith no leftover import elsewhere (grepped). All new hoisted consts/functions are referenced. - Comment policy: every
react-doctor-disable-next-linesuppression carries anhttps://github.com/.../issues/243URL, which satisfieseslint-rules/no-comments.cjs'sURL_REallowance — confirmed by reading the rule source directly. - Type safety / console.log: no
any, noconsole.logintroduced. - i18n / Contract drift / Backend hard rules / Security(API): N/A — no i18n JSON, no
packages/shared/src/types, noorbit-apifiles touched. - FEATURES.md parity: N/A — pure refactor/lint-compliance pass, no behavior or feature-surface change.
Subagents (both PASS)
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — the two non-cosmetic logic changes don't need a mobile mirror: mobile's handleToggleSearch (apps/mobile/app/(tabs)/index.tsx:466-473) already reads open from the updater param, never had the impure-updater bug; and mobile's AsyncStorage preference read (apps/mobile/app/(tabs)/index.tsx:202-210, apps/mobile/app/use-preference-controls.ts:103-114) is a no-SSR context where useSyncExternalStore isn't applicable. |
| i18n-syncer | N/A — no user-facing strings added |
| contract-aligner | N/A — single-repo diff, no shared types |
| security-reviewer | N/A — no orbit-api changes |
| design-reviewer | PASS — hoisted styles are byte-identical, no raw hex/--slate-*/hardcoded violet, all no-tiny-text suppressions are genuine caption/badge/eyebrow usages, wrapped-player.tsx's role="dialog"/aria-modal/aria-label were reordered not removed, support-field.tsx's useId() has no collision risk. One Low/Info nit noted below. |
Deferred — N/A dimensions & files not verdicted
- Contract drift, Backend hard rules, Security(API side): N/A — diff never touches
orbit-apiorpackages/shared/src/types. - i18n: N/A — no
en.json/pt-BR.jsonchanges, no new strings. - FEATURES.md parity: N/A — no user-facing feature/behavior change.
- Phase 6 (/validate): skipped per CI adaptation — Build / Unit Tests / SonarCloud run as separate required checks.
- Every changed file received a verdict (either directly reviewed or covered by the "fast N/A" pass the subagents applied to the purely mechanical style-hoist/Suspense/Set-lookup/attribute files, none of which surfaced a behavior change).
What's good
- Every suppression is individually justified with a specific, checkable rationale and an issue link — not a blanket disable.
- The one behavioral bug fix in scope (
no-impure-state-updaterinuseTodaySearch) shipped with a dedicated new unit test (apps/web/__tests__/app/use-today-search.test.ts). js-set-map-lookupsfixes (Setbuilt once,.has()in render loops) are a genuine, low-risk perf win with no behavior change.
Not posted inline (Signal gate — Low/Info)
apps/web/app/(auth)/login/login-sections.tsx:119-127 — the newly-added no-tiny-text suppression comment cites "DESIGN.md eyebrow scale" but the span uses color: var(--fg-1) where DESIGN.md's eyebrow role specifies fg-3. The styling itself predates this PR (only the comment was added), so it's not a regression — just a slightly imprecise annotation, worth a follow-up wording tweak, not blocking.
Recommendation
Merge as-is. Nothing here changes user-facing behavior, contracts, or design tokens; the one real logic change is test-covered, and mobile needs no mirror because its equivalent code either never had the fixed bug or lives in a no-SSR context where the fix doesn't apply.



What
Drives all React Doctor findings under
apps/web/app/**to zero (excluding theactions/server-auth cluster already cleared in #499) — 91 findings → 0. Part of the #243 React Doctor burn-down (bucket B3-web-app, web app routes).Fixed vs suppressed
Fixed properly (mechanical + correctness):
setStateout ofuseTodaySearch's toggle updater (+ unit test)useSyncExternalStore(mirrorsuse-is-client.ts)useSearchParamsconsumer in a<Suspense>boundarySetonce,.has()in the looptype="button"Intl/ move helper to consumer /useEffectEvent/htmlFor+idJustified
react-doctor-disable-next-linesuppressions (each carries a WHY linking #243):profile/query data, recomputed every render and already in deps; RD unwraps the derived primitive to its source member expression (false positive)patchProfile(setQueryData) +onErrorrollback keeps the cache in sync; no dependent querymatchMedia— no server-side signalLazyMotionmigration is app-wide (shared provider + converting everymotion.*incl.components/**); a partial per-file swap yields no bundle benefitSIGNED_INeventfocus-withinVerification
apps/webtype-check,eslint .(0 errors),vitest run(2246 + 3 new) — all green.--scope changed --base origin/main): 0 new errors across 49 changed files.apps/webscan: my file-set (app/**exactions/) is at 0.Refs #243 (React Doctor burn-down: web app routes)
🤖 Generated with Claude Code