Skip to content

fix: buildCssSnippet self-referential var() bug + slot styling guidance #158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
himerus merged 3 commits into from
Mar 21, 2026
+142 −36
Merged

fix: buildCssSnippet self-referential var() bug + slot styling guidance #158

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f0831a
fix: buildCssSnippet self-referential var() bug + slot styling guidan…
himerus Mar 21, 2026
0192413
style: format benchmark fixture JSON files
himerus Mar 21, 2026
bd07aff
chore: add changeset for CSS snippet fix
himerus Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 51 additions & 4 deletions packages/core/src/handlers/styling-diagnostics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,16 @@ export function buildAntiPatterns(meta: ComponentMetadata): AntiPatternWarning[]
});
}

// Warn about font vs layout inheritance through slots
if (meta.slots.length > 0) {
warnings.push({
pattern: `${tag}::slotted(div) { margin: 10px; } /* expecting layout to inherit */`,
explanation:
'Slotted content inherits font styles (color, font-size, line-height) from the shadow DOM, but layout properties (margin, padding, display, width) must be set in light DOM CSS — they do not inherit through the shadow boundary.',
correctApproach: `Style layout in light DOM CSS: ${tag} > div { margin: 10px; }. Font properties like color and font-size will inherit from the component's shadow DOM automatically.`,
});
}

return warnings;
}

Expand All @@ -200,13 +210,14 @@ export function buildCssSnippet(meta: ComponentMetadata): string {
const lines: string[] = [];
const tag = meta.tagName;

// Token customization section
// Token customization section — show how to OVERRIDE custom properties
if (meta.cssProperties.length > 0) {
lines.push(`/* Token customization */`);
lines.push(`/* Token customization — override on the component host */`);
lines.push(`${tag} {`);
for (const prop of meta.cssProperties.slice(0, 5)) {
const defaultVal = prop.description ? `/* ${prop.description} */` : '';
lines.push(` ${prop.name}: var(${prop.name}) ${defaultVal};`.trimEnd());
const value = prop.default ?? guessDefaultValue(prop.name);
const comment = prop.description ? ` /* ${prop.description} */` : '';
lines.push(` ${prop.name}: ${value};${comment}`);
}
lines.push(`}`);
}
Expand All @@ -223,6 +234,25 @@ export function buildCssSnippet(meta: ComponentMetadata): string {
}
}

// Slot styling section — show how to style slotted content in light DOM
if (meta.slots.length > 0) {
lines.push('');
lines.push(`/* Slot styling — target slotted elements in light DOM CSS */`);
const hasDefaultSlot = meta.slots.some((s) => s.name === '');
const namedSlots = meta.slots.filter((s) => s.name !== '');

if (hasDefaultSlot) {
lines.push(`${tag} > * { /* styles for default slot content */ }`);
}
for (const slot of namedSlots.slice(0, 3)) {
const desc = slot.description ? ` /* ${slot.description} */` : '';
lines.push(`${tag} [slot="${slot.name}"] { ${desc.trim()} }`);
}
Comment on lines +247 to +250
Copy link

@coderabbitai coderabbitai bot Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Minor formatting inconsistency when slot description is empty.

When slot.description is falsy, desc is '' and desc.trim() produces an empty string, resulting in output like my-button [slot="prefix"] { } with a double space before the closing brace. This is purely cosmetic but slightly inconsistent with other generated CSS.

✨ Optional: Clean up empty description handling 
     for (const slot of namedSlots.slice(0, 3)) {
       const desc = slot.description ? ` /* ${slot.description} */` : '';
-      lines.push(`${tag} [slot="${slot.name}"] { ${desc.trim()} }`);
+      lines.push(`${tag} [slot="${slot.name}"] {${desc} }`);
     }
📝 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.

Suggested change
for (const slot of namedSlots.slice(0, 3)) {
const desc = slot.description ? ` /* ${slot.description} */` : '';
lines.push(`${tag} [slot="${slot.name}"] { ${desc.trim()} }`);
}
for (const slot of namedSlots.slice(0, 3)) {
const desc = slot.description ? ` /* ${slot.description} */` : '';
lines.push(`${tag} [slot="${slot.name}"] {${desc} }`);
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/handlers/styling-diagnostics.ts` around lines 247 - 250,
The generated slot CSS includes an extra space when slot.description is empty;
update the formatting in the loop over namedSlots so you conditionally render
the description and its surrounding spaces rather than always inserting
desc.trim(). Specifically, in the block that builds desc and calls lines.push
(referencing namedSlots, desc, tag, slot.name, and the lines.push call), change
the push so it emits `{ ${slot.description.trim()} }` only when a non-empty
description exists and otherwise emits `{ }` (no double space).

lines.push(`/* Note: slotted content inherits font styles (color, font-size)`);
lines.push(` from the shadow DOM, but layout (margin, padding, display)`);
lines.push(` must be set here in light DOM CSS. */`);
}

if (lines.length === 0) {
lines.push(`/* ${tag} exposes no CSS customization points in its CEM. */`);
lines.push(`/* Check the component documentation for styling options. */`);
Expand All @@ -231,6 +261,23 @@ export function buildCssSnippet(meta: ComponentMetadata): string {
return lines.join('\n');
}

/**
* Guesses a sensible placeholder value based on CSS property name patterns.
* Used when the CEM doesn't specify a default value.
*/
function guessDefaultValue(propName: string): string {
const lower = propName.toLowerCase();
if (/color|bg|background/.test(lower)) return '#value';
if (/size|font/.test(lower)) return '1rem';
if (/radius/.test(lower)) return '4px';
if (/spacing|padding|margin|gap/.test(lower)) return '1rem';
if (/shadow/.test(lower)) return '0 1px 2px rgba(0,0,0,.1)';
if (/weight/.test(lower)) return '400';
if (/width|height/.test(lower)) return 'auto';
if (/opacity/.test(lower)) return '1';
return '#value';
}

// ─── Main Entry Point ────────────────────────────────────────────────────────

/**
Expand Down
58 changes: 29 additions & 29 deletions tests/__fixtures__/benchmark-results/latest-benchmark.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"timestamp": "2026-03-21T06:20:17.944Z",
"timestamp": "2026-03-21T06:42:44.161Z",
"scorecards": {
"material": {
"library": "material",
Expand Down Expand Up @@ -187,7 +187,7 @@
"heuristic": 426,
"untested": 747
},
"timestamp": "2026-03-21T06:20:17.942Z"
"timestamp": "2026-03-21T06:42:44.159Z"
},
"spectrum": {
"library": "spectrum",
Expand Down Expand Up @@ -375,7 +375,7 @@
"heuristic": 404,
"untested": 601
},
"timestamp": "2026-03-21T06:20:17.942Z"
"timestamp": "2026-03-21T06:42:44.159Z"
},
"vaadin": {
"library": "vaadin",
Expand Down Expand Up @@ -563,7 +563,7 @@
"heuristic": 439,
"untested": 717
},
"timestamp": "2026-03-21T06:20:17.942Z"
"timestamp": "2026-03-21T06:42:44.159Z"
},
"fluentui": {
"library": "fluentui",
Expand Down Expand Up @@ -751,7 +751,7 @@
"heuristic": 139,
"untested": 293
},
"timestamp": "2026-03-21T06:20:17.942Z"
"timestamp": "2026-03-21T06:42:44.159Z"
},
"carbon": {
"library": "carbon",
Expand Down Expand Up @@ -939,7 +939,7 @@
"heuristic": 397,
"untested": 793
},
"timestamp": "2026-03-21T06:20:17.942Z"
"timestamp": "2026-03-21T06:42:44.159Z"
},
"ui5": {
"library": "ui5",
Expand Down Expand Up @@ -1127,7 +1127,7 @@
"heuristic": 582,
"untested": 1304
},
"timestamp": "2026-03-21T06:20:17.943Z"
"timestamp": "2026-03-21T06:42:44.160Z"
},
"calcite": {
"library": "calcite",
Expand Down Expand Up @@ -1315,7 +1315,7 @@
"heuristic": 318,
"untested": 848
},
"timestamp": "2026-03-21T06:20:17.943Z"
"timestamp": "2026-03-21T06:42:44.160Z"
},
"porsche": {
"library": "porsche",
Expand Down Expand Up @@ -1503,7 +1503,7 @@
"heuristic": 304,
"untested": 702
},
"timestamp": "2026-03-21T06:20:17.943Z"
"timestamp": "2026-03-21T06:42:44.160Z"
},
"ionic": {
"library": "ionic",
Expand Down Expand Up @@ -1691,7 +1691,7 @@
"heuristic": 331,
"untested": 744
},
"timestamp": "2026-03-21T06:20:17.944Z"
"timestamp": "2026-03-21T06:42:44.161Z"
},
"wired": {
"library": "wired",
Expand Down Expand Up @@ -1879,7 +1879,7 @@
"heuristic": 78,
"untested": 208
},
"timestamp": "2026-03-21T06:20:17.944Z"
"timestamp": "2026-03-21T06:42:44.161Z"
},
"elix": {
"library": "elix",
Expand Down Expand Up @@ -2067,7 +2067,7 @@
"heuristic": 148,
"untested": 740
},
"timestamp": "2026-03-21T06:20:17.944Z"
"timestamp": "2026-03-21T06:42:44.161Z"
},
"helix": {
"library": "helix",
Expand Down Expand Up @@ -2255,7 +2255,7 @@
"heuristic": 440,
"untested": 555
},
"timestamp": "2026-03-21T06:20:17.944Z"
"timestamp": "2026-03-21T06:42:44.161Z"
}
},
"comparisonTable": {
Expand Down Expand Up @@ -3278,66 +3278,66 @@
"API Surface Quality",
"Event Architecture"
],
"timestamp": "2026-03-21T06:20:17.944Z"
"timestamp": "2026-03-21T06:42:44.161Z"
},
"performance": {
"totalMs": 2907,
"totalMs": 2262,
"phases": [
{
"name": "load-libraries",
"durationMs": 131
"durationMs": 168
},
{
"name": "score-all-libraries",
"durationMs": 2773
"durationMs": 2091
},
{
"name": "score-material",
"durationMs": 263
"durationMs": 247
},
{
"name": "score-spectrum",
"durationMs": 153
"durationMs": 105
},
{
"name": "score-vaadin",
"durationMs": 861
"durationMs": 640
},
{
"name": "score-fluentui",
"durationMs": 74
"durationMs": 34
},
{
"name": "score-carbon",
"durationMs": 185
"durationMs": 139
},
{
"name": "score-ui5",
"durationMs": 301
"durationMs": 192
},
{
"name": "score-calcite",
"durationMs": 232
"durationMs": 262
},
{
"name": "score-porsche",
"durationMs": 190
"durationMs": 74
},
{
"name": "score-ionic",
"durationMs": 125
"durationMs": 68
},
{
"name": "score-wired",
"durationMs": 34
"durationMs": 20
},
{
"name": "score-elix",
"durationMs": 295
"durationMs": 169
},
{
"name": "score-helix",
"durationMs": 60
"durationMs": 141
},
{
"name": "generate-scorecards",
Expand Down
59 changes: 56 additions & 3 deletions tests/handlers/styling-diagnostics.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,19 @@ describe('buildAntiPatterns', () => {
);
expect(partWarning).toBeUndefined();
});

it('includes font inheritance warning when slots exist', () => {
const warnings = buildAntiPatterns(buttonMeta);
const inheritWarning = warnings.find((w) => w.explanation.includes('inherit'));
expect(inheritWarning).toBeDefined();
expect(inheritWarning!.explanation).toContain('font');
});

it('skips font inheritance warning when no slots', () => {
const warnings = buildAntiPatterns(bareComponent);
const inheritWarning = warnings.find((w) => w.explanation.includes('inherit'));
expect(inheritWarning).toBeUndefined();
});
});

describe('buildCssSnippet', () => {
Expand All @@ -224,12 +237,55 @@ describe('buildCssSnippet', () => {
expect(snippet).toContain('--my-button-bg');
});

it('does NOT generate self-referential var() in token customization', () => {
const snippet = buildCssSnippet(buttonMeta);
// Should NOT contain --my-button-bg: var(--my-button-bg) — that's circular
expect(snippet).not.toMatch(/--my-button-bg:\s*var\(--my-button-bg\)/);
// Should contain a direct value assignment
expect(snippet).toMatch(/--my-button-bg:\s*[^v]/);
});

it('uses CEM default value when available', () => {
const metaWithDefaults: ComponentMetadata = {
...bareComponent,
cssProperties: [
{ name: '--my-card-bg', description: 'Background', default: '#ffffff' },
{ name: '--my-card-radius', description: 'Radius', default: '4px' },
],
};
const snippet = buildCssSnippet(metaWithDefaults);
expect(snippet).toContain('#ffffff');
expect(snippet).toContain('4px');
});

it('includes part customization for components with CSS parts', () => {
const snippet = buildCssSnippet(buttonMeta);
expect(snippet).toContain('Part-based customization');
expect(snippet).toContain('::part(base)');
});

it('includes slot styling section when component has slots', () => {
const snippet = buildCssSnippet(buttonMeta);
expect(snippet).toContain('Slot styling');
expect(snippet).toContain('light DOM');
});

it('includes named slot selector example', () => {
const snippet = buildCssSnippet(buttonMeta);
expect(snippet).toContain('[slot="prefix"]');
});

it('includes default slot child selector example', () => {
const snippet = buildCssSnippet(buttonMeta);
// Default slot: style children of the host
expect(snippet).toContain('my-button >');
});

it('skips slot section for components without slots', () => {
const snippet = buildCssSnippet(bareComponent);
expect(snippet).not.toContain('Slot styling');
});

it('generates fallback message for bare components', () => {
const snippet = buildCssSnippet(bareComponent);
expect(snippet).toContain('no CSS customization');
Expand All @@ -248,9 +304,6 @@ describe('buildCssSnippet', () => {
})),
};
const snippet = buildCssSnippet(manyProps);
// Should have at most 5 properties listed (each appears twice: decl + var)
const propMatches = snippet.match(/--prop-/g);
expect(propMatches!.length).toBeLessThanOrEqual(10);
// Should not contain --prop-5 through --prop-9
expect(snippet).not.toContain('--prop-5');
// Should have at most 3 parts listed
Expand Down
Loading