chore(ui): flatten nested ternaries (Sonar typescript:S3358) - #483
Conversation
Refactors all 42 open S3358 nested-ternary occurrences in scope (41 in apps/web, 1 in packages/shared) into equivalent readable forms: early-return render helpers, if/else assignments, and extracted suffix/label variables. Logic and rendered output are identical. 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
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #483
Scope: PR #483 - flatten nested ternaries (Sonar typescript:S3358) - 26 files in apps/web + 1 file in packages/shared
Recommendation: APPROVE
Summary
This PR eliminates every nested-ternary (Sonar typescript:S3358) instance across 26 apps/web files and one packages/shared utility (parseReviewMomentKey), rewriting each into an equivalent let/if-else assignment or an extracted local render-helper with early returns. Every hunk was checked by hand against the pre-image; each rewrite preserves the exact branch order, condition polarity, and rendered/returned value of the original ternary chain - a behavior-preserving mechanical refactor, not a redesign. CI corroborates: Build, Type Check, Lint, Design Token Guard, and Contract Drift all report SUCCESS.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
- [Info] apps/web/app/(app)/retrospective/_components/retrospective-dashboard.tsx:158-185 - the returned JSX inside the new habits.map callback isn't re-indented to its new nesting level. Purely cosmetic, lint/Prettier pass on it; noted for awareness, not a reportable finding.
- [Info] Cross-platform parity: this PR touches only apps/web/** and packages/shared/** - no apps/mobile/** file included. The PR body states apps/mobile's S3358 instances are handled by a separate sibling slice. Since this refactor changes zero behavior/props/data-flow, there is no behavioral drift for a mobile mirror to close. Sibling mobile PR's existence was not independently verifiable in this environment - a tracking note, not a blocking parity violation for this PR.
Correctness verification
All 27 ternary-to-if/else or ternary-to-render-helper rewrites (26 web files + 1 shared file) were checked branch-by-branch against the pre-image and are logically identical - including layout.tsx's candidateKey chain, retrospective-dashboard.tsx's statusLabel chain (branch order flipped but exhaustive/equivalent), the loading/error/empty/content chains across accountability-section, friend-profile-view, pair-detail, social-feed, social-friends, social/page, upgrade/page, reschedule-sheet, today-rail, empty-state, goal-card, the command-menu.tsx outer/inner split, the value-ternary rewrites in login-sections, the three calendar views, multi-month-heatmap, edit-goal-modal/goal-detail-drawer, move-parent-overlay, habit-row-check-circle, habit-row-menu, route-transition-shell, pricing-section, and engagement-prompt-store.ts's parseReviewMomentKey. No any, no console.log, no new comments, no dropped className/token/style value, no changed prop shape, no new i18n key, and no packages/shared/src/types/* or orbit-api DTO touched.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A - reconstructed inline; behavior-preserving refactor, nothing for a mobile mirror to catch up to |
| i18n-syncer | N/A - no user-facing string/locale JSON change |
| contract-aligner | N/A - no shared types/endpoints or orbit-api DTO change |
| security-reviewer | N/A - orbit-api untouched |
| design-reviewer | PASS - every extracted branch preserves its exact pre-existing JSX/className/style/token values verbatim |
Validation
| Check | Result |
|---|---|
| Lint | PASS (CI) |
| Type check | PASS (CI) |
| Tests | in progress in CI at review time; PR body reports 2239 web + shared tests passing locally |
| Build (web) | PASS (CI) |
| Design Token Guard | PASS (CI) |
| Contract Drift | PASS (CI) |
| Build (api) | N/A - orbit-api not touched |
Deferred
i18n / contract-drift / security / backend-hard-rules / FEATURES.md parity dimensions: N/A, surface not touched. CI Unit Tests still in progress at review time. Mobile sibling PR for remaining apps/mobile S3358 instances: existence not independently verifiable here (gh pr list/issue view were blocked in this session) - Info only. All 27 changed files received an explicit correctness verdict above; none deferred.
What's good
Textbook mechanical refactor: every ternary chain replaced with a strictly equivalent, more readable construct, verified by hand across all sites. No NOSONAR/eslint-disable suppressions. Extracted render-helpers are descriptively named and small. Zero behavior/prop/token drift slipped in alongside the mechanical rewrite.
Recommendation
Approve and merge. No Critical/High findings survive review.
The S3358 render-helper extraction left the loading spinner and load-error EmptyState blocks duplicated verbatim across the social feed, friends, and buddies sections, tripping the SonarCloud new-code duplication gate (8.9%). Extract them into shared SocialSectionSpinner / SocialSectionLoadError components (DRY rule 10). Rendered output unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Note on SonarCloud conditionsThe first scan flagged two new-code conditions:
No |
There was a problem hiding this comment.
Code Review: PR #483
Recommendation: APPROVE
Summary
Pure mechanical refactor: 42 Sonar typescript:S3358 nested-ternary violations across 27 apps/web files + 1 packages/shared file, rewritten into let/if-else assignments or extracted render-helper functions (plus one new shared component, apps/web/app/(app)/social/_components/social-section-states.tsx, deduping three identical loading/error blocks). Spot-checked the most complex conversions by hand (command-menu.tsx, today-rail.tsx, retrospective-dashboard.tsx, habit-row-check-circle.tsx, packages/shared/src/stores/engagement-prompt-store.ts, route-transition-shell.tsx, pricing-section.tsx, move-parent-overlay.tsx, layout.tsx) — every rewrite preserves the original ternary chain's branch order and output exactly. apps/mobile is untouched by design (PR body + issue #243 defer it to a sibling slice).
Findings
Critical: None.
High: None.
Medium: None. A design-reviewer flag on apps/web/components/upgrade/pricing-section.tsx:117 (--status-bad used for body text vs. the -text variant) traced via git blame to commit bab3e455 — pre-existing, untouched by this diff (which only touches that file's eyebrow ternary around lines 38-51). Out of scope per rubric.
Low/Info: friend-profile-view.tsx still hand-rolls its own spinner identical to the new SocialSectionSpinner rather than reusing it — legitimate follow-up (that file wasn't one of the three dedup targets), not a defect. Not posted per signal gate.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — web-only, behavior-preserving refactor; mobile mirrors remain aligned; mobile's own S3358 pass is intentionally deferred to #243 |
| i18n-syncer | N/A — no string/key changes |
| contract-aligner | N/A — no shared-type/endpoint changes; orbit-api untouched |
| security-reviewer | N/A — orbit-api untouched |
| design-reviewer | PASS — new social-section-states.tsx verified byte-for-byte against its 3 call sites; no token/a11y/copy regressions in any reviewed file |
Validation
Skipped per this workflow's CI adaptations (Build / Unit Tests / SonarCloud run as separate required checks). PR body self-reports: turbo type-check pass, ESLint 0 errors, Vitest 2239 web tests + shared pass, React Doctor 0 issues, jscpd 0 new clones — not independently re-run in this pass.
Deferred
Dimensions 10-14 (i18n, contract drift, security, backend hard rules, FEATURES.md parity) are N/A — this diff touches none of their surfaces (no strings, no shared types/endpoints, no orbit-api, no feature/behavior change). The sibling orbit-api repo is not checked out in this job; no cross-repo dimension was needed for this diff anyway. All 28 changed files received a verdict — nothing silently skipped.
What's good
Zero NOSONAR/eslint-disable suppressions; the social-section-states.tsx extraction is a clean rule-6 application (3 real duplicated call sites, not invented); engagement-prompt-store.ts's rewrite is strictly better than the original (drops a redundant double startsWith check); comment policy is clean (only 2 new JSDoc blocks, both sanctioned).
Recommendation
Approve as-is — no correctness, contract, security, or parity risk found. Mobile-side S3358 cleanup should land as its own PR per #243.
|
1 participant
What
Fixes every open typescript:S3358 (nested ternary) instance in the scoped surfaces: 41 in
apps/web+ 1 inpackages/shared(42 total). Theapps/mobileinstances are handled by a sibling slice.Each nested ternary is rewritten into an equivalent, more readable form — behavior and rendered output identical:
let x; if/elseassignments (e.g.login-sectionstitle,calendar-*labels,pricing-sectioneyebrow,multi-month-heatmapalphaFor/cellSize,command-menuactivePageLabel).accountability-section,friend-profile-view,pair-detail,social-feed,social-friends,social/page,upgrade/page,today-rail,reschedule-sheet,goal-card,empty-state,command-menu).goal-detail-drawer/edit-goal-modalunitSuffix,move-parent-overlayhoverClass).No
// NOSONAR/eslint-disableused — every instance is a real refactor.Verification
turbo type-check(web + shared) — passeslint(web + shared) — 0 errorsvitest run— web 2239 tests pass, shared pass--blocking error) — 0 issuesRefs #243
🤖 Generated with Claude Code