chore(mobile): drive React Doctor findings to zero in app/hooks/lib (#243) - #502
Conversation
…243) Burn down all 101 React Doctor findings in the mobile app/hooks/lib/modules/test-mocks file-set (bucket B6). Fixed properly where safe; used justified inline `react-doctor-disable-next-line` suppressions (each linking #243) only for genuine false positives and deliberate architectural choices. Refs #243 (React Doctor burn-down: mobile app/hooks/lib) 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 #502 (thomasluizon/orbit-ui-mobile)
Scope: PR #502 — chore(mobile): drive React Doctor findings to zero in app/hooks/lib (#243)
Recommendation: NEEDS WORK
Summary
A 63-file, mechanical React Doctor burn-down across apps/mobile/{app,hooks,lib,modules,test-mocks} — exhaustive-deps fixes, loop/lookup micro-optimizations, memoized Intl formatters, a useState→useRef conversion, and 41 justified react-doctor-disable-next-line suppressions (each linking issue #243). The refactor is behavior-preserving and well-verified (lint/typecheck/1070 tests/coverage all green per the PR body), and every suppression comment checked is factually accurate (e.g. the two effect-needs-cleanup false-positive claims both genuinely clean up via a nested helper). One genuine defect survived review: the diff directly rewrites a single-habit-delete flow in index.tsx that is provably unreachable dead code, duplicating a second, working delete flow that already lives in habit-list.tsx.
Findings
Critical
None.
High
[HIGH] Dead, duplicate single-habit-delete flow touched by this diff
· dimension: Dead / stale code (#2)
· location: apps/mobile/app/(tabs)/index.tsx:102,134,165,428-439,722-727; apps/mobile/app/(tabs)/today-modals.tsx:137-145
· issue: The diff converts habitPendingDelete from useState to habitPendingDeleteRef (a rerender-state-only-in-handlers fix), but the entire flow it belongs to — showHabitDeleteConfirm state, habitPendingDeleteRef, confirmHabitDelete, and a dedicated deleteHabit = useDeleteHabit() instance — is unreachable. Nothing in the codebase ever calls setShowHabitDeleteConfirm(true) or assigns a non-null value to habitPendingDeleteRef.current (only writes are to null, at lines 436 and 725). HabitDetailDrawer (the only consumer of detailHabit) has zero delete-related props anywhere in its component tree (components/habits/habit-detail-drawer.tsx + its subdirectory) — confirmed by full-file read and grep. The real, reachable single-habit-delete UX lives entirely inside components/habit-list.tsx, which owns its own independent showDeleteConfirm/habitToDelete/deleteMutation (a second, separate useDeleteHabit() call) wired through HabitRowMenu. An independent adversarial skeptic pass confirmed this reading of the code.
· risk: A second useDeleteHabit() instance, a ref, a boolean, a callback chain, and a whole <ConfirmDialog> ship in every build with no way to ever trigger them — dead weight that misleads future maintainers into thinking there are two competing "confirm single delete" mechanisms when only one is wired up. Violates CLAUDE.md rule 2 ("delete unused code immediately"). Pre-dates this PR, but the diff's own hunks are the exact lines in question, so it was in hand to catch.
· fix: Remove habitPendingDeleteRef (index.tsx:165), showHabitDeleteConfirm/setShowHabitDeleteConfirm (index.tsx:134), confirmHabitDelete (index.tsx:428-439), the deleteHabit = useDeleteHabit() instantiation (index.tsx:102), the four props threaded to TodayModals (showHabitDeleteConfirm, onHabitDeleteOpenChange, onConfirmHabitDelete at index.tsx:722-727), and the matching <ConfirmDialog> block in today-modals.tsx:137-145 — unless a detail-view delete action was actually intended, in which case wire HabitDetailDrawer to it instead.
· reference: CLAUDE.md rule 2
Medium
None concretely actionable beyond the above.
Low / Info
- Info:
apps/mobile/app/(tabs)/index.tsxkeeps ahasProAccesslocal alias (line 116, still used at 117/508-ish) alongside several sites now inliningprofile?.hasProAccess ?? falsedirectly (lines 226, 233, 518) after the exhaustive-deps fix — a minor style split, not a behavior issue. Not posted per the signal gate (style/naming, not actionable). - Info: This PR is intentionally mobile-only and does not touch
apps/web, per its own stated bucket scoping (B6-mob-app) and consistent with the immediately-preceding, already-merged sibling PR of the same title/scope (commita5e09360). Every change reviewed is behavior-preserving (dependency-array corrections, loop-style rewrites, memoization, comment-only suppressions), so no new web/mobile behavioral drift was introduced — the cross-platform parity mandate is not violated by a PR that changes zero observable behavior. - Info:
apps/mobile/modules/orbit-widget/src/OrbitWidgetView.web.tsxaddssandbox="allow-scripts"to an iframe — a real, positive security hardening (avoids theallow-scripts+allow-same-originsandbox-escape combo), no adverse effect.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A / PASS — mobile-only lint burn-down, no behavior changes; all changed hooks with web mirrors preserve data flow, error handling, and business logic identically. |
| i18n-syncer | N/A — no user-facing strings added, changed, or removed |
| contract-aligner | N/A — no packages/shared/src/types/* or orbit-api changes |
| security-reviewer | N/A — no orbit-api changes |
| design-reviewer | PASS — only two files touch anything render-adjacent (iframe sandbox attribute; suppression comments above pre-existing, unchanged fontSize: 11/10 styles) — no new colors, gradients, or banned tokens introduced. |
Validation
| Check | Result |
|---|---|
| Lint | N/A — CI wrapper context; PR body states npm run lint passed |
| Type check | N/A — same; PR body states tsc passed |
| Tests | N/A — same; PR body states 1070/1070 passing, 85.84% line coverage |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- Contract drift + backward-compat guard (#11): N/A — no
packages/shared/src/types/*or orbit-api DTO hunks in this diff. - Security (#12, API side) and Backend hard rules (#13): N/A —
orbit-apinot touched. - FEATURES.md parity (#14): N/A — pure refactor, no feature added/changed/removed, no gating/platform/locale claim touched.
What's good
- Every
react-doctor-disable-next-linesuppression carries a WHY note with the issue-#243 URL, satisfying the comment-policy gate exactly. - The two
effect-needs-cleanupfalse-positive claims (use-notifications.ts,use-offline.ts) were independently verified — both effects do clean up correctly via a nested helper/unsubscribe. - The
useLoginCodeEntryno-impure-state-updaterfix (derivingcanResendfromresendCountdown === 0plus a dedicated cleanup effect) is a correct, cleaner replacement for the prior side-effecting state updater. no-prop-callback-in-rendersuppressions inuse-today-view-sync.tscite the exact "adjusting state during render" pattern this repo's ownapps/mobile/CLAUDE.mddocuments as the sanctioned alternative toset-state-in-effect— internally consistent.- Dead code was proactively removed elsewhere in the same diff (
buildDayLabelsinfriend-profile-sheet.tsx, hoisted-out local key arrays), showing the author is generally alert to exactly this class of issue — which makes the one surviving instance worth a quick follow-up.
Recommendation
Delete the unreachable habitPendingDeleteRef / showHabitDeleteConfirm / confirmHabitDelete / second useDeleteHabit() instance in index.tsx and the paired dead <ConfirmDialog> in today-modals.tsx (or wire HabitDetailDrawer to them if a detail-view delete action was actually intended) — this is a small, contained fix directly in code the diff already touched. Once that's resolved, this PR is otherwise clean and ready to merge.
Extract the identical post-mutation success block shared by confirmBulkLog and confirmBulkSkip into applyBulkMutationSuccesses, removing the one duplicated block SonarCloud's new-code duplication gate flagged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review: PR #502 (thomasluizon/orbit-ui-mobile)
Scope: PR #502 — "chore(mobile): drive React Doctor findings to zero in app/hooks/lib (#243)"
Recommendation: NEEDS WORK
Summary
This is a large (61-file) React-Doctor lint burn-down across apps/mobile/{app,hooks,lib,modules,test-mocks} — dependency-array fixes, filter().map() → for…of rewrites, useMemo/useWindowDimensions hoists, a useState→useRef conversion, and 41 justified react-doctor-disable-next-line suppressions, each with a WHY comment linking #243. The refactors reviewed are behavior-preserving and generally well-reasoned. One High finding survives adversarial verification: this diff's own state→ref conversion touches a piece of TodayScreen delete-confirmation code that is provably unreachable dead code, predating this PR. One Medium design finding on a tiny-text lint suppression.
Findings
Critical
None.
High
[High] habitPendingDeleteRef / showHabitDeleteConfirm / confirmHabitDelete in TodayScreen is unreachable dead code that this diff edits directly
· dimension: 2 — Dead/stale code (CLAUDE.md rule 2; severity ladder: "dead code that ships" = High)
· location: apps/mobile/app/(tabs)/index.tsx:165,428-439,722-726; wired dialog at apps/mobile/app/(tabs)/today-modals.tsx:138-145
· issue: This diff converts habitPendingDelete from useState to habitPendingDeleteRef (a useRef) to satisfy the rerender-state-only-in-handlers React-Doctor rule. But nothing in the entire apps/mobile tree ever calls setShowHabitDeleteConfirm(true) (grep across the whole tree: zero matches), and nothing ever assigns a non-null habit to habitPendingDeleteRef.current (only read at line 429, set to null at 436/725). HabitListHandle (the only imperative surface exposed to index.tsx) exposes no delete-trigger method either. Independently verified: the real, live single-habit-delete flow is fully self-contained inside apps/mobile/components/habit-list.tsx, which has its own separate showDeleteConfirm/habitToDelete state (set at habit-list.tsx:851-852) and its own confirm dialog in components/habit-list/confirm-dialogs.tsx — entirely independent of index.tsx's mechanism.
· risk: The <ConfirmDialog> for showHabitDeleteConfirm in today-modals.tsx:138 can never open, and confirmHabitDelete can never delete anything through this path — it is pure dead weight (state, a ref, a callback, a whole dialog wired end-to-end) that a future edit could easily mistake for a live code path and build on top of. This is pre-existing (confirmed present in the base commit before this PR too), but this diff directly rewrites these exact lines without noticing the mechanism is orphaned, doubling down on it instead of deleting it.
· fix: In this PR or a fast follow-up, delete habitPendingDeleteRef, showHabitDeleteConfirm/setShowHabitDeleteConfirm, confirmHabitDelete, the showHabitDeleteConfirm/onHabitDeleteOpenChange/onConfirmHabitDelete props threaded through to TodayModals, and the <ConfirmDialog> block at today-modals.tsx:138-145 — the single-habit delete UX already lives correctly in habit-list.tsx.
· reference: CLAUDE.md rule 2 ("Delete unused code immediately"); rubric dimension 2; severity ladder ("dead code that ships" = High)
· verdict: CONFIRMED (independent skeptic pass re-grepped the whole apps/mobile tree and the HabitListHandle surface and found no reachable trigger; confirmed habit-list.tsx owns the actual live delete-confirm flow).
Medium
[Medium] no-tiny-text suppression drops below DESIGN.md's documented type floor
· dimension: 8 — DESIGN.md / AI-slop
· location: apps/mobile/app/(tabs)/calendar/_components/calendar-time-grid.tsx:213 (fontSize 11), :227 (fontSize 10)
· issue: DESIGN.md's type scale floors at --fs-xs: 12 and even the smallest documented styles ("eyebrow", "meta") are 12px — these two new react-doctor-disable-next-line no-tiny-text suppressions go below that floor (11px / 10px) with a WHY comment framed as "deliberate density," but the actual constraint (fixed-height time-grid slot) belongs on the block-height token, not the type scale.
· risk: Low visual risk in isolation, but it normalizes suppressing a real type-scale floor via lint-disable rather than fixing the root cause (an undersized slot). Note the same file already has several pre-existing, unsuppressed fontSize 10/11 uses elsewhere (allDayLabel, hourLabel, colHeaderWeekday, AllDayChip, AllDayMoreChip), so the pattern is already widespread — this doesn't introduce a new visual regression, just extends an existing gap.
· fix: If the density is genuinely load-bearing, grow the fixed block/lane height token to accommodate 12px instead of suppressing the floor; otherwise accept 12px here.
· reference: DESIGN.md (type scale, --fs-xs: 12); rubric dimension 8
Low / Info
None posted (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — no violations; the web-side React Doctor burn-down for the equivalent routes/hooks already landed independently in commit f394c44 (#501); this PR's changes are structural-only (no behavior change) so no new mirror work is owed |
| i18n-syncer | N/A — no user-facing strings added or changed |
| contract-aligner | N/A — no packages/shared/src/types/* or endpoints.ts changes |
| security-reviewer | N/A — no orbit-api changes |
| design-reviewer | ISSUES — see Medium finding above; otherwise PASS (tokens, radii, no card-in-card, no raw hex/slate; scene-sentence reads as Orbit) |
Validation
| Check | Result |
|---|---|
| Lint | N/A — CI adaptation: this PR runs Build / Unit Tests / SonarCloud as separate required checks |
| Type check | N/A — same as above |
| Tests | N/A — same as above |
| Build (api) | N/A — orbit-api untouched |
Deferred — N/A dimensions & files not verdicted
- Dimension 11 (Contract drift + backward-compat guard): N/A — no
packages/shared/src/types/*or orbit-api DTO changes in this diff. - Dimension 12 (Security): reviewed opportunistically (the
sandbox="allow-scripts"iframe hardening inOrbitWidgetView.web.tsxis a net-positive fix, not a gap); no orbit-api surface to check. - Dimension 13 (Backend hard rules): N/A —
orbit-apinot touched, and the sibling repo is not checked out in this CI job. - Dimension 14 (FEATURES.md parity): N/A — pure internal refactor, no user-facing feature surface, gating, or platform-availability change.
- Every changed file was given a verdict (grouped by pattern in the Correctness/Dead-code/Type-safety passes above); no file was silently skipped. The 41
react-doctor-disable-next-linesuppressions were spot-checked for a genuine WHY-with-URL note (all present) and for defensibility; none beyond the tiny-text one above raised a concern.
What's good
- Every suppression carries a WHY comment linking #243, satisfying the comment-policy gate exactly.
- The
filter().map()→for…ofrewrites (habit-optimistic-helpers.ts, offline-mutations.ts, use-habits.ts, use-bulk-actions.ts, use-calendars.ts, query-client.ts, sentry.ts) are faithful single-pass translations — verified the loop bodies preserve the original filter+map semantics. - The
useState→useRefconversions and dependency-array fixes elsewhere (use-gamification.ts, use-wrapped.ts, use-referral.ts, use-time-format.ts, use-profile.ts, use-today-date.ts, _layout.tsx) correctly read from the livequery.data/ prop rather than a stale closed-over alias. calendar-sync.tsx's rewrittenhabits.push({ id: item.habitId, title: item.title })replaces an unsafeas stringcast with a real truthiness narrow — a genuine type-safety improvement (rule 3), not just a lint dodge.- The
expo-modules-corepin removal and theDimensions.get()→useWindowDimensions()swap are correct, low-risk fixes.
Recommendation
Fix or explicitly follow up on the dead single-habit-delete-confirm mechanism in index.tsx/today-modals.tsx before merge — either strip it out in this PR (small, mechanical deletion) or open a tracked follow-up issue and note it in the PR description, since this diff is the one touching those exact lines. The tiny-text Medium is a judgment call, not a blocker, but worth a one-line note in the follow-up.
Addresses the PR #502 review [HIGH]. The habitPendingDelete / showHabitDeleteConfirm / confirmHabitDelete flow (plus a second useDeleteHabit() instance and the paired <ConfirmDialog> in today-modals) is unreachable: nothing sets showHabitDeleteConfirm to true or assigns a non-null habit, and HabitDetailDrawer has no delete-related props. The real, reachable single-habit-delete UX lives in components/habit-list.tsx. Removes the dead flow (index.tsx + the paired dialog and three props in today-modals.tsx) instead of the earlier state->ref conversion. CLAUDE.md rule 2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
PR #502 Review — chore(mobile): drive React Doctor findings to zero in app/hooks/lib (#243)
Recommendation: APPROVE
Summary
Mechanical, low-risk chore spanning 61 files under apps/mobile/{app,hooks,lib,modules,test-mocks} + package.json. It:
- Fixes 60 React Doctor findings properly: exhaustive-deps corrections using real reactive member expressions instead of stale local aliases,
.filter().map()→for…ofcollapses,Array.includes()→Set.has()in loops,Intl.*construction hoisted intouseMemo, static arrays hoisted to module scope, an impure-state-updater fix inuse-login-code-entry.ts(derivedcanResendfromresendCountdown === 0instead of a separately-tracked boolean),Dimensions.get()→useWindowDimensions(), and sequential-await parallelization (Promise.all) inoffline-mutations.ts. - Suppresses 41 more findings with WHY-linked
react-doctor-disable-next-linecomments, each citing issue #243 with a concrete justification (deliberateAnimatedAPI retention pinned to worklets/reanimated ABI, deliberate boolean-prop aggregators, deliberate optimistic-update mutations with no cache to invalidate, etc.). - Removes a dead single-habit-delete flow from the Today screen (
apps/mobile/app/(tabs)/index.tsx,today-modals.tsx). - Drops an unneeded direct
expo-modules-corepin frompackage.json. - Hardens
OrbitWidgetView.web.tsx's iframe withsandbox="allow-scripts".
Verification performed
I read the full diff and deep-read the highest-risk files (app/(tabs)/index.tsx, use-today-selection.ts, use-bulk-actions.ts, use-play-billing.ts, use-gamification.ts, use-wrapped.ts, providers.tsx, use-chat-composer.ts, use-login-code-entry.ts, use-today-view-sync.ts, calendar.tsx/calendar-day-detail.tsx), tracing every dependency-array rewrite for stale-closure or dead-variable regressions. None found.
Two candidate risks were specifically investigated and refuted:
-
calendar-day-detail.tsxkey change (key={\${item.habitId}-${index}`}→key={item.habitId}): confirmedcalendar-time-grid.tsxalready keys by barehabitId` elsewhere in the same file family — habitId is unique per day in this data model, not a duplicate-key regression. -
Removed single-habit-delete flow on the Today screen — flagged by an automated parity check comparing against web's
habit-list.tsx, which still has an active per-row delete action. I independently verified this directly against the diff and source:- Diffed
apps/mobile/app/(tabs)/index.tsxagainst its pre-PR version (commitf394c446):setHabitPendingDeletewas never called anywhere in the file except its ownuseStatedeclaration, before or after this PR — it was never passed as a prop to any child component, so it could never be set to a truthy value. This was orphaned dead state, not a reachable delete path. -
grep'd all ofapps/mobileforhabitPendingDelete/showHabitDeleteConfirm: zero remaining references — the removal is complete and clean. - Confirmed
apps/mobile/components/habit-list.tsx(untouched by this PR, the shared component the Today screen renders) already owns its own independent, fully-wired delete flow:useDeleteHabit()(line 255), anonDeletehandler (line 1108), andHabitListConfirmDialogs(line 1389) withonDeleteOpenChangewiring. This is the same component used on the Today screen, so per-habit delete capability is fully preserved and mirrors web'shabit-list.tsxflow. False positive — no parity regression.
- Diffed
Design review (design-reviewer subagent) returned PASS: no DESIGN.md/AI-slop violations. The no-tiny-text and rn-no-scrollview-mapped-list suppressions cover pre-existing sizes/lists with no visual delta, and the sandbox="allow-scripts" addition is security hardening, not a styling change.
Contract-aligner, i18n-syncer, security-reviewer, and FEATURES.md dimensions were all N/A — no packages/shared/src/types change, no orbit-api change (not checked out in this job — cross-repo dimension not verifiable), no new user-facing strings, no feature-surface change.
Findings
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 0 (not posted per signal gate) |
Nothing survived verification. Clean PR — plain approval.
…e-sheet The inlined day-strip useMemo token-matched the apps/web mirror (friend-profile-view.tsx), tripping SonarCloud's cross-file duplication gate on new code. Reverted this file to its main structure (the buildDayLabels helper) so the shared block stays unchanged (not "new code"), and cleared the js-hoist-intl finding with a justified suppression instead: the proper DRY fix is a packages/shared util, which is out of this mobile-only bucket. Net diff vs main is a single comment line; RD file-set stays at 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review: PR #502 (thomasluizon/orbit-ui-mobile) — diff since last review
Scope: apps/mobile/app/social/_components/friend-profile-sheet.tsx (commit 71052aad)
Recommendation: APPROVE
Review scope
- Last Claude review on this PR: APPROVE, submitted
2026-07-13T23:19:40Z, against commit9ae51293(parent of the approval). - One commit landed after that approval:
71052aad657d2aa8f7594f61cb94fd2ef0518d97— "chore(mobile): clear SonarCloud new-code duplication on friend-profile-sheet", committed2026-07-13T23:26:33Z. - That commit touches exactly one file:
apps/mobile/app/social/_components/friend-profile-sheet.tsx(13 insertions / 10 deletions in the commit diff). - This is the entire unreviewed surface. Everything else on the PR was already reviewed and approved.
Summary
The commit reverts a useMemo-wrapped ActivityStrip day-label builder back to the plain module-scope buildDayLabels(count, locale) function, and replaces the js-hoist-intl React-Doctor finding with a suppression comment. Diffing the current file against origin/main directly confirmed the commit message's claim exactly: the net diff vs main is one added comment line — the function body, call site, and all behavior are byte-for-byte identical to what already ships on main. No functional regression is introduced; this is a clean revert-plus-suppression, not new code.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
- Info: The suppression comment's claim — "This 7-day-strip builder is mirrored verbatim in apps/web friend-profile-view.tsx" — was independently verified:
apps/web/app/(app)/social/_components/friend-profile-view.tsx:244-253contains the identicalIntl.DateTimeFormat({ weekday: 'narrow' })+ 7-cell date-array construction +{ key, label }shape. The comment is factually accurate. Theparity-checkersubagent flagged this as "PARTIAL" only because the web side still wraps the block inuseMemowhile mobile now doesn't — a cosmetic pattern difference, not a behavioral one (logic is 100% identical). Not actionable; no fix needed. - Info: The comment passes the
local/no-commentslint rule via theURL_REallowance (contains an issue URL), same as the other justified suppressions already reviewed and approved in this PR. - Info: The PR body's suppression tally is now stale by one (this commit moved one
js-hoist-intlfix from "fixed" to "suppressed"). Non-code, cosmetic, not worth a finding under the signal gate.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PARTIAL (nit only) — web mirror has identical logic/output; only the useMemo-vs-plain-function wrapping pattern differs, confirmed non-behavioral |
| i18n-syncer | N/A — no user-facing strings changed |
| contract-aligner | N/A — no packages/shared/src/types/* or orbit-api changes |
| security-reviewer | N/A — no orbit-api changes |
| design-reviewer | PASS — zero JSX/styling/token/rendering delta; pure code-structure + comment change |
Validation
Skipped per CI setup — this PR runs Build / Unit Tests / SonarCloud as separate required checks.
Deferred — N/A dimensions
- Contract drift + backward-compat: N/A — no shared-types or DTO hunks in this diff.
- Security / backend hard rules: N/A —
orbit-apiuntouched and not checked out (not verifiable in this environment). - FEATURES.md parity: N/A — no user-facing feature surface changed.
What's good
- The commit message's technical claim ("net diff vs main is a single comment line") is independently verified true via a direct
git diffagainstorigin/main— high-trust, well-documented chore commit. - The suppression comment satisfies the comment-policy gate (WHY + issue URL) and its factual claim about the web mirror was verified accurate, not just asserted.
Recommendation
Nothing to fix. The diff since the last review is a clean, verified, zero-behavior-change revert with a properly justified lint suppression.
Findings summary
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 3 (not blocking, informational only) |
|
There was a problem hiding this comment.
Code Review: PR #502 (thomasluizon/orbit-ui-mobile)
Scope: PR #502 — chore(mobile): drive React Doctor findings to zero in app/hooks/lib (#243)
Recommendation: NEEDS WORK
Summary
This is the review of the PR's current head (merge commit af8f94a1, "Merge branch 'main' into chore/rd-mobile-app-hooks-lib"). Two prior Claude reviews on this PR already reached APPROVE — the first against commit 9ae51293 (after the dead single-habit-delete flow was removed in response to an earlier [HIGH] finding), the second against 71052aad (the SonarCloud duplication follow-up on friend-profile-sheet.tsx). I re-verified the full 63-file diff independently (not just the delta since the last review, since the merge itself pulls in unrelated apps/web commits from main that needed ruling out) and traced every useMemo/useCallback dependency-array rewrite, every filter().map() → for…of collapse, and all 41 react-doctor-disable-next-line suppressions for a stale-closure or dead-code regression. I found none — the refactor is behavior-preserving, and the two prior reviews' conclusions hold.
The one thing that changes the recommendation is external to the code: the SonarCloud Code Analysis check is currently FAILING on this exact head commit (verified live via gh pr view --json statusCheckRollup, not the code diff itself), and it wasn't caught by the prior reviews (which pre-date this check run). That's the only blocker below.
Findings
Critical
None.
High
[HIGH] SonarCloud Code Analysis check is currently failing on the PR's head commit
· dimension: 12 — adjacent to Security/CI-gate hygiene (this repo's claude-review.yml explicitly treats SonarCloud as one of the PR's required separate checks)
· location: GitHub check run SonarCloud Code Analysis on commit af8f94a1 — https://sonarcloud.io/dashboard?id=thomasluizon_orbit-ui-mobile&pullRequest=502
· issue: gh pr view 502 --json statusCheckRollup reports this check COMPLETED / FAILURE, timestamped 2026-07-13T23:39:52Z — after the final commit (23:37:34Z), so it is current, not stale. I do not have SonarCloud API/web access in this sandbox to pull the exact failing metric, so the root cause isn't independently confirmed. The strongest concrete lead: this diff repeats one verbatim WHY-comment string — "Deliberate React Native Animated API; migrating to reanimated risks the pinned worklets 0.10.0 / reanimated 4.5.0 ABI (SDK 57)…" — identically across 13 files (confirmed by grep), which is exactly the class of new-code duplication a previous commit on this same PR (71052aad) already had to fix once for friend-profile-sheet.tsx.
· risk: A required check failing blocks merge as-is; if unaddressed, this either stalls the PR or gets merged past a red gate.
· fix: Open the SonarCloud dashboard link above to read the actual failing condition. If it is new-code duplication from the repeated comment text, shorten the boilerplate to a short comment + the shared issue link (drop the restated migration-risk prose, which is identical in every file) or factor the justification into the linked issue #243 instead of repeating it verbatim 13 times.
· reference: .github/workflows/claude-review.yml ("this PR already runs Build / Unit Tests / SonarCloud as separate required checks")
Medium
None concretely actionable beyond the above.
Low / Info
- Info:
apps/mobile/package.jsondrops the directexpo-modules-corepin (expo-no-redundant-dependency); I flagged and then refuted a suspectedpackage-lock.jsondrift here myself — the root lockfile'sapps/mobilemanifest mirror still listsexpo-modules-core: 57.0.3whilepackage.jsonno longer does. However, theBuild,Type Check,Unit Tests, andReact Doctorchecks all reportSUCCESSon this exact head commit (each runsnpm cifirst per.github/workflows/test.yml), which directly contradicts a hard lockfile-sync failure. Deferring to that live evidence over my static read — noting it here in case it resurfaces, not as an actionable finding. - Info: The PR body describes only the React-Doctor burn-down bucket; three commits in the branch's own history (
b3fa3a2fdedupe bulk-mutation success handling,9ae51293remove dead single-habit-delete flow,71052aadSonarCloud dedup) aren't mentioned in the body. All three are legitimate, already independently reviewed/approved fixes — no action needed, just noting for anyone reading the PR description cold.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — mobile-only internal lint-finding burn-down bucket (issue #243), no user-facing behavior change; the equivalent web-side bucket already shipped separately (58845ff2, #499). Spot-verified myself: calendar.tsx's weekStartsOn fix (mobile: useMemo) vs. the web mirror's existing suppression comment for the identical linter rule are different remediation styles for the same non-issue — runtime behavior is identical on both platforms. Backgrounded subagent run did not return before this report was finalized. |
| i18n-syncer | N/A — no i18n keys added, changed, or removed in this diff. |
| contract-aligner | N/A — no packages/shared/src/types/*, endpoints.ts, or orbit-api changes. |
| security-reviewer | N/A — no orbit-api changes. |
| design-reviewer | PASS (self-verified) — every UI-adjacent hunk (tiny-text suppression comments, a React key-only change, the sandbox="allow-scripts" iframe hardening, hoisted const arrays) changes no rendered output. Backgrounded subagent run did not return before this report was finalized. |
Validation
Per this repo's CI-adaptation rule, Build / Type Check / Unit Tests / Lint / React Doctor are this PR's own required checks (not re-run here); see the High finding above for the one check that is currently red.
| Check | Result |
|---|---|
| Lint | PASS (CI) |
| Type check | PASS (CI) |
| Tests | PASS (CI, 1070/1070 per PR body) |
| Build | PASS (CI) |
| SonarCloud Code Analysis | FAIL (CI, current) — see High finding |
Deferred — N/A dimensions & files not verdicted
- Dimension 13 (Backend hard rules): N/A, no
orbit-apifiles in this diff; sibling repo not checked out in this environment. - Dimension 14 (FEATURES.md parity): N/A, this is a behavior-preserving refactor/bugfix bucket with no feature add/remove/gating change.
parity-checkeranddesign-reviewersubagents were dispatched but did not return a result before this report was written; both dimensions were covered by my own direct verification instead (see Subagents table) rather than left unchecked.- SonarCloud's exact failing metric/rule was not independently confirmed (no SonarCloud API/web access in this sandbox) — only the check's FAILURE status and timestamp were confirmed live via the GitHub API.
What's good
- Every one of the 41
react-doctor-disable-next-linesuppressions carries a WHY comment linking issue #243, satisfyingno-comments.cjsand giving a concrete, checkable rationale rather than a bare disable. - The
useState→derived-constfix inuse-login-code-entry.ts(canResend = resendCountdown === 0) correctly moves the side effect (clearResendTimer) out of the state updater into its own effect — a real fix, not just a lint-satisfying shuffle. - Every exhaustive-deps rewrite I traced (
use-gamification.ts,use-wrapped.ts,use-profile.ts,use-referral.ts,use-today-date.ts,TodayScreen'shasProAccess/currentStreak/dayProgress) replaces a stale local alias with the livequery.data/profilemember expression it was always equal to — genuinely closes real (if usually harmless) staleness windows, not cosmetic churn. - The
calendar-sync.tsxfilter+map→for-loop rewrite incidentally removes two unsafeas stringcasts that the old chained-lambda version needed (TS can narrow inside the singleifblock); a real type-safety improvement, not just a mechanical transform. OrbitWidgetView.web.tsxiframe hardening (sandbox="allow-scripts") is a correct, minimal fix for the sandbox-escape combo (noallow-same-originpaired with it).
Recommendation
Hold merge only on the SonarCloud Code Analysis check — open the dashboard link, confirm the failing condition, and fix it (most likely the repeated 13x verbatim comment string, given the precedent in this same PR's 71052aad). The code substance itself has now survived three independent review passes (two prior APPROVEs plus this one) with no surviving correctness, type-safety, comment-policy, or dead-code findings.
… guard enforces (#543) PR #502 (React Doctor burn-down) dropped "expo-modules-core": "57.0.3" from apps/mobile/package.json as an unrelated drive-by. The release job installs the isolated mobile workspace with --package-lock=false, so a from-scratch resolve is free to pick any version satisfying expo 57.0.4's ~57.0.3 range; it took the newest, 57.0.5, and the pin guard failed the first release run since (29512207801). Restore the pin at 57.0.3, the version the root lockfile and the last green release (2026-07-09) both resolved. Also make the guard distinguish a deleted pin from a real drift. It read the expected version straight out of package.json, so a removed entry compared against undefined and reported "pinned to undefined" instead of naming the actual fault. A missing pin now fails as UNPINNED with a message that says the pin must be restored, not dropped from the guard. Verified by reproducing the release step locally: prepare-release-workspace plus the same lockfile-free install, then the guard extracted verbatim from the workflow YAML. All five modules resolve to the verified set and the step exits 0; deleting the pin again reproduces the UNPINNED failure. Claude-Session: https://claude.ai/code/session_01Sz19TStgQiJxZCZiE9tNsr Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>


Drives all 101 React Doctor findings to zero in the mobile file-set
apps/mobile/{app,hooks,lib,modules,test-mocks}/**+package.json(burn-down bucket B6-mob-app). Fixed properly where safe; used justified inlinereact-doctor-disable-next-linesuppressions (each linking #243) only for genuine false positives and deliberate architectural choices.Fixed properly (60)
[query.data],[profile?.hasProAccess],[i18n.language]) instead of a see-through local alias, or reads a fresh snapshot inside the effect.weekStartsOnincalendar.tsxwas promoted to auseMemoboundary. One redundant derived dep (selectedCountincalendar-sync.tsx) removed by readingselectedIds.sizedirectly..filter().map()double-passes into singlefor…ofloops.privacy.tsx/terms.tsx.Array.includes()in a loop →Set.has().Intl.*construction moved intouseMemo(achievements.tsx,friend-profile-sheet.tsx).calendar-day-detailkeys byhabitId;retrospective-dashboardkeys narrative spans by character offset.use-login-code-entry: removed side effects from thesetResendCountdownupdater by derivingcanResend = resendCountdown === 0and clearing the timer in an effect.use-horizontal-swipe:Dimensions.get()→useWindowDimensions().offline-mutations: independent awaits and scope invalidations parallelized withPromise.all.index.tsx:habitPendingDelete(read only in handlers, never rendered) converted to auseRef.sandbox="allow-scripts"(avoids theallow-scripts+allow-same-originescape combo).expo-modules-corepin (Expo pins the compatible native core transitively).Justifiably suppressed (41) — each with a WHY comment linking #243
AnimatedAPI across the Today/login/streak/theme motion system. Migrating risks the pinned worklets 0.10.0 / reanimated 4.5.0 ABI (SDK 57) and would require rewriting the sharedlib/motion.tsAnimated-config helpers + cross-componentAnimated.Valueprops. Not mechanically rewritten — animation stability prioritized over clearing the rule.patchProfile()inonMutatewrites the verbatim value (name / AI toggles), anduseReportUser/useSuggestTagsmutate no client-visible cached state.use-today-view-syncare the documented adjusting-state-during-render pattern (mirrors web) with idempotent guards; 1 is a Vitest host-component mock.unsubscribe()/stopPolling()); RD can't trace the indirection.use-login-code-entryauto-submits on completion from any input source (typing, paste, externalsetCodeDigits/SMS autofill), which an event-handler-only call would miss.use-play-billingrestore: sequential to avoid racing the native Play billing client; restore set is effectively ≤1 subscription.Verification (all green)
npm run lint(expo lint) ·npm run type-check(tsc) ·npx vitest run(1070/1070) · coverage above thresholds (85.84% lines).--scope changed --base origin/main --blocking error): 0 new errors.apps/mobilescan: file-set B6 at 0 (101 → 0).Refs #243 (React Doctor burn-down: mobile app/hooks/lib)
🤖 Generated with Claude Code