refactor(ui): flatten nested ternaries in apps/mobile (Sonar typescript:S3358) - #484
Conversation
…pt:S3358) Extract nested conditional expressions into named consts, if/else assignments, and hoisted ReactNode variables so no ternary is nested inside another. Behavior- and render-preserving across all 44 files / 59 S3358 instances scoped to apps/mobile. apps/web + packages/shared S3358 instances are handled by a sibling slice, so parity is preserved at the batch level. Refs #243 Co-Authored-By: Claude Opus 4.8 <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 #484 — refactor(ui): flatten nested ternaries in apps/mobile (Sonar typescript:S3358)
Scope: PR #484 in thomasluizon/orbit-ui-mobile (44 files, 59 nested-ternary instances flattened)
Recommendation: APPROVE
Summary
This is a pure, mechanical, behavior-preserving refactor that eliminates every SonarCloud typescript:S3358 (nested ternary) instance scoped to apps/mobile. Each transform is one of four small, well-understood patterns (hoist inner ternary to a named const, convert to an if/else block before return, cond ? X : null → cond && X where the guard is boolean/nullable-object, or multi-branch JSX chains hoisted into a let x: ReactNode variable). I walked all 44 changed files diff-hunk by diff-hunk and traced every extracted variable/branch back to the original ternary to confirm identical runtime behavior. No logic, styling, token, or i18n-key changes were found.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
- Info: A few of the
cond && (...)conversions (apps/mobile/app/(tabs)/calendar.tsx:9-30,apps/mobile/app/accountability-pair.tsx:378,apps/mobile/components/goal-card.tsx:1134) rely on the guard being a strict boolean or a nullable object rather than a number/string, which is the classic React "renders a stray0" footgun for this pattern. Verified each guard's type (activeError/rangeErrorare query-error objects,isPending/isSelectModeare booleans,trackingDotis{...} | null) — none can be a falsy-but-truthy-rendering primitive, so this is not a live bug, just worth naming as the one thing this transform class needs to keep checking each time it's applied.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — see note below (checked manually, not delegated) |
| i18n-syncer | N/A — no i18n keys added, removed, or renamed; every t(...) call in the diff is a pre-existing key relocated verbatim into the new if/const structure |
| contract-aligner | N/A — no packages/shared/src/types/* or endpoints.ts touched |
| security-reviewer | N/A — no orbit-api code in this diff |
| design-reviewer | PASS — confirmed every color/style computation still resolves through the same semantic tokens as before (tokens.fg1/fg2/fg4, tintFromPrimary, tokens.primary, etc.); no new AI-slop tell, no raw --slate-*/hardcoded rgba, no em dash introduced |
Parity note: this PR only touches apps/mobile. The PR body states the apps/web + packages/shared S3358 instances were fixed in a sibling PR. I confirmed this directly: PR #483 ("chore(ui): flatten nested ternaries (Sonar typescript:S3358)"), already merged to main ahead of this PR, fixes the corresponding web-side files for the same components touched here (calendar-range-view, calendar-week-view, social/*, upgrade/page, goal-card, goal-detail-drawer, edit-goal-modal, empty-state, pricing-section, habit-row-check-circle, move-parent-overlay, retrospective-dashboard, login-sections). Parity is preserved at the batch level, as claimed — not a finding.
Validation
| Check | Result |
|---|---|
| Lint | N/A in CI review — Build/Unit Tests/SonarCloud run as separate required checks on this PR per repo CI config |
| Type check | N/A in CI review — same as above |
| Tests | N/A in CI review — PR body reports vitest run 1056/1056 passing (200 files) and tsc --noEmit clean; not independently re-run here |
| Build (api) | N/A — orbit-api not touched, not checked out in this job |
Deferred — N/A dimensions & files not verdicted
- FEATURES.md parity (#14): N/A — pure refactor, no user-facing behavior change, no screen/route/tool/gating change.
- Backend hard rules (#13): N/A —
orbit-apinot touched. - Contract drift (#11): N/A — no shared-type or DTO surface touched.
- i18n (#10): N/A — no string/key changes (see Subagents).
- Validation (Lint/Type/Test): not run in this session — per this repo's CI wrapper instructions, Build/Unit Tests/SonarCloud already run as separate required checks on this PR, so re-running here would be redundant.
- Cross-repo dimensions requiring
orbit-api: not verifiable in this job (sibling repo not checked out). - Every one of the 44 changed files got an explicit verdict (traced against its pre-refactor version) — nothing in the diff was skipped.
What's good
- Every single-variable extraction and
if/elsechain is traceable 1:1 back to the original ternary's branch order and condition — zero behavior drift found across 59 instances. - No
any, noconsole.log, no new comments (narration or otherwise) introduced anywhere in the diff — clean on rules 3, 4, and 5. - The PR body is unusually thorough: it names the four transform patterns used, discloses the sibling-PR split for parity, and reports its own verification commands (tsc, vitest, expo lint, react-doctor, jscpd) with actual output — exactly the kind of self-auditing a pure mechanical refactor should ship with.
- Correctly resisted the temptation to "clean up while here" — no drive-by logic changes riding along with the Sonar fix.
Recommendation
Approve as-is. This PR does exactly what it says, nothing more, and every transformation checks out against the original code.
CI status — ready to merge (SonarCloud coverage-only override)All required checks pass. The only red is SonarCloud → new_coverage 0.0% (< 80%), which needs an admin override at merge. SonarCloud PR quality-gate conditions:
The transient Lint / Type Check / Contract Drift / Dependency Audit reds were a GitHub Actions action-resolution outage ("Failed to resolve action download info") and all pass on re-run. |
|
There was a problem hiding this comment.
/pr-review — PR #484
Scope: refactor(ui): flatten nested ternaries in apps/mobile (Sonar typescript:S3358) — 44 files, 59 instances.
No diff since last review. This bot already posted an APPROVE on this exact head commit (04615549629166bc74b8f5873c811a93e8c1747f) at 2026-07-13T13:32:19Z. gh pr view 484 confirms headRefOid is byte-identical and the PR is now MERGED — zero new commits, zero new hunks since that review. Re-walking the full rubric would just reproduce the prior pass verbatim.
Recommendation: APPROVE (carried forward, unchanged)
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 1 (carried forward — cond && (...) guard-type pattern noted, assessed as not a live bug) |
Findings, still valid at the current head:
- Pure, mechanical, behavior-preserving refactor; all 44 files traced 1:1 against pre-refactor ternary logic.
- No
any, noconsole.log, no new comments introduced; no i18n or logic changes. - Parity: corresponding
apps/web/packages/sharedS3358 instances were handled in the already-merged sibling PR #483 — confirmed directly. - No
packages/sharedororbit-apisurface touched → contract-drift and backward-compat-guard dimensions are not applicable; cross-repo dimensions requiring theorbit-apicheckout are not verifiable in this CI job but are also not triggered by this diff. - Validation (lint/type-check/tests) intentionally not re-run — owned by CI's Build / Unit Tests / SonarCloud required checks per this workflow's scope.


What
Flattens every nested conditional expression flagged by SonarCloud rule typescript:S3358 (
Extract this nested ternary operation into an independent statement) that is scoped to apps/mobile — 44 files / 59 instances.apps/web+packages/sharedS3358 instances are handled by a sibling slice, so cross-platform parity is preserved at the batch level.How (behavior- and render-preserving)
Each nested ternary was removed with the smallest structural change that keeps rendered output identical:
const(or anif/elsehelper for 3+ levels), leaving only single-level ternaries.const.cond ? (inner) : nullin JSX/style arrays →cond && (inner)(falsy is ignored identically), used only where the discarded branch isnull/falsy and the guard is a boolean.let x: ReactNodecomputed withif/else ifbeforereturn, rendered as{x}.No
any, no narration comments, no i18n or logic changes.Verification
npx tsc --noEmit— cleannpx vitest run— 1056 passed (200 files)npx expo lint— 0 errors (1 pre-existing warning in untouchedcalendar-sync.tsx)react-doctor --blocking error --scope changed— 0 blocking errorsRefs #243