Skip to content

refactor(ui): use design-system cn for all conditional classNames#6642

Merged
otavio merged 1 commit into
masterfrom
refactor/ui/cn-util
Jul 13, 2026
Merged

refactor(ui): use design-system cn for all conditional classNames#6642
otavio merged 1 commit into
masterfrom
refactor/ui/cn-util

Conversation

@luizhf42

Copy link
Copy Markdown
Member

What

Consolidated all conditional className construction across console, website, and the design-system package to use the shared cn() utility (clsx + tailwind-merge), replacing ad-hoc patterns that lacked Tailwind class conflict resolution.

Why

The codebase had four competing patterns for conditional classes: template literal ternaries, [...].filter(Boolean).join(" "), bare .join(" "), and a local naive cn in utils/cn.ts that only concatenated strings without tailwind-merge. Conflicting Tailwind classes (e.g. text-sm overridden by text-xs) were resolved by CSS source order rather than deterministically. The design-system already exported a proper cn() backed by clsx + tailwind-merge — it just wasn't used outside the DS primitives.

Changes

  • apps/console/src/utils/cn.ts: deleted. Its sole consumer (DataTable.tsx) now imports from @shellhub/design-system/cn
  • packages/design-system/components/Reveal.tsx, ShimmerCard.tsx: migrated from template literal class concatenation to their own package's cn
  • 93 files across console + website: every className={\...`}with interpolation converted tocn(). Empty-string ternary branches (cond ? "x" : "") simplified to cond && "x"`. Complex multi-condition expressions extracted into named variables for readability
  • SupportedPlatforms.tsx intentionally skipped — uses dynamic class construction (bg-${p.color}/10) which is a separate issue

Testing

  • tsc --noEmit passes for console, website, and design-system
  • npm run build (console) passes clean
  • npm run lint passes with 0 errors on both apps (pre-existing warnings only)
  • No remaining filter(Boolean).join, @/utils/cn imports, or template literal className patterns (except SupportedPlatforms.tsx)

@luizhf42 luizhf42 requested a review from a team as a code owner July 13, 2026 18:09
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

Claude finished @luizhf42's task in 4m 19s —— View job


Code Review Complete

Reviewed 91 files across code quality, security, testing, language patterns, and architecture. This is a well-executed, near-mechanical migration to the shared cn() utility — the conditional-branch simplifications (cond ? "x" : ""cond && "x") and named-variable extractions are all semantics-preserving, and the intended tailwind-merge conflict resolution (last-wins) is the correct behavior the PR set out to establish.

1 inline issue posted (1 high).

Finding

  • ui/apps/console/src/pages/admin/settings/Authentication.tsx:13 — unused cn import. This file's diff only adds the import with no cn(...) usage, so it fails tsc --noEmit (noUnusedLocals: trueTS6133) and npm run lint (unused-imports/no-unused-imports: "error"), contradicting the PR's testing claims. Remove the import.

Note

Verified both new import specifiers resolve: @shellhub/design-system/cnprimitives/cn.ts (package exports map) and the internal ../primitives/cn used by Reveal.tsx/ShimmerCard.tsx. The DS cn extends tailwind-merge with the custom text-2xs/text-3xs font-size group, so size-conflicting migrations (e.g. text-sm + text-xs) now resolve deterministically to the last class — intended, not a regression.


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Comment thread ui/apps/console/src/pages/admin/settings/Authentication.tsx Outdated
Replace ad-hoc conditional class construction patterns with the
design-system's `cn()` utility (`clsx` + `tailwind-merge`), which
handles falsy values natively and resolves Tailwind class conflicts
deterministically.

Migrated patterns:
- template literal ternaries (`\`base ${cond ? "a" : "b"}\``)
- `[...].filter(Boolean).join(" ")`
- `[...].join(" ")`
- local naive `cn` in `utils/cn.ts` (deleted)

Complex multi-condition expressions are extracted into named
variables for readability; simple single-ternary cases stay inline.
@luizhf42 luizhf42 force-pushed the refactor/ui/cn-util branch from 9c8dce1 to 2fdb415 Compare July 13, 2026 18:16
@gustavosbarreto

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

Code Review

  • Gathered PR context
  • Reviewing with specialized agents
  • Posting feedback

@otavio

otavio commented Jul 13, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 8m 52s —— View job


Code Review Complete

Reviewed 90 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

This is a clean, near-mechanical migration to the shared cn() utility. Verification highlights:

  • No unused imports — every file that adds import { cn } has at least one real cn(...) call site (the earlier Authentication.tsx unused-import was already fixed).
  • No dropped branches — all cond ? "x" : ""cond && "x" conversions had an empty falsy branch; no cond ? "a" : "b" with a non-empty else was ever collapsed.
  • Import resolution verified@shellhub/design-system/cn maps to ./primitives/cn.ts via the package exports map, and the deleted @/utils/cn has zero remaining references anywhere in ui/ (DataTable.tsx was its sole consumer, now repointed).
  • No cross-repo impact — the enterprise cloud/ repo is Go-only, so no frontend breakage is possible.
  • tailwind-merge conflict resolution is intended — the one behavioral change worth noting (the DataTable.tsx swap from the naive concat-only cn to the tailwind-merge cn) makes conflicting utilities resolve deterministically last-wins instead of by CSS source order. This is precisely the goal the PR set out to establish, not a regression.

If you push additional changes and want a new review, tag @shellhub-io/admin and a team member can trigger it.

@otavio otavio merged commit b631099 into master Jul 13, 2026
16 checks passed
@otavio otavio deleted the refactor/ui/cn-util branch July 13, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants