Skip to content

Commit 5c62124

Browse files
committed
refactor(ui): migrates accordion to Kobalte, fixes scroll/a11y
- Replaces custom accordion with @kobalte/core Accordion (aria, keyboard nav, heading semantics via h3) - Extracts OrgContent sub-component for SolidJS reactivity isolation - Fixes FLIP animation scroll preservation (reads-before-writes pattern) - Fixes accordion default expansion mid-load shift (stable initial value) - Uses unfiltered selectedCount in accordion badge - Adds fill-mode: forwards to accordion CSS animation - Adds 8 new tests (1580 total): FLIP rAF callback with call-order assertion, accordion threshold transitions, loading/error states, selectedCount filter invariant
1 parent 3e77d31 commit 5c62124

File tree

6 files changed

+604
-286
lines changed

6 files changed

+604
-286
lines changed

src/app/components/onboarding/RepoSelector.tsx

Lines changed: 211 additions & 204 deletions
Large diffs are not rendered by default.

src/app/index.css

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,25 @@
136136
scrollbar-color: color-mix(in oklch, var(--color-base-content) 30%, transparent) var(--color-base-200);
137137
}
138138

139+
/* ── Kobalte accordion animation ─────────────────────────────────────────── */
140+
141+
.kb-accordion-content {
142+
overflow: hidden;
143+
animation: kb-accordion-down 200ms ease-in-out forwards;
144+
}
145+
.kb-accordion-content[data-closed] {
146+
animation: kb-accordion-up 200ms ease-in-out forwards;
147+
}
148+
149+
@keyframes kb-accordion-down {
150+
from { height: 0; }
151+
to { height: var(--kb-accordion-content-height); }
152+
}
153+
@keyframes kb-accordion-up {
154+
from { height: var(--kb-accordion-content-height); }
155+
to { height: 0; }
156+
}
157+
139158
@media (prefers-reduced-motion: reduce) {
140159
.animate-slow-pulse,
141160
.animate-toast-in, .animate-toast-out,
@@ -145,7 +164,8 @@
145164
.loading {
146165
animation: none;
147166
}
148-
.accordion-panel {
149-
transition: none;
167+
.kb-accordion-content,
168+
.kb-accordion-content[data-closed] {
169+
animation: none;
150170
}
151171
}

src/app/lib/scroll.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@
55
*/
66
export function withScrollLock(fn: () => void): void {
77
const y = window.scrollY;
8-
try { fn(); } finally { window.scrollTo(0, y); }
8+
try {
9+
fn();
10+
} finally {
11+
window.scrollTo(0, y);
12+
}
913
}
1014

1115
/**
@@ -34,18 +38,29 @@ export function withFlipAnimation(fn: () => void): void {
3438
}
3539

3640
// Execute state change (SolidJS updates DOM synchronously)
41+
const scrollY = window.scrollY;
3742
fn();
3843

39-
// Last, Invert, Play
44+
// Last, Invert, Play — reads before writes to avoid layout thrash.
45+
// All getBoundingClientRect calls happen before scrollTo so the browser
46+
// doesn't force a synchronous layout recalculation between them.
4047
requestAnimationFrame(() => {
4148
const afterItems = document.querySelectorAll<HTMLElement>("[data-repo-group]");
49+
const scrollDrift = window.scrollY - scrollY;
50+
const deltas: { el: HTMLElement; dy: number }[] = [];
4251
for (const el of afterItems) {
4352
const key = el.dataset.repoGroup!;
4453
const old = before.get(key);
4554
if (!old) continue;
4655
const now = el.getBoundingClientRect();
47-
const dy = old.top - now.top;
56+
// Adjust for scroll drift: old.top was measured at scrollY,
57+
// now.top is measured at the current (possibly drifted) scroll position.
58+
const dy = old.top - now.top - scrollDrift;
4859
if (Math.abs(dy) < 1) continue;
60+
deltas.push({ el, dy });
61+
}
62+
window.scrollTo(0, scrollY);
63+
for (const { el, dy } of deltas) {
4964
el.animate(
5065
[{ transform: `translateY(${dy}px)` }, { transform: "translateY(0)" }],
5166
{ duration: 200, easing: "ease-in-out" },

tests/components/DashboardPage.test.tsx

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -682,6 +682,17 @@ describe("DashboardPage — onAuthCleared integration", () => {
682682
});
683683

684684
describe("DashboardPage — scroll preservation on poll refresh", () => {
685+
// MOCK INVARIANT: fetchAllData is mocked via vi.fn() and never calls its
686+
// onLightData callback, so phaseOneFired is always false inside pollFetch().
687+
// This means every poll cycle takes the withScrollLock branch (not the
688+
// fine-grained produce() path). If fetchAllData is ever changed to invoke
689+
// onLightData in tests, phaseOneFired will become true and withScrollLock
690+
// will NOT be called, silently breaking this test.
691+
//
692+
// window.scrollTo is the correct behavioral proxy for withScrollLock:
693+
// withScrollLock captures scrollY then calls window.scrollTo(0, y) after
694+
// the setter. Asserting scrollTo was called with the saved position is
695+
// equivalent to asserting withScrollLock ran and completed successfully.
685696
it("preserves scroll position when setDashboardData replaces arrays", async () => {
686697
const issues = [makeIssue({ id: 1, title: "Scroll test issue" })];
687698
vi.mocked(pollService.fetchAllData).mockResolvedValue({
@@ -700,11 +711,15 @@ describe("DashboardPage — scroll preservation on poll refresh", () => {
700711
document.documentElement.scrollTop = 500;
701712
vi.spyOn(window, "scrollTo");
702713

703-
// Trigger a second poll (subsequent refresh — the path that uses withScrollLock)
714+
// Trigger a second poll (subsequent refresh — the path that uses withScrollLock).
715+
// phaseOneFired is false (mock never calls onLightData), so withScrollLock
716+
// wraps the full atomic setDashboardData replacement and restores scroll.
704717
if (capturedFetchAll) {
705718
await capturedFetchAll();
706719
}
707720

721+
// window.scrollTo(0, 500) is the observable side-effect of withScrollLock
722+
// saving and restoring the pre-update scroll position.
708723
expect(window.scrollTo).toHaveBeenCalledWith(0, 500);
709724
vi.restoreAllMocks();
710725
document.documentElement.scrollTop = 0;

0 commit comments

Comments
 (0)