Skip to content

chore: 🤖 support asset aliases with centralised resolver configuration#876

Open
punkbit wants to merge 14 commits intomainfrom
chore/support-asset-aliases-over-circular-deps-resolution
Open

chore: 🤖 support asset aliases with centralised resolver configuration#876
punkbit wants to merge 14 commits intomainfrom
chore/support-asset-aliases-over-circular-deps-resolution

Conversation

@punkbit
Copy link
Collaborator

@punkbit punkbit commented Mar 5, 2026

Why?

Introduces a centralised asset configuration with unified aliases and deprecated name mappings. It inherits from the original resolver used for the deprecations, but we found that we may want to accept customisation via aliases. Because there are naming conventions to facilitate adding assets easily, the user might want to add custom aliases, e.g. for a case like c#, which has a # character, which is not a valid JavaScript identifier, the user might still want to use c# instead of c-sharp.

⚠️ Depends on #841
⚠️ Closes #873 to favour current due to circular dependencies

How?

  • Creates a common name resolver and an asset resolver factory for consistent name resolution across all asset systems
  • Makes retro active names fn redundant
  • Adapts the resolver pattern in Flags, Icons, Logos, and Payments systems

Preview?

demo-support-asset-aliases-carrot-svg.mov

@changeset-bot
Copy link

changeset-bot bot commented Mar 5, 2026

🦋 Changeset detected

Latest commit: 6181f06

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clickhouse/click-ui Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 46s —— View job


PR Review: chore: 🤖 support asset aliases with centralised resolver configuration

Nice refactor — the centralized resolver is a clean approach and the deletion of the retroactiveNames.ts files is well-executed. Found one clear regression bug and a type safety concern worth addressing.


🚨 Must Fix

  • Flag.tsx — Theme logic is inverted (regression): Line 33 reads:
    resolvedTheme === THEMES.Dark ? FlagsLight[resolvedName] : FlagsDark[resolvedName]
    This is the opposite of the original code (resolvedTheme === 'light' ? FlagsLight : FlagsDark) and will serve FlagsLight assets in dark mode and FlagsDark assets in light mode. Compare with Icon.tsx:31, Logo.tsx:31, and Payment.tsx:31-33 which all correctly use THEMES.Light as the condition. Fix this →

💡 Suggestions (non-blocking)

  • config.tsAssetAlias and AssetDeprecatedName collapse to string: Both aliases and deprecated objects are cast as AssetAliasMap (Record<string, string>), so keyof typeof ASSET_NAME_MAPPINGS.aliases resolves to string, not a literal union. This means name: FlagName | AssetAlias | AssetDeprecatedName is functionally name: string, losing autocomplete and type narrowing on the prop. The as const on the outer object can't undo those inner casts. If you want AssetAlias = 'c#', the aliases object doesn't need as AssetAliasMap — only deprecated needs it (since it's larger and intentionally mutable). Worth considering if type safety on name matters for consumers.

  • config.ts:2-3 — Type declarations depend on mutable runtime value: Since ASSET_NAME_MAPPINGS is exported and mutable (not readonly), types derived from it via typeof reflect the cast widened types anyway. If the intent is for consumers to extend aliases at runtime, this is fine — just worth documenting in the changeset.


Tasks

  • Gather context and read changed files
  • Analyze code for issues
  • Post review

Base automatically changed from fix/circular-dependencies-first-pass to main March 6, 2026 13:25
@punkbit punkbit force-pushed the chore/support-asset-aliases-over-circular-deps-resolution branch from e316245 to 6181f06 Compare March 6, 2026 19:07
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 16s —— View job


PR Review: chore: 🤖 support asset aliases with centralised resolver configuration

Solid refactor — the centralized resolver is a clean pattern, the deletion of per-asset retroactiveNames.ts files is well-executed, and all four components now share a consistent structure. The alias-first, deprecated-second lookup order in resolveAssetName is exactly right.

The previously flagged theme regression in Flag.tsx is not present in the current code — line 27 correctly uses resolvedTheme === 'light' ? FlagsLight[resolvedName] : FlagsDark[resolvedName], consistent with Icon.tsx, Logo.tsx, and Payment.tsx.


💡 Suggestions (non-blocking)

  • config.ts:2-3AssetAlias and AssetDeprecatedName both collapse to string: Because both aliases and deprecated are cast as AssetAliasMap (Record<string, string>), keyof typeof ASSET_NAME_MAPPINGS.aliases and keyof typeof ASSET_NAME_MAPPINGS.deprecated both resolve to string. This means name: FlagName | AssetAlias | AssetDeprecatedName on all four component interfaces simplifies to name: string, losing autocomplete and type narrowing for consumers. If stricter prop types are desired, only deprecated needs the as AssetAliasMap cast (since it's large and intentionally mutable) — aliases can stay uncast to preserve its literal keys (e.g., 'c#') as a proper union type.

  • config.ts:242-244createAssetResolver generic is cosmetic: The type parameter T on createAssetResolver<T>() only affects the cast on the return value. Since resolveAssetName is shared across all asset types, a deprecated Icon name passed to resolveFlagName will be "resolved" to a valid-looking string (e.g., Activityactivity) before the Flag lookup returns null. This is safe (graceful null), but slightly surprising. Worth a brief comment noting that the resolver is intentionally shared and the null-return guard in each component handles unknown names.


Tasks

  • Gather context
  • Read changed files
  • Analyze code for issues
  • Post review

@workflow-authentication-public
Copy link
Contributor

📚 Storybook Preview Deployed

✅ Preview URL: https://click-piesg0mhi-clickhouse.vercel.app

Built from commit: 522f6c693e22f8e3b8e1065c3600f800900348eb

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.

1 participant