refactor: className 構築を cx ヘルパーで統一 (#260) - #657
Conversation
template literal + `.trim()` 方式では条件付き className が空文字になると 連続空白(二重空白)が残る問題があった。falsy 値を除去し単一スペースで 結合する cx ヘルパー(src/utils/cx.ts)を新設し、src/components/ui 配下で 動的に className を組み立てている箇所を統一した。 - 新設: src/utils/cx.ts と単体テスト(15 ケース) - 移行: ActionButton / BareInput / ChevronIcon / ChipLabel / ClearButton / CloseIcon / CopyButton / FileInputButton / OutputField / ProgressBar / ResultTable / Section / StatusBadge / ToggleGroup - 対象外: NotificationBanner(条件なし補間のみ)、CountInput / Select / ErrorMessage(静的文字列のみ)は連続空白リスクがないため変更なし クラス名・順序は変えない純粋リファクタで視覚変化なし。
🖼️ Visual Regression Test 結果
|
fumtas1k
left a comment
There was a problem hiding this comment.
レビュー結果(多角的観点)
結論: 方針・実装ともに妥当な純粋リファクタ。承認相当ですが、1 点だけ確認したい挙動差(非ブロッカー)があるため COMMENT で出します。
✅ ロジック / 正確性
cx = values.filter(Boolean).join(' ')は意図どおり。.trim()方式が直せていなかった中間の二重空白を確実に除去できています。実際に develop のCopyButtoncompact パスbtn-copy is-compact ${stateClass} rounded-md…はstateClass===''でis-compact rounded-mdの二重空白が残り、末尾.trim()では消えていませんでした → 本 PR で解消。これは修正の主目的と一致します。- 各移行のクラス名・順序は等価です。
ChevronIconのopen ? ' rotate-180' : ''→open && 'rotate-180'、StatusBadge/ChipLabelの${className ? \${className}` : ''}→ 末尾className` 引数化、いずれも出力同値。
🟡 確認したい 1 点(非ブロッカー)— Section.tsx
truthy 判定 headerSlot ? を headerSlot != null に変更していますが、これは厳密には挙動が変わるケースがあります。
headerSlot={cond && <X/>}(React で頻出パターン)で cond===false のとき headerSlot は false になります。
- 旧:
headerSlot ?→false⇒ flex レイアウトクラス 付かない - 新:
headerSlot != null→false != nullは true ⇒ flex justify-between が付く
hasHeader 側が != null なので title 単独でヘッダ div は描画され、そこに余計な flex … justify-between が乗ります。子が title span 1 個なので justify-between でも左寄せのまま=**おそらく視覚回帰なし(VRT 60/60 pass とも整合)**ですが、潜在的な意味のズレです。
ClassValue 型互換(ReactNode 不可)のために boolean 化が必要だっただけなら、元の truthy 挙動を完全保存する !!headerSlot && '…' の方が忠実です。!= null(= hasHeader と揃える)が意図的な選択なら、その旨で OK。判断はお任せします。
✅ アーキテクチャ
clsx追加でなく内部cx採用は、最小依存方針(SPEC/decisions/lock 更新負荷の回避)と整合。良い判断です。- 対象を
src/components/uiに限定したのも issue #260 の主旨どおりで適切。
✅ セキュリティ
- className 文字列の結合のみで、
dangerouslySetInnerHTML等への外部入力流入はなし。XSS 等の新規攻撃面なし。
✅ フロントエンド / デザイン
- クラス名・順序不変の純粋リファクタで視覚変化なし。VRT 全 60 件 pass、
@layer componentsの variant 制約(hover:等の手書きクラス問題)にも抵触しない範囲です。
✅ テスト
- 15 ケースは結合 / falsy 除去 / 二重空白なし / 前後空白なし / 全 falsy / 数値を網羅。陽性対照(残るべき値が残る)と陰性対照(消えるべき値が消える)の両方を持っており十分です。
cxはガード/検知器ではなくユーティリティのため test-gates の陽性対照要件も満たしています。
💡 nit / フォローアップ(任意)
ClassValueにnumberを含めていますが、現状 ui の呼び出しに数値引数はありません(clsx慣習に沿うだけで無害)。cx('w-', 4) => 'w- 4'(接頭辞と数値の間にスペース)はやや不自然な挙動ですが実使用がないので問題なし。- スコープ外ですが
src/components/tools/*にも同型の\…${cond ? 'x' : ''}…`パターンが多数あり、一部は同じ潜在二重空白を抱えています(例:CertDecoder.tsx:287はmono===falseでgap-2 captionの二重空白、.trim()もなし)。#260 の範囲外で本 PR の判断は正しいですが、tools/へのcx` 横展開を別 issue 化すると一貫性が上がります。
LGTM。Section.tsx の != null が意図どおりなら、そのまま merge で問題ありません。
Generated by Claude Code
cx の ClassValue 型に合わせた条件式を `headerSlot != null` としていたが、
`headerSlot={cond && <X/>}` で cond===false のとき false != null が true となり
旧実装(truthy 判定)と挙動が変わるケースがあった。純粋リファクタの趣旨に従い
`!!headerSlot` に変更して元の挙動を完全保存する。
|
レビューありがとうございます。 🟡 💡 nit #2(tools/ への横展開): スコープ外のため follow-up として #658 を起票しました( nit #1( Generated by Claude Code |
fumtas1k
left a comment
There was a problem hiding this comment.
再レビュー(commit 9116573)
修正を確認しました。👍
✅ Section.tsx — 指摘点の解消
headerSlot != null → !!headerSlot && '…' に変更され、旧実装の truthy 判定を完全保存できています。
!!headerSlotは boolean を返すためClassValue(falseを含む)と型互換headerSlot={cond && <X/>}でcond===false(=false)/0/''のとき flex レイアウトが付かず、元のheaderSlot ?と同一挙動
これで Section は純粋リファクタの趣旨どおりになりました。残る指摘(tools/ への横展開)はスコープ外の任意フォローアップなので本 PR では対応不要です。
💡 nit(任意・本文のみ)
PR 本文「## 補足」が旧記述(headerSlot != null とした)のままなので、merge 前に !!headerSlot(元 truthy 挙動を保存)へ更新しておくと履歴と整合します。コードには影響しません。
LGTM。 コード上のブロッカーはありません。npm run test / astro check が green であれば merge して問題ないと判断します。
Generated by Claude Code
概要
issue #260 対応。
template literal + .trim()方式では条件付き className が空文字になると**連続空白(二重空白)**が残る問題があった(CopyButton.tsx等)。.trim()は前後のみで中間空白は除去されない。falsy 値を除去し単一スペースで結合する内部ヘルパー
cx(src/utils/cx.ts)を新設し、src/components/ui/配下で動的に className を組み立てている箇所を統一した。変更点
src/utils/cx.ts+ 単体テストsrc/utils/__tests__/cx.test.ts(15 ケース: 結合 / falsy 除去 / 連続空白なし / 前後空白なし / 全 falsy / 数値)NotificationBanner(--${variant}補間のみ・条件なし)、CountInput/Select/ErrorMessage(静的文字列のみ)は連続空白リスクがないため変更なしクラス名・順序は一切変えない純粋リファクタで、視覚変化なし(連続空白の除去のみ)。VRT baseline 更新は不要。
補足
Section.tsxは cx のClassValue型(ReactNode を含まない)に合わせ、条件部を!!headerSlot && '…'とした(boolean 化で型互換)。!!headerSlotは旧実装の truthy 判定(headerSlot ?)を完全保存するため、headerSlot={cond && <X/>}でcond===false/0/''のときも元と同一挙動(flex レイアウトを付けない)。検証
node_modules/.bin/astro check: 0 errors / 0 warnings / 0 hints(401 files)npm run test:cx.test.ts15 件含む 2086 件 passsw-cache-version×2 はdist/sw.js未生成で要npm run build、codex-git-add-files×1 は git shell スクリプトのサンドボックス差)。いずれも本変更と無関係。フォローアップ
src/components/tools/*へのcx横展開(同型の二重空白パターンが残存): cx ヘルパーを src/components/tools/* へ横展開(className 構築統一) #658Closes #260
https://claude.ai/code/session_01GEXKuG45NXc9tHZP2SBjr4