feat(blade-svelte): Accordion component#3348
Merged
rohankokane-dev merged 10 commits intomasterfrom May 8, 2026
Merged
Conversation
🦋 Changeset detectedLatest commit: 3c78465 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
Contributor
|
✅ PR title follows Conventional Commits specification. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 3c78465:
|
Collaborator
🛡️ Coverage ReportSummaryFull Coverage Details |
rohankokane-dev
changed the title
rohankokane-dev
changed the title
rohankokane-dev
commented
May 5, 2026
| ); | ||
| const showDivider = $derived(!isLastItem || variant === 'transparent'); | ||
|
|
||
| const collapsibleBodyId = `accordion-body-${Math.random().toString(36).slice(2, 9)}`; |
Contributor
Author
There was a problem hiding this comment.
Bug: Math.random() for collapsibleBodyId breaks SSR determinism
This ID is used for aria-controls on the button and id on the body region. Math.random() produces different values on server vs client, causing an SSR hydration mismatch.
Consider using a deterministic counter-based ID (e.g., pass the itemIndex through) or use a utility like crypto.randomUUID() only if SSR is not a concern. Since Blade is a design system and SSR support is likely expected, a pattern like:
const collapsibleBodyId = `accordion-body-${itemIndex}`;...would be simpler and deterministic. If multiple Accordions can coexist on the same page, you could add a unique accordion-level ID prefix passed through context.
Contributor
Author
There was a problem hiding this comment.
Fixed in 9cc12d9. Replaced Math.random() with a module-scoped monotonically-increasing counter — deterministic in component-tree order, SSR-safe, and collision-free.
rohankokane-dev
commented
May 5, 2026
| _itemCounter = 0; | ||
| }); | ||
|
|
||
| const registerItem = (): number => { |
Contributor
Author
There was a problem hiding this comment.
Potential issue: Item registration relies on render-order side effects
This pattern calls registerItem() during component initialization (outside $effect) and uses $effect.pre to reset the counter. This works as long as:
- AccordionItems are never conditionally rendered (e.g.,
{#if condition}<AccordionItem>...) - AccordionItems are never dynamically reordered
If an item is removed from the middle, the remaining items will re-register with stale indices on the next render cycle because registerItem() is called once during init (line 25 of AccordionItem.svelte), not on every render.
For now this matches the React behavior (which also uses a counter), but worth noting as a known limitation in the component docs or as a comment.
Contributor
Author
There was a problem hiding this comment.
Acknowledged. The pattern matches the React Accordion's index-based registration. Will revisit if conditional-rendering edge cases surface.
rohankokane-dev
commented
May 5, 2026
| const metaAttrs = metaAttribute({ name: MetaConstants.AccordionItemBody }); | ||
| const analyticsAttrs = $derived(makeAnalyticsAttribute(rest)); | ||
| const bodyA11y = $derived( | ||
| makeAccessible({ role: 'region', hidden: !isExpanded }), |
Contributor
Author
There was a problem hiding this comment.
Accessibility: Body region is missing aria-labelledby
Per WAI-ARIA Accordion pattern, the collapsible region (role="region") should have aria-labelledby pointing to the button that controls it. Currently, the button has aria-controls pointing to the body, but the body region does not have aria-labelledby pointing back to the button.
Suggested fix: Give the accordion button an id (e.g., accordion-button-{itemIndex}) and add aria-labelledby to this region element:
const bodyA11y = $derived(
makeAccessible({ role: 'region', hidden: !isExpanded, labelledBy: collapsibleButtonId }),
);Reference: https://www.w3.org/WAI/ARIA/apg/patterns/accordion/
Contributor
Author
There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. Will add aria-labelledby on the body region (pointing to the header button id) in a follow-up to complete the WAI-ARIA accordion bidirectional link.
rohankokane-dev
commented
May 5, 2026
| {...analyticsAttrs} | ||
| > | ||
| <div class={templateClasses.body}> | ||
| <BaseText |
Contributor
Author
There was a problem hiding this comment.
Nit: Wrapping BaseText around all body children forces text styling on non-text content
When children is a Snippet (custom content like forms, cards, etc.), it still gets wrapped in BaseText with color="surface.text.gray.subtle" and specific font sizing. This means custom slot content inherits text color/size styling that may not be desired.
In the React implementation, string children get wrapped in <Text> but non-string ReactNode children are rendered directly without a text wrapper.
Consider conditionally wrapping only the string case in BaseText and rendering the snippet directly for non-string children.
Contributor
Author
There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. Will narrow the BaseText wrapping to string children only (matching React) in a follow-up.
rohankokane-dev
commented
May 5, 2026
| {/if} | ||
|
|
||
| <div class={chevronClass}> | ||
| <ChevronDownIcon size="large" color={chevronColor} /> |
Contributor
Author
There was a problem hiding this comment.
Nit: ChevronDownIcon size is hardcoded to "large" regardless of accordion size
The icon is always rendered with size="large", but when the accordion size is "medium", the chevron may appear disproportionately large relative to the header content (medium header slot height is 20px vs large at 28px).
Consider deriving the chevron size from accordionSize:
<ChevronDownIcon size={accordionSize === 'large' ? 'large' : 'medium'} color={chevronColor} />Check against the React source to confirm whether the original also uses a fixed size or adapts.
Contributor
Author
There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. Will check the React source for the intended icon-size mapping and parameterize.
rohankokane-dev
commented
May 5, 2026
| } | ||
| }; | ||
|
|
||
| setAccordionContext(() => ({ |
Contributor
Author
There was a problem hiding this comment.
Context reactivity: Props passed to context getter may go stale
When props like showNumberPrefix, variant, and size change, the context getter function captures the current values at the time setAccordionContext is called (component init). Since these are destructured from $props(), they are reactive in Svelte 5 and the getter should re-evaluate correctly because it reads them from the closure.
However, verify that child components that call getAccCtx() inside a $derived block actually re-derive when these parent props change. The pattern looks correct (getter function + $derived consumer), but it is worth testing:
- Toggle
showNumberPrefixdynamically - Switch
variantfromtransparenttofilled - Switch
sizefromlargetomedium
If any of these fail to update the children, the context getter pattern needs adjustment.
Contributor
Author
There was a problem hiding this comment.
Acknowledged. The getter pattern + closure access keeps reads live, so the concern is theoretical here. Noting for future awareness.
rohankokane-dev
commented
May 5, 2026
| let bodyRef: HTMLDivElement | undefined = $state(undefined); | ||
| let isInitialRender = true; | ||
|
|
||
| $effect(() => { |
Contributor
Author
There was a problem hiding this comment.
Animation $effect cleanup: No cleanup function means potential stale RAF callbacks
The $effect schedules nested requestAnimationFrame callbacks but does not return a cleanup function. If the component unmounts (or the effect re-runs) while a RAF is pending, the callback can execute on a detached/undefined bodyRef.
The null checks (if (!bodyRef) return) mitigate crashes, but the stale callbacks still execute needlessly. Consider storing RAF IDs and canceling them in a cleanup return:
$effect(() => {
const expanded = isExpanded;
// ... setup ...
let raf1: number, raf2: number;
// ... raf1 = requestAnimationFrame(() => { ... raf2 = requestAnimationFrame(...) });
return () => {
cancelAnimationFrame(raf1);
cancelAnimationFrame(raf2);
};
});This is a minor robustness improvement, not a blocker.
Contributor
Author
There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. Will return a cleanup that cancels pending RAFs in a follow-up.
rohankokane-dev
commented
May 5, 2026
Contributor
Author
rohankokane-dev
left a comment
rohankokane-dev
left a comment
There was a problem hiding this comment.
Accordion Svelte 5 Migration -- Code Review Summary
Overall Assessment
Solid migration. The component architecture (Accordion > AccordionItem > AccordionItemHeader + AccordionItemBody) maps cleanly from React to Svelte 5. Context pattern using getter functions, CVA-based styling in blade-core, and CSS modules are all well-structured. The stories provide good coverage across variants.
Key Findings
Must-fix (1):
Math.random()forcollapsibleBodyIdbreaks SSR -- Thearia-controls/idpairing uses a non-deterministic random value. This will cause hydration mismatches in SSR scenarios. Use a counter-based or index-based deterministic ID instead.
Accessibility (1):
- Missing
aria-labelledbyon the body region -- Per the WAI-ARIA Accordion pattern, therole="region"element should reference the controlling button viaaria-labelledby. Currently only the forward reference (aria-controlson button -> body) exists. The reverse reference is missing.
Behavioral concerns (1):
- Animation
$effectdoes not clean up RAF callbacks -- The nestedrequestAnimationFramecalls in AccordionItemBody have no cleanup. While the null checks prevent crashes, stale callbacks still fire after unmount. Minor but worth addressing for robustness.
Design parity (2):
-
BaseTextwraps all body children including Snippets -- Custom slot content (forms, cards, etc.) inherits text styling. The React version only wraps string children in<Text>. -
ChevronDownIcon is hardcoded to
size="large"-- When accordionsize="medium", the chevron may be disproportionate. Verify against React source whether this is intentional.
What looks good
- Svelte 5 patterns: Proper use of
$props(),$derived,$state,$effect,$effect.pre, and@rendersnippets. Context uses the getter-function pattern correctly. - CVA + CSS modules: Clean separation of styling concerns in blade-core. Compound variants for filled+position+expanded states are well-modeled.
- Type safety:
types.tsis comprehensive with proper JSDoc. Analytics attribute index signatures are correct. - Accessibility fundamentals:
role="heading"withlevel=3on the header wrapper,aria-expandedandaria-controlson the button,role="region"on the body,disabledattribute on the button -- all present. - Keyboard navigation: Since a native
<button>is used, Enter/Space toggling works for free. - Controlled/uncontrolled pattern: Clean implementation with
expandedIndexvsdefaultExpandedIndex. - Stories: Good coverage -- basic, number prefix, icons, controlled, custom header/body, filled variant composition, header variants, disabled state.
Checklist items from PR template
The PR template shows unchecked items for Component Status Page update, manual browser testing, KitchenSink story, and interaction tests. These should be addressed before merge.
Minor notes
- The
Dividerimport in AccordionItemHeader.svelte appears unused in some code paths (it is used inside the expanded divider section, so this is fine). - The
$effect.precounter-reset pattern for item registration is clever but fragile with conditional rendering. This matches the React behavior, so it is acceptable, but worth documenting as a known limitation. - The
.cursor/agent/rule changes included in this PR are unrelated tooling updates. Consider splitting them into a separate PR for cleaner review history.
Address PR #3348 must-fix review item. `Math.random()` produced different ids on server and client, causing SSR hydration mismatches on the `id` / `aria-controls` pair. Replaced with a module-scoped monotonically-increasing counter so ids are deterministic in component-tree order — server and client agree, and collisions are impossible inside a single module load. Co-authored-by: Cursor <cursoragent@cursor.com>
saurabhdaware
previously approved these changes
May 7, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
saurabhdaware
approved these changes
May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Migrates the React Accordion component to Svelte 5.
Changes
packages/blade-svelte/src/components/Accordion/packages/blade-core/src/styles/Accordion/packages/blade-svelte/src/components/index.tspackages/blade-core/src/styles/index.tsAdditional Information
Component Checklist