Skip to content

Add icon compatibility layer for Semantic UI → Lucide migration#1027

Merged
JSv4 merged 3 commits intomainfrom
claude/resolve-issue-1012-PYpGf
Mar 1, 2026
Merged

Add icon compatibility layer for Semantic UI → Lucide migration#1027
JSv4 merged 3 commits intomainfrom
claude/resolve-issue-1012-PYpGf

Conversation

@JSv4
Copy link
Copy Markdown
Collaborator

@JSv4 JSv4 commented Feb 28, 2026

Summary

Introduces a compatibility layer to support both legacy Semantic UI icon names and native Lucide icon names, enabling a gradual migration from Semantic UI to Lucide React. This allows existing code using SUI icon names to continue working while new code can use Lucide names directly.

Key Changes

  • iconCompat.ts: New utility module providing:

    • SEMANTIC_TO_LUCIDE mapping of ~100 SUI icon names to their Lucide equivalents
    • resolveIconName() function to convert SUI names to Lucide kebab-case names with fallback to help-circle
    • resolveIcon() function to resolve names directly to Lucide components
    • Explicit imports of only the ~100 icons actually used (avoiding barrel import bloat)
  • DynamicIcon.tsx: New React component that:

    • Accepts icon names as strings (both SUI and Lucide formats)
    • Renders the resolved Lucide icon with configurable size, color, and stroke width
    • Supports accessibility attributes (aria-label, aria-hidden)
    • Gracefully falls back to HelpCircle for unknown names
  • iconCompat.test.ts: Comprehensive unit tests covering:

    • All SUI-to-Lucide mappings
    • Lucide passthrough for already-valid names
    • Fallback behavior for unknown icons
    • Whitespace and case normalization
    • Alias consistency (e.g., "check" and "checkmark" resolve to same component)
  • DynamicIcon.ct.tsx: Component tests validating:

    • Rendering of SUI and Lucide icon names
    • Fallback rendering for unknown names
    • Accessibility attributes
    • Visual gallery of supported icons

Implementation Details

  • Uses explicit imports from lucide-react to ensure only referenced icons are bundled
  • Icon name normalization (lowercase, trim, collapse whitespace) enables case-insensitive lookups
  • Comprehensive mapping covers common SUI aliases and variants (e.g., "file", "file outline", "file text" all map to file-text)
  • Accessibility-first: icons are aria-hidden by default unless an aria-label is provided

https://claude.ai/code/session_01MVtcTrYsbeNnDpFoVbxgvm

Build the foundational icon compatibility layer that enables all subsequent
Semantic UI → Lucide icon migration work:

- Add `resolveIconName()` and `resolveIcon()` utilities in
  `frontend/src/utils/iconCompat.ts` with a tree-shakeable KNOWN_ICONS map
  (~98 explicit Lucide imports) and a SEMANTIC_TO_LUCIDE mapping table
  (122 entries covering all SUI icon names used in the codebase)
- Add `<DynamicIcon>` component in
  `frontend/src/components/widgets/icon-picker/DynamicIcon.tsx` that accepts
  string-based icon names (SUI or Lucide) with aria-label/aria-hidden support
- Add 20 Vitest unit tests covering mapping correctness, passthrough,
  fallback, alias consistency, whitespace handling, and case sensitivity
- Add 6 Playwright component tests verifying rendering of SUI names,
  Lucide names, unknown names, and accessibility attributes

https://claude.ai/code/session_01MVtcTrYsbeNnDpFoVbxgvm
@claude
Copy link
Copy Markdown

claude bot commented Feb 28, 2026

Code Review

Overall this is a clean, well-structured addition. The explicit-imports strategy for lucide-react is the right call for bundle size, and the test coverage is solid. A few things worth addressing before merge:


Bugs / Semantic Issues

1. normalise vs normalize (naming convention)

The codebase uses American English throughout (colorUtils.ts: normalizeHexColor, navigationUtils.ts: normalize). The private normalise function in iconCompat.ts is inconsistent. Rename to normalize for consistency.

2. Missing AlertCircle — incorrect fallback mappings

AlertCircle is not imported and not in KNOWN_ICONS, so two SUI mappings degrade silently:

"exclamation circle": "info",           // shows i-circle, should show !-circle
"warning circle":     "alert-triangle", // shows triangle, should show circle with !

Lucide has AlertCircle which is the correct semantic match for both. Fix:

import { AlertCircle, ... } from "lucide-react";

// In KNOWN_ICONS:
"alert-circle": AlertCircle,

// In SEMANTIC_TO_LUCIDE:
"exclamation circle": "alert-circle",
"warning circle":     "alert-circle",

3. exchange maps to arrow-right (wrong semantics)

SUI's exchange icon is bidirectional. arrow-right is one-way. Lucide has ArrowLeftRight which is the correct equivalent:

import { ArrowLeftRight, ... } from "lucide-react";
// KNOWN_ICONS: "arrow-left-right": ArrowLeftRight,
// SEMANTIC_TO_LUCIDE: "exchange": "arrow-left-right",

4. upload SUI name maps to cloud-upload (potentially surprising)

SEMANTIC_TO_LUCIDE maps "upload" -> "cloud-upload", but the plain Upload icon (already in KNOWN_ICONS) is semantically closer for generic upload actions. Cloud-upload is specifically for cloud storage. Worth an explicit comment at minimum, or reconsider the mapping.


Code Quality

5. Misplaced comment in KNOWN_ICONS

// Note: "spinner" SUI name maps to "loader" (Lucide's Loader icon)
loader: Loader,

This comment lives inside KNOWN_ICONS (Lucide names -> components) but describes a SEMANTIC_TO_LUCIDE mapping. Move it to the spinner entry in SEMANTIC_TO_LUCIDE:

spinner: "loader", // Lucide uses "Loader" for animated spinner

6. Hardcoded mapsToHelpCircle set in tests is fragile

const mapsToHelpCircle = new Set(["question circle outline"]);

If new SUI names that intentionally map to help-circle are added, this test gives a false-positive failure. Derive it dynamically:

const mapsToHelpCircle = new Set(
  Object.entries(SEMANTIC_TO_LUCIDE)
    .filter(([, v]) => v === "help-circle")
    .map(([k]) => k)
);

Accessibility

7. Missing role="img" when aria-label is present

When an aria-label is provided, the SVG should carry role="img" so screen readers announce it correctly. Lucide SVGs don't set a role by default:

<IconComponent
  role={ariaLabel ? "img" : undefined}
  aria-label={ariaLabel}
  aria-hidden={ariaHidden ?? !ariaLabel}
  ...
/>

The component test for aria-label should also assert role="img" is present.


Minor

8. Magic number for default size

size = 16 is a bare literal. Per project convention (constants.ts already has PIPELINE_UI.ICON_SIZE), consider adding a DYNAMIC_ICON_DEFAULT_SIZE constant (or similar) to constants.ts. Not blocking, but keeps things searchable/configurable.

9. No barrel export for the icon-picker directory

icon-picker/ currently only contains DynamicIcon.tsx. An index.ts re-export would align with other widget directories and simplify imports for consumers.


What's Good

  • The two-lookup strategy (SEMANTIC_TO_LUCIDE -> KNOWN_ICONS) is clean and avoids duplication.
  • Normalization (lowercase + whitespace collapse) before lookup is robust for the SUI name format.
  • Defaulting to aria-hidden={true} when no label is provided is correct accessibility behaviour.
  • The gallery component test with docScreenshot follows the project screenshot convention exactly.
  • Comprehensive alias consistency tests are a nice touch.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

JSv4 added 2 commits February 28, 2026 14:18
- Rename normalise→normalize for American English consistency
- Import CircleAlert and ArrowLeftRight from lucide-react
- Fix "exclamation circle" and "warning circle" to map to circle-alert
- Fix "exchange" to map to arrow-left-right (bidirectional)
- Add role="img" to SVG when aria-label is present for accessibility
- Replace magic number 16 with DYNAMIC_ICON_DEFAULT_SIZE constant
- Move spinner comment from KNOWN_ICONS to SEMANTIC_TO_LUCIDE
- Add explanatory comment for upload→cloud-upload mapping choice
- Replace hardcoded mapsToHelpCircle set with dynamic derivation
- Create barrel export index.ts for icon-picker module
- Fix alphabetical ordering in KNOWN_ICONS (loader before lock)
- Update tests for new warning circle behavior and role="img"
@claude
Copy link
Copy Markdown

claude bot commented Feb 28, 2026

Follow-up Review (Corrected Analysis)

The previous review comment on this PR (also from this account) contained several inaccurate bug reports. Looking at the actual code, all of those alleged issues are already correctly implemented:

  • normalize (not normalise) is already the function name
  • CircleAlert (not AlertCircle) is imported and correctly handles warning circle and exclamation circle
  • exchange already maps to arrow-left-right
  • The spinner comment is already in SEMANTIC_TO_LUCIDE (not KNOWN_ICONS)
  • The test already derives mapsToHelpCircle dynamically
  • role={ariaLabel ? "img" : undefined} is already in the component
  • DYNAMIC_ICON_DEFAULT_SIZE constant is already in constants.ts
  • index.ts barrel export already exists

Sorry for the noise.


Real Issues Found

1. Dead Upload import and KNOWN_ICONS entry (minor)

SEMANTIC_TO_LUCIDE["upload"] = "cloud-upload" causes resolveIconName("upload") to return early at line 409, so KNOWN_ICONS["upload"] at line 210 is never reached. The Upload component is imported and registered but unreachable via resolveIcon("upload"). Since no other SUI name maps to plain "upload", this entry is dead code.

Fix options:

  • Remove Upload from imports and KNOWN_ICONS (cloud-upload is clearly the intended SUI mapping, per the inline comment on line 374)
  • Or add an explicit note that the KNOWN_ICONS entry is shadowed by the SUI mapping

2. Misleading JSDoc on aria-hidden prop (minor)

DynamicIcon.tsx:18 — The JSDoc says "When true (default) the icon is hidden from assistive technology." The actual default is !ariaLabel — the icon is hidden only when no aria-label is provided. Suggested replacement: "Defaults to true when no aria-label is provided, false otherwise."

3. Test gap: className and strokeWidth not tested (very minor)

The component tests cover rendering and accessibility paths well, but do not assert that className and strokeWidth props are forwarded to the underlying SVG element.


What Is Well Done

  • The two-level lookup (SEMANTIC_TO_LUCIDE then KNOWN_ICONS) is clean and avoids duplicating name strings alongside component references.
  • Normalization (lowercase + whitespace collapse) before lookup handles SUI multi-word names correctly.
  • aria-hidden defaulting based on ariaLabel presence is accessibility-correct out of the box.
  • Explicit imports over a barrel import is the right call for tree-shaking / bundle size.
  • Alias consistency tests are a good confidence net for callers using different SUI name variants.
  • Gallery screenshot test follows the project docScreenshot convention correctly.

Overall this is solid, well-structured work. The dead Upload entry is the only thing worth fixing; the rest are documentation nits.

@JSv4 JSv4 merged commit 578d902 into main Mar 1, 2026
14 checks passed
@JSv4 JSv4 deleted the claude/resolve-issue-1012-PYpGf branch March 1, 2026 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants