feat(clerk-js): Introduce color-mix and css supports utiliies #6151
Conversation
alexcarpenter
requested review from
aeliox,
wobsoriano and
a team
as code owners
🦋 Changeset detectedLatest commit: 262ebef The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
alexcarpenter
requested review from
tmilewski
and removed request for
aeliox and
wobsoriano
📝 WalkthroughWalkthroughThe changes introduce new utilities and supporting infrastructure for CSS color mixing within the codebase. New modules provide functions to generate color scales using CSS Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/clerk-js/src/ui/utils/cssSupports.ts (2)
44-51:propertyparameter is silently ignored for known features
getPropertyForFeaturehard-codes'color'for both registered features, so callers cannot override the property even thoughtestCSSFeatureexposes apropertyargument. Either document that limitation or honour the supplied property.If flexibility is intended, return
defaultPropertywhen no explicit mapping exists:- const propertyMap: Partial<Record<CSSFeature, string>> = { + const propertyMap: Readonly<Partial<Record<CSSFeature, string>>> = { relativeColorSyntax: 'color', colorMix: 'color', }; - return propertyMap[feature] || defaultProperty; + return propertyMap[feature] ?? defaultProperty;Consider deleting the
propertyparameter altogether to avoid a misleading API.
81-83: Narrow the return type ofgetCachedSupports
Record<string, boolean>hides the real key space and loses autocomplete. Prefer:-export const getCachedSupports = (): Record<string, boolean> => { - return Object.fromEntries(supportCache.entries()); -}; +export const getCachedSupports = (): Partial<Record<CSSFeature, boolean>> => ( + Object.fromEntries(supportCache.entries()) as Partial<Record<CSSFeature, boolean>> +);packages/clerk-js/src/ui/utils/colorMix.ts (2)
26-43: Hard-coded step maps look inconsistent withLIGHT_SHADES_COUNT/DARK_SHADES_COUNT
LIGHT_SHADES_COUNTandDARK_SHADES_COUNTare both7, yet the step tables skip certain values (4,6for light;3,5for dark).
This produces uneven deltas (e.g. shade 100 jumps five steps, 200 jumps three).If the goal is to spread shades uniformly, consider deriving steps programmatically:
const shadePositions: Record<ColorShade, number> = { }; const step = shadePositions[shade]; return step < 0 ? `hsl(from ${color} h s calc(l + (${Math.abs(step)} * ((${TARGET_L_50_SHADE} - l) / ${LIGHT_SHADES_COUNT}))))` : step > 0 ? `hsl(from ${color} h s calc(l - (${step} * ((l - ${TARGET_L_900_SHADE}) / ${DARK_SHADES_COUNT}))))` : color;This keeps the intent explicit and avoids manual maps becoming stale.
83-91:shadeshould beColorShadeforgetColorMixAlphaAccepting
numberallows unintended values at compile-time; the function already falls back for unknown shades.-const getColorMixAlpha = (color: string, shade: number): string => { +const getColorMixAlpha = (color: string, shade: ColorShade): string => {If fallback support for arbitrary numbers is required, overloads (or a separate helper) communicate that intent more clearly.
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts (1)
221-238: Edge-case expectation couples tests to implementation detailsFor an empty string the test asserts the exact template literal rather than the observable behaviour (i.e. “returns a valid relative-color expression”).
If the implementation changes (e.g. spacing), the test will fail without a user-visible regression.Prefer a looser assertion:
expect(result.startsWith('hsl(from')).toBe(true);This keeps the test intent while reducing maintenance noise.
packages/clerk-js/src/ui/utils/__tests__/cssSupports.spec.ts (1)
50-62: Cache-behaviour test may pass even if caching is brokenThe test currently:
- Calls
testCSSFeature.- Expects
mockCSSSupportsto have been called once.- Calls
testCSSFeatureagain and expects the call count to remain 1.However, if the second call returns the same boolean without consulting the cache (e.g. via another
CSS.supportscall that also returnstrue), this expectation still passes because the call count becomes2.
To make the assertion robust:expect(mockCSSSupports).toHaveBeenCalledTimes(1); ... testCSSFeature('relativeColorSyntax'); expect(mockCSSSupports).toHaveBeenCalledTimes(1); // unchangedAdd an explicit second expectation after the second invocation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts(1 hunks)packages/clerk-js/src/ui/utils/__tests__/cssSupports.spec.ts(1 hunks)packages/clerk-js/src/ui/utils/colorMix.ts(1 hunks)packages/clerk-js/src/ui/utils/cssSupports.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
| const checkCSSFeatures = (features: CSSFeature[]): Record<CSSFeature, boolean> => { | ||
| return features.reduce( | ||
| (acc, feature) => { | ||
| acc[feature] = testCSSFeature(feature); | ||
| return acc; | ||
| }, | ||
| {} as Record<CSSFeature, boolean>, | ||
| ); |
There was a problem hiding this comment.
Return type of checkCSSFeatures is too strict
When the caller supplies only a subset of features the resulting object lacks the other keys, yet it is cast to Record<CSSFeature, boolean>, breaking type-safety.
-const checkCSSFeatures = (features: CSSFeature[]): Record<CSSFeature, boolean> => {
+const checkCSSFeatures = (features: CSSFeature[]): Partial<Record<CSSFeature, boolean>> => {Down-stream code that expects a full record can still testCSSFeature directly.
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/utils/cssSupports.ts around lines 58 to 65, the
return type of checkCSSFeatures is incorrectly declared as Record<CSSFeature,
boolean>, which implies all CSSFeature keys are present. To fix this, change the
return type to a partial record such as Partial<Record<CSSFeature, boolean>> or
an object type that only includes keys for the supplied features, ensuring type
safety when only a subset of features is provided.
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts (2)
45-58: Tests replicate implementation details – prefer behavioural assertionsThe expected strings hard-code the exact
calc()expression thatgetColorMixcurrently produces.
This couples the test to the internal formula instead of the observable contract (“lighten colour for lower shades / darken for higher shades”). Any harmless refactor (e.g. changing spacing or simplifying math) will break the test.Consider asserting only the essential behaviour, e.g.
expect(result).toMatch(new RegExp(`^hsl\\(from ${testColor} `)); expect(result).toContain('calc(l +'); // or 'calc(l -' for dark shadesThis keeps the test robust while still ensuring the correct path is taken.
Also applies to: 60-73
95-99: Avoidas anyby typing the shade explicitlyCasting to
anyhides type-safety. A lightweight fix keeps strict typing intact:- const result = getColorMix(testColor, Number(shade) as any); + const numericShade = Number(shade) as 100 | 200 | 300 | 400 | 500 | 600 | 700 | 800 | 900; + const result = getColorMix(testColor, numericShade);(or reuse the library’s shade type alias if one exists).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts (1)
170-175: Add the missing negative assertion for completenessThe comment states that
colorMixshould not be queried whenrelativeColorSyntaxis supported, but the assertion is missing.expect(mockCssSupports.relativeColorSyntax).toHaveBeenCalled(); + expect(mockCssSupports.colorMix).not.toHaveBeenCalled();This guards against future regressions in feature-detection precedence.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts (1)
41-55: Tests are over-coupled to the exact HSL string – consider looser assertionsThe expectations hard-code the full
hsl(from …)formula. Any internal refactor (e.g. whitespace, rounding, using%instead of*) will break tests that should still logically pass.
Instead, assert behaviourally:
- light shades: result starts with
hsl(from …and contains'l +'- dark shades: result contains
'l -'This keeps coverage high without freezing the implementation.
Also applies to: 56-69
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts
[error] 9-9: 'afterEach' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 9-9: 'afterEach' is defined but never used.
(unused-imports/no-unused-imports)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts (1)
217-223: Empty-string color edge-case feels synthetic – double-check requirementPassing
''togetColorMixproduces a malformed CSS string that is unlikely to be used in production. Confirm that supporting an empty string is truly required; otherwise remove the test and update the function to throw or return a safe fallback.
| describe('integration tests', () => { | ||
| test('should prioritize relative color syntax over color-mix when both are supported', () => { | ||
| mockCssSupports.relativeColorSyntax.mockReturnValue(true); | ||
| mockCssSupports.colorMix.mockReturnValue(true); | ||
|
|
||
| const result = getColorMix(testColor, 100); | ||
|
|
||
| expect(result).toBe(`hsl(from ${testColor} h s calc(l + (5 * ((97 - l) / 7))))`); | ||
| expect(mockCssSupports.relativeColorSyntax).toHaveBeenCalled(); | ||
| // Should not call colorMix since relativeColorSyntax is supported | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert that colorMix is NOT called when relative syntax is available
The goal of this test is to prove that getColorMix prefers relative color syntax.
You already left a comment (💬 171) but the assertion is missing, allowing a silent regression.
expect(mockCssSupports.relativeColorSyntax).toHaveBeenCalled();
- // Should not call colorMix since relativeColorSyntax is supported
+ expect(mockCssSupports.colorMix).not.toHaveBeenCalled();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('integration tests', () => { | |
| test('should prioritize relative color syntax over color-mix when both are supported', () => { | |
| mockCssSupports.relativeColorSyntax.mockReturnValue(true); | |
| mockCssSupports.colorMix.mockReturnValue(true); | |
| const result = getColorMix(testColor, 100); | |
| expect(result).toBe(`hsl(from ${testColor} h s calc(l + (5 * ((97 - l) / 7))))`); | |
| expect(mockCssSupports.relativeColorSyntax).toHaveBeenCalled(); | |
| // Should not call colorMix since relativeColorSyntax is supported | |
| }); | |
| describe('integration tests', () => { | |
| test('should prioritize relative color syntax over color-mix when both are supported', () => { | |
| mockCssSupports.relativeColorSyntax.mockReturnValue(true); | |
| mockCssSupports.colorMix.mockReturnValue(true); | |
| const result = getColorMix(testColor, 100); | |
| expect(result).toBe(`hsl(from ${testColor} h s calc(l + (5 * ((97 - l) / 7))))`); | |
| expect(mockCssSupports.relativeColorSyntax).toHaveBeenCalled(); | |
| + expect(mockCssSupports.colorMix).not.toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts around lines 161 to
172, the test lacks an assertion to confirm that the colorMix function is not
called when relativeColorSyntax is supported. Add an explicit expectation to
verify that mockCssSupports.colorMix has not been called, ensuring the test
properly validates that getColorMix prioritizes relative color syntax over
colorMix.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts (1)
161-171: Still missing the “should NOT call colorMix” assertionThe integration test documents the expectation in a comment but never verifies it.
Add the explicit check so a regression can’t slip through:expect(mockCssSupports.relativeColorSyntax).toHaveBeenCalled(); - // Should not call colorMix since relativeColorSyntax is supported + expect(mockCssSupports.colorMix).not.toHaveBeenCalled();
🧹 Nitpick comments (2)
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts (2)
1-9: Import order: pullviin before it’s used
vi.mockis executed before the currentviimport is evaluated.
While ESM hoisting makes this technically legal, it’s non-intuitive and upsets many static-analysis / IDE tools. Pulling theviimport to the very top keeps the file readable without breaking the “mock-before-module-under-test” rule.-// Mock the cssSupports module -vi.mock('../cssSupports', () => ({ - cssSupports: { - relativeColorSyntax: vi.fn(), - colorMix: vi.fn(), - }, -})); - -import { beforeEach, describe, expect, test, vi } from 'vitest'; +import { vi } from 'vitest'; + +// Mock the cssSupports module +vi.mock('../cssSupports', () => ({ + cssSupports: { + relativeColorSyntax: vi.fn(), + colorMix: vi.fn(), + }, +})); + +import { beforeEach, describe, expect, test } from 'vitest';
41-69: Expectations are ultra-brittle – prefer a pattern or snapshotAsserting the exact
calc()string makes the test fragile: a harmless refactor (e.g. dropping superfluous parentheses or changing whitespace) will break it, even though behaviour is identical.Consider relaxing the assertion:
- expect(result).toBe(`hsl(from ${testColor} h s calc(l + (${steps} * ((97 - l) / 7))))`); + expect(result).toMatch( + new RegExp( + `^hsl\\(from ${testColor.replace('#', '\\\\#')} h s calc\\(l \\+ .*\\)\\)$`, + ), + );or switch to inline snapshots so the intent survives future formatting tweaks.
This applies to both the light-shade and dark-shade branches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts (1)
218-223: Empty-string edge case asserts invalid CSS – confirm this is intentional
hsl(from h s …)is not valid CSS; the space where the colour should be ends up empty.
If the production code really returns this, is that the desired fallback?
A safer expectation might be that an empty input is returned unchanged or throws, rather than generating malformed CSS.Please double-check the underlying utility’s behaviour and adjust either the implementation or the test accordingly.
| const getRelativeColorSyntax = (color: string, shade: ColorShade): string => { | ||
| const { TARGET_L_50_SHADE, TARGET_L_900_SHADE, LIGHT_SHADES_COUNT, DARK_SHADES_COUNT } = CONFIG; | ||
|
|
||
| const lightShadeMap: Record<number, number> = { 400: 1, 300: 2, 200: 3, 100: 5 }; |
There was a problem hiding this comment.
should this be Record<ColorShade, number>?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/clerk-js/src/ui/utils/colorMix.ts (5)
3-8: Add documentation comments for CONFIG constants.The CONFIG constants would benefit from JSDoc comments explaining what each value represents, especially the target lightness values and their significance in the color mixing calculations.
+/** + * Configuration constants for color mixing calculations + */ const CONFIG = { + /** Target lightness value for the lightest shade (50) */ TARGET_L_50_SHADE: 97, + /** Target lightness value for the darkest shade (900) */ TARGET_L_900_SHADE: 12, + /** Number of steps for light shades (25-400) */ LIGHT_SHADES_COUNT: 7, + /** Number of steps for dark shades (600-950) */ DARK_SHADES_COUNT: 7, } as const;
12-24: Add JSDoc documentation for the main function.The function logic is well-structured with proper feature detection and fallback behavior. Consider adding JSDoc documentation to explain the parameters and return behavior.
+/** + * Generates a color variant based on the specified shade using CSS feature detection + * @param color - Base color in any valid CSS format + * @param shade - Target shade value (25-950, with 500 being the base) + * @returns CSS color string adjusted to the specified shade + */ const getColorMix = (color: string, shade: ColorShade): string => {
26-60: Add explanatory comments for complex lightness calculations.The HSL lightness calculation formulas are mathematically complex and would benefit from comments explaining the interpolation logic.
if (shade in lightShadeMap) { const steps = lightShadeMap[shade]; + // Interpolate lightness towards TARGET_L_50_SHADE based on step count return `hsl(from ${color} h s calc(l + (${steps} * ((${TARGET_L_50_SHADE} - l) / ${LIGHT_SHADES_COUNT}))))`; } if (shade in darkShadeMap) { const steps = darkShadeMap[shade]; + // Interpolate lightness towards TARGET_L_900_SHADE based on step count return `hsl(from ${color} h s calc(l - (${steps} * ((l - ${TARGET_L_900_SHADE}) / ${DARK_SHADES_COUNT}))))`; }
62-96: Add documentation for the lookup tables.The lookup tables are well-structured, but would benefit from documentation explaining their purpose and how the percentage values were determined.
+/** + * Lookup table for color-mix syntax approach + * Maps shade values to mix color (white/black) and percentage for visual consistency + */ const SHADE_MIX_DATA = { // ... existing content } as const; +/** + * Alpha percentage values for creating transparent color overlays + * Maps shade values to opacity percentages + */ const ALPHA_PERCENTAGES = { // ... existing content } as const;
112-120: Consider improving type safety for undefined case.The undefined check is good defensive programming, but given that ALPHA_PERCENTAGES has all ColorShade keys, this case should theoretically never occur. Consider adding a type assertion or comment to clarify this.
const getColorMixAlpha = (color: string, shade: ColorShade): string => { const alphaPercentage = ALPHA_PERCENTAGES[shade]; - if (alphaPercentage === undefined) { + // This should never happen as ALPHA_PERCENTAGES covers all ColorShade values + if (alphaPercentage === undefined) { return color; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/clerk-js/src/ui/utils/colorMix.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/clerk-js/src/ui/utils/colorMix.ts (2)
10-10: ColorShade type definition looks comprehensive.The type definition properly includes all shade values including the half-shades (150, 750, 850) that were mentioned in previous review comments. This ensures type safety across all the mapping objects in the file.
98-110: Helper functions are well-implemented.The helper functions are clean and focused with proper handling of the base color case (shade 500).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/clerk-js/src/ui/utils/colors/alphaScale.ts (1)
6-8: Simplify the function parameter type.The parameter type union is unnecessarily complex. Since
CssColorOrScalelikely includesstring | ColorScale<string>, the current union is redundant.-export const generateAlphaScale = ( - color: string | ColorScale<string> | CssColorOrScale | undefined, -): ColorScale<string> => { +export const generateAlphaScale = (color: CssColorOrScale | undefined): ColorScale<string> => {packages/clerk-js/src/ui/utils/colors/utils.ts (2)
115-123: Inconsistent function visibility.The
getColorMixAlphafunction is defined but not exported, while similar functions likegetColorMixare exported. This inconsistency could lead to confusion about the intended API surface.Either export the function if it's part of the public API:
-const getColorMixAlpha = (color: string, shade: ColorShade): string => { +export const getColorMixAlpha = (color: string, shade: ColorShade): string => {Or remove it from the exports list:
-export { getColorMix, getColorMixAlpha, colorMix, applyScalePrefix }; +export { getColorMix, colorMix, applyScalePrefix };
125-127: Consider adding type safety to scale prefix utility.The
applyScalePrefixfunction uses generic Object methods which lose type information. Consider making it more type-safe.-const applyScalePrefix = (scale: ColorScale<string | undefined>, prefix: string) => { +const applyScalePrefix = <T>(scale: ColorScale<T>, prefix: string): Record<string, T> => { return Object.fromEntries(Object.entries(scale).map(([shade, color]) => [prefix + shade, color])); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/clerk-js/src/ui/customizables/parseVariables.ts(1 hunks)packages/clerk-js/src/ui/utils/__tests__/colorMix.spec.ts(1 hunks)packages/clerk-js/src/ui/utils/colors/alphaScale.ts(1 hunks)packages/clerk-js/src/ui/utils/colors/lightnessScale.ts(1 hunks)packages/clerk-js/src/ui/utils/colors/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/ui/utils/tests/colorMix.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: triage
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
packages/clerk-js/src/ui/utils/colors/lightnessScale.ts (1)
6-50: Well-structured color scale generation utility with proper progressive enhancement.The implementation correctly handles different input types and gracefully falls back through CSS feature support levels. The early returns and type checking provide good defensive programming.
packages/clerk-js/src/ui/customizables/parseVariables.ts (2)
20-53: Well-implemented progressive enhancement for color scale generation.The conditional logic properly detects CSS feature support and integrates the new utilities effectively. The approach of generating alpha scales from the 500-level colors of the lightness scales is logical and maintains consistency.
26-30: ```shell
#!/bin/bashDisplay color utils (applyScalePrefix, COLOR_SCALE, getColorMixSyntax, getRelativeColorSyntax)
sed -n '1,200p' packages/clerk-js/src/ui/utils/colors/utils.ts
</details> <details> <summary>packages/clerk-js/src/ui/utils/colors/utils.ts (4)</summary> `5-10`: **Well-defined configuration constants.** The configuration constants provide clear targets for lightness calculations and are properly typed as const. This makes the color generation predictable and maintainable. --- `15-27`: **Solid progressive enhancement pattern in color mixing.** The `getColorMix` function properly cascades through CSS feature support levels with appropriate fallbacks. The early return for shade 500 is efficient. --- `29-63`: **Complex but mathematically sound relative color syntax generation.** The lightness calculations use proper HSL math to distribute shades evenly between target lightness values. The separate mapping objects for light and dark shades provide clear, maintainable configuration. --- `65-81`: **Well-calibrated shade mixing percentages.** The predefined mixing data provides consistent shade generation across the color scale. The percentages appear well-balanced for creating visually pleasing color progressions. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| return output as ColorScale<string>; | ||
| } |
There was a problem hiding this comment.
Address unsafe type assertions.
The function uses multiple as ColorScale<string> assertions while the internal output object contains undefined values. This creates a type safety gap where the return type doesn't match the actual runtime values when CSS features aren't supported.
Consider one of these approaches:
Option 1: Make the return type honest about potential undefined values:
-): ColorScale<string> => {
+): ColorScale<string | undefined> => {
- return output as ColorScale<string>;
+ return output;
- return color as ColorScale<string>;
+ return color;
- return output as ColorScale<string>;
+ return output;
- return output as ColorScale<string>;
+ return output;Option 2: Provide fallback values instead of undefined:
const output: ColorScale<string | undefined> = {
- '25': undefined,
+ '25': 'transparent',
// ... repeat for all shades
};Also applies to: 31-32, 39-39, 42-42
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/utils/colors/alphaScale.ts around lines 28-29,
31-32, 39, and 42, the function returns output cast as ColorScale<string> while
output may contain undefined values, causing unsafe type assertions. To fix
this, either update the return type to allow undefined values explicitly or
ensure all undefined entries in output are replaced with appropriate fallback
string values before returning, so the return type accurately reflects the
runtime data.
Description
Introduce new color-mix and css supports utilities to support css variable usage within the appearance variables property.
Resolves USER-2201
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit