Skip to content

Commit 534a7b4

Browse files
authored
feat(ui): scroll lock fix + org-grouped accordion for 6+ orgs (#54)
* fix(ui): preserves scroll position on repo lock/move/unlock * feat(ui): adds org-grouped accordion to repo picker for 6+ orgs * feat(ui): FLIP animation, poll scroll fix, and accordion UX improvements - extracts withScrollLock to shared src/app/lib/scroll.ts with try/finally - adds withFlipAnimation (200ms ease-in-out, reduced-motion fallback) - replaces instant repo pin/move with FLIP animation via data-repo-group attrs - preserves scroll position on timed poll refresh in DashboardPage - refactors accordion: single bordered unit, inline Select/Deselect in header bar, loading spinner on collapsed headers, orgId sanitization for DOM IDs - refactors orgContent closure (removes isAccordion check), Show+fallback pattern - adds 12 new tests (1573 total, 71 files, typecheck clean) * 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 31df2d1 commit 534a7b4

12 files changed

Lines changed: 1314 additions & 160 deletions

File tree

src/app/components/dashboard/ActionsTab.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,7 @@ export default function ActionsTab(props: ActionsTabProps) {
293293
});
294294

295295
return (
296-
<div class="bg-base-100">
296+
<div class="bg-base-100" data-repo-group={repoGroup.repoFullName}>
297297
{/* Repo header */}
298298
<div class={`group/repo-header flex items-center bg-base-200/60 border-y border-base-300 hover:bg-base-200 transition-colors duration-300 ${highlightedReposActions().has(repoGroup.repoFullName) ? "animate-reorder-highlight" : ""}`}>
299299
<button

src/app/components/dashboard/DashboardPage.tsx

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import { pushNotification } from "../../lib/errors";
2727
import { getClient, getGraphqlRateLimit } from "../../services/github";
2828
import { formatCount } from "../../lib/format";
2929
import { setsEqual } from "../../lib/collections";
30+
import { withScrollLock } from "../../lib/scroll";
3031
import { Tooltip } from "../shared/Tooltip";
3132

3233
const globalSortOptions: SortOption[] = [
@@ -192,13 +193,17 @@ async function pollFetch(): Promise<DashboardData> {
192193
} else {
193194
// Phase 1 did NOT fire (cached data existed or subsequent poll).
194195
// Full atomic replacement — all fields (light + heavy) may have
195-
// changed since the last cycle.
196-
setDashboardData({
197-
issues: data.issues,
198-
pullRequests: data.pullRequests,
199-
workflowRuns: data.workflowRuns,
200-
loading: false,
201-
lastRefreshedAt: now,
196+
// changed since the last cycle. Preserve scroll position: SolidJS
197+
// DOM updates are synchronous within the setter, so save/restore
198+
// around it to prevent scroll reset from <For> DOM rebuild.
199+
withScrollLock(() => {
200+
setDashboardData({
201+
issues: data.issues,
202+
pullRequests: data.pullRequests,
203+
workflowRuns: data.workflowRuns,
204+
loading: false,
205+
lastRefreshedAt: now,
206+
});
202207
});
203208
}
204209
rebuildHotSets(data);

src/app/components/dashboard/IssuesTab.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -349,7 +349,7 @@ export default function IssuesTab(props: IssuesTabProps) {
349349
});
350350

351351
return (
352-
<div class="bg-base-100">
352+
<div class="bg-base-100" data-repo-group={repoGroup.repoFullName}>
353353
<div class={`group/repo-header flex items-center bg-base-200/60 border-y border-base-300 hover:bg-base-200 transition-colors duration-300 ${highlightedReposIssues().has(repoGroup.repoFullName) ? "animate-reorder-highlight" : ""}`}>
354354
<button
355355
onClick={() => toggleExpandedRepo("issues", repoGroup.repoFullName)}

src/app/components/dashboard/PullRequestsTab.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,7 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
456456
});
457457

458458
return (
459-
<div class="bg-base-100">
459+
<div class="bg-base-100" data-repo-group={repoGroup.repoFullName}>
460460
<div class={`group/repo-header flex items-center bg-base-200/60 border-y border-base-300 hover:bg-base-200 transition-colors duration-300 ${highlightedReposPRs().has(repoGroup.repoFullName) ? "animate-reorder-highlight" : ""}`}>
461461
<button
462462
onClick={() => toggleExpandedRepo("pullRequests", repoGroup.repoFullName)}

src/app/components/onboarding/RepoSelector.tsx

Lines changed: 248 additions & 140 deletions
Large diffs are not rendered by default.

src/app/components/shared/RepoLockControls.tsx

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { Show, createMemo } from "solid-js";
22
import { viewState, lockRepo, unlockRepo, moveLockedRepo, type LockedReposTab } from "../../stores/view";
33
import { Tooltip } from "./Tooltip";
4+
import { withFlipAnimation } from "../../lib/scroll";
45

56
interface RepoLockControlsProps {
67
tab: LockedReposTab;
@@ -26,7 +27,7 @@ export default function RepoLockControls(props: RepoLockControlsProps) {
2627
<Tooltip content="Pin to top">
2728
<button
2829
class="btn btn-ghost btn-xs opacity-0 group-hover/repo-header:opacity-100 focus:opacity-100 max-sm:opacity-60 sm:max-lg:opacity-60 transition-opacity"
29-
onClick={() => lockRepo(props.tab, props.repoFullName)}
30+
onClick={() => withFlipAnimation(() => lockRepo(props.tab, props.repoFullName))}
3031
aria-label={`Pin ${props.repoFullName} to top of list`}
3132
>
3233
{/* Heroicons 20px solid: lock-open */}
@@ -40,7 +41,7 @@ export default function RepoLockControls(props: RepoLockControlsProps) {
4041
<Tooltip content="Unpin">
4142
<button
4243
class="btn btn-ghost btn-xs"
43-
onClick={() => unlockRepo(props.tab, props.repoFullName)}
44+
onClick={() => withFlipAnimation(() => unlockRepo(props.tab, props.repoFullName))}
4445
aria-label={`Unpin ${props.repoFullName}`}
4546
>
4647
{/* Heroicons 20px solid: lock-closed */}
@@ -52,7 +53,7 @@ export default function RepoLockControls(props: RepoLockControlsProps) {
5253
<Tooltip content={lockInfo().isFirst ? "Already at top of pinned list" : "Move up"}>
5354
<button
5455
class="btn btn-ghost btn-xs"
55-
onClick={() => moveLockedRepo(props.tab, props.repoFullName, "up")}
56+
onClick={() => withFlipAnimation(() => moveLockedRepo(props.tab, props.repoFullName, "up"))}
5657
disabled={lockInfo().isFirst}
5758
aria-label={`Move ${props.repoFullName} up`}
5859
>
@@ -65,7 +66,7 @@ export default function RepoLockControls(props: RepoLockControlsProps) {
6566
<Tooltip content={lockInfo().isLast ? "Already at bottom of pinned list" : "Move down"}>
6667
<button
6768
class="btn btn-ghost btn-xs"
68-
onClick={() => moveLockedRepo(props.tab, props.repoFullName, "down")}
69+
onClick={() => withFlipAnimation(() => moveLockedRepo(props.tab, props.repoFullName, "down"))}
6970
disabled={lockInfo().isLast}
7071
aria-label={`Move ${props.repoFullName} down`}
7172
>

src/app/index.css

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,25 @@
154154
scrollbar-color: color-mix(in oklch, var(--color-base-content) 30%, transparent) var(--color-base-200);
155155
}
156156

157+
/* ── Kobalte accordion animation ─────────────────────────────────────────── */
158+
159+
.kb-accordion-content {
160+
overflow: hidden;
161+
animation: kb-accordion-down 200ms ease-in-out forwards;
162+
}
163+
.kb-accordion-content[data-closed] {
164+
animation: kb-accordion-up 200ms ease-in-out forwards;
165+
}
166+
167+
@keyframes kb-accordion-down {
168+
from { height: 0; }
169+
to { height: var(--kb-accordion-content-height); }
170+
}
171+
@keyframes kb-accordion-up {
172+
from { height: var(--kb-accordion-content-height); }
173+
to { height: 0; }
174+
}
175+
157176
@media (prefers-reduced-motion: reduce) {
158177
.animate-slow-pulse,
159178
.animate-toast-in, .animate-toast-out,
@@ -164,4 +183,8 @@
164183
.loading {
165184
animation: none;
166185
}
186+
.kb-accordion-content,
187+
.kb-accordion-content[data-closed] {
188+
animation: none;
189+
}
167190
}

src/app/lib/scroll.ts

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
/**
2+
* Wraps a synchronous function call with scroll position preservation.
3+
* SolidJS fine-grained DOM updates complete synchronously within fn(),
4+
* so a single synchronous scrollTo is sufficient — no rAF needed.
5+
*/
6+
export function withScrollLock(fn: () => void): void {
7+
const y = window.scrollY;
8+
try {
9+
fn();
10+
} finally {
11+
window.scrollTo(0, y);
12+
}
13+
}
14+
15+
/**
16+
* FLIP animation for reorderable lists. Records positions of elements
17+
* matching `[data-repo-group]` before fn(), then animates them to their
18+
* new positions after the DOM update.
19+
*
20+
* Consistent with TrackedTab's FLIP: 200ms ease-in-out, respects
21+
* prefers-reduced-motion (falls back to withScrollLock).
22+
*/
23+
export function withFlipAnimation(fn: () => void): void {
24+
if (typeof window === "undefined") { fn(); return; }
25+
26+
// Reduced motion: fall back to instant scroll lock
27+
if (window.matchMedia("(prefers-reduced-motion: reduce)").matches) {
28+
withScrollLock(fn);
29+
return;
30+
}
31+
32+
// First: record positions of all repo group wrappers
33+
const items = document.querySelectorAll<HTMLElement>("[data-repo-group]");
34+
const before = new Map<string, DOMRect>();
35+
for (const el of items) {
36+
const key = el.dataset.repoGroup!;
37+
before.set(key, el.getBoundingClientRect());
38+
}
39+
40+
// Execute state change (SolidJS updates DOM synchronously)
41+
const scrollY = window.scrollY;
42+
fn();
43+
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.
47+
requestAnimationFrame(() => {
48+
const afterItems = document.querySelectorAll<HTMLElement>("[data-repo-group]");
49+
const scrollDrift = window.scrollY - scrollY;
50+
const deltas: { el: HTMLElement; dy: number }[] = [];
51+
for (const el of afterItems) {
52+
const key = el.dataset.repoGroup!;
53+
const old = before.get(key);
54+
if (!old) continue;
55+
const now = el.getBoundingClientRect();
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;
59+
if (Math.abs(dy) < 1) continue;
60+
deltas.push({ el, dy });
61+
}
62+
window.scrollTo(0, scrollY);
63+
for (const { el, dy } of deltas) {
64+
el.animate(
65+
[{ transform: `translateY(${dy}px)` }, { transform: "translateY(0)" }],
66+
{ duration: 200, easing: "ease-in-out" },
67+
);
68+
}
69+
});
70+
}

tests/components/DashboardPage.test.tsx

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -724,6 +724,51 @@ describe("DashboardPage — onAuthCleared integration", () => {
724724
});
725725
});
726726

727+
describe("DashboardPage — scroll preservation on poll refresh", () => {
728+
// MOCK INVARIANT: fetchAllData is mocked via vi.fn() and never calls its
729+
// onLightData callback, so phaseOneFired is always false inside pollFetch().
730+
// This means every poll cycle takes the withScrollLock branch (not the
731+
// fine-grained produce() path). If fetchAllData is ever changed to invoke
732+
// onLightData in tests, phaseOneFired will become true and withScrollLock
733+
// will NOT be called, silently breaking this test.
734+
//
735+
// window.scrollTo is the correct behavioral proxy for withScrollLock:
736+
// withScrollLock captures scrollY then calls window.scrollTo(0, y) after
737+
// the setter. Asserting scrollTo was called with the saved position is
738+
// equivalent to asserting withScrollLock ran and completed successfully.
739+
it("preserves scroll position when setDashboardData replaces arrays", async () => {
740+
const issues = [makeIssue({ id: 1, title: "Scroll test issue" })];
741+
vi.mocked(pollService.fetchAllData).mockResolvedValue({
742+
issues,
743+
pullRequests: [],
744+
workflowRuns: [],
745+
errors: [],
746+
});
747+
748+
render(() => <DashboardPage />);
749+
await waitFor(() => {
750+
screen.getByText("owner/repo");
751+
});
752+
753+
// Simulate user scrolled down
754+
document.documentElement.scrollTop = 500;
755+
vi.spyOn(window, "scrollTo");
756+
757+
// Trigger a second poll (subsequent refresh — the path that uses withScrollLock).
758+
// phaseOneFired is false (mock never calls onLightData), so withScrollLock
759+
// wraps the full atomic setDashboardData replacement and restores scroll.
760+
if (capturedFetchAll) {
761+
await capturedFetchAll();
762+
}
763+
764+
// window.scrollTo(0, 500) is the observable side-effect of withScrollLock
765+
// saving and restoring the pre-update scroll position.
766+
expect(window.scrollTo).toHaveBeenCalledWith(0, 500);
767+
vi.restoreAllMocks();
768+
document.documentElement.scrollTop = 0;
769+
});
770+
});
771+
727772
describe("DashboardPage — onHotData integration", () => {
728773
it("applies hot poll PR status updates to the store", async () => {
729774
const testPR = makePullRequest({

0 commit comments

Comments
 (0)