fix(accordion): remove stale summary button refs on panel removal

[zigzagdev]

Fixes #

Additional description

Motivation:

• Accordion leaked stale summaryButtons refs on panel removal — a detached DOM node from a removed panel stayed in the array, so keyboard navigation could focus it instead of a remaining panel.

• This was already reported and fixed in #3193, but that PR was closed during the recent React 19 / TypeScript rewrite (#3195):

  We rewrote much of the library recently. I'm going to close this PR, but please re-open if there is still an issue.

• I reverified against the rewritten components/accordion/index.tsx and confirmed the bug is still happen, so reopening this fix here.

Root cause: addSummaryButton only ever pushed refs into summaryButtonsRef.current and de-duped by identity (!summaryButtonsRef.current.includes(button)). It never handled the null callback React fires when a panel's button unmounts, so a detached DOM node from a removed panel stayed in the array. Keyboard navigation (ArrowUp/ArrowDown) could then focus that stale detached node instead of the correct remaining panel.

What I did: Keyed summaryButtonsRef.current by the child's render index instead of push/identity, and clear the slot (delete summaryButtonsRef.current[index]) when its ref callback fires with null on unmount. React.Children.map's index is now passed through to addSummaryButton per child.

---

CONTRIBUTOR checklist (do not remove)

Please complete for every pull request.

Green gate — all of these must pass locally (CI runs the same):

• npm run lint passes with 0 errors (npm run lint:fix to auto-fix). Warnings are the tracked gradual-migration backlog; do not add new errors.
• npm run typecheck is clean (tsc --noEmit).
• npm test -- --run passes — this runs both Vitest projects (jsdom unit + real-browser browser) and the story HTML snapshots. If you changed rendered markup on purpose, update snapshots with npm test -- --run -u and review the diff.
• npm run build succeeds (type-check + Vite library build + .d.ts + entry shims).

Behavior & docs:

• First-time contributors: sign the CLA (a bot will comment with instructions).
• Added/updated Vitest + React Testing Library tests for the change (template: components/button/__tests__). New props and bug fixes should get a regression test. Tests that genuinely need a real browser (focus traversal, layout/measurement, canvas) go in a *.browser.test.jsx file (the browser project); everything else is jsdom.
  • Added focuses the correct remaining button after a panel is removed to components/accordion/__tests__/accordion.test.jsx (jsdom unit project — the fix's .focus() call is triggered by component state, not real browser tab traversal, so jsdom is sufficient here). Verified it fails against the pre-fix code (focus lands on the stale removed panel's button) and passes after the fix.
• Reviewed the component's Storybook stories: npm run storybook → http://localhost:6007/. Confirm the story actually renders and behaves (deprecated Storyshots/Karma are gone; there is no separate browser-tests server).
• Story/example code uses the component's real prop API (e.g. render-props like onRenderContent / onRenderVisualPicker, icon props, events), not assumptions — a common source of "component looks broken" reports has been stories authored against the wrong API. Cross-check against components/<name>/__examples__.
• Storybook Accessibility (a11y) panel shows no new violations for the affected stories.
• If you added or changed a type, remember runtime PropTypes were removed in the TS migration — types are the contract. Ship accurate .d.ts (they're generated by the build); JS consumers rely on them via editor type-acquisition.

REVIEWER checklist (do not remove)

• CI is green (GitHub Actions CI workflow: lint, typecheck, test, build). Chromatic visual review, if configured, has been checked.
• Change has adequate test coverage; bug fixes include a regression test.
• Reviewed the affected Storybook stories and their a11y panels.
• Rendered markup still conforms to SLDS — check the story HTML snapshot diff (components/__tests__/__snapshots__/story-snapshots.test.jsx.snap).
• Public API / package surface: if exports changed, the deep-import contract still holds (@salesforce/design-system-react/components/<name> and nested paths resolve for ESM, CJS, and types). Removing/renaming a public component or export is a breaking change → feat!:/BREAKING CHANGE: and a major version bump.

[showerbee]

Note: this review was assisted by an AI coding agent — I ran it and vetted the findings below.

Fix works for the reported bug, but I think it's only half the fix.

Confirmed: button refs no longer accumulate stale entries when a panel unmounts — traced the old push-based addSummaryButton against the new regression test, fails on old code / passes on new code.

Not addressed — same root cause: currButtonIndex (index.tsx:61) never resyncs when the child list shrinks or reorders. Repro: 4 panels, focus at index 3, panel 0 gets removed — items shift but currButtonIndex stays 3, now pointing at the wrong button. This is the same stale-index-after-removal bug we just fixed for combobox pills (#3211/0912e29e0, 7efc644, 5a56bee). Shipping this without clamping/resyncing the index feels like we'll be back here in a month with a near-identical report.

Smaller things, non-blocking:

• index.tsx:88-97 — index math assumes every positional slot maps to a real Panel; conditionally-rendered children (cond && <Panel/>) will leave a permanently-undefined ref at that index and arrow-key nav stalls there.
• index.tsx:91 — delete summaryButtonsRef.current[index] leaves array holes; a Map<number, HTMLButtonElement> would be more explicit and avoid .length overstating the real count.
• index.tsx:88-97, 117-118 — useCallback on addSummaryButton buys nothing since it's invoked via a new inline arrow per child on every render; every re-render detaches/reattaches all button refs.

Would like to see currButtonIndex resync handled before merge — the rest can be follow-up.

[zigzagdev]

@showerbee

Thanks for the comment.

I reproduced the stale currButtonIndex issue with a dedicated regression test before touching any code, and it matched your description exactly.
Four panels, focus on the last one, removing the first panel left the tracked index pointing past the correct position, so the next ArrowUp press became a no-op instead of moving focus.

I fixed that first case in dd4ebeaf1, clamping currButtonIndex when childCount changes.
I adjusted it during render rather than in an effect, since doing it in an effect triggered the react-hooks/set-state-in-effect lint rule.
The regression test is in 7cf9cfd44, it fails on the old code and passes on the new code.

That clamp only catches the case where the tracked index ends up past the end of the list.
Removing a panel before the focused one can also shift the focused button to an earlier position while the index stays in range but now points at the wrong button.
I fixed that case in 4f44e01eb, resyncing currButtonIndex to wherever document.activeElement actually is among the tracked buttons, falling back to the childCount clamp when nothing is focused.
The regression test is in 6bbd262c4.

I left the three non-blocking items you listed as follow-up, since you mentioned they could be handled separately.