Skip to content

Commit 501f7b5

Browse files
wgordon17wgordon17
committed
fix(ui): addresses PR review findings across all 10 verified issues
- Extracts groupByRepo, computePageLayout, slicePageGroups to shared generic lib/grouping.ts, eliminating verbatim duplication between IssuesTab and PullRequestsTab - Wraps pageCount in createMemo, consistent with commit 7749a68 - Inlines pageSize (removes unnecessary createMemo wrapper) - Adds named isRepoCollapsed accessor in IssuesTab/PullRequestsTab, consistent with ActionsTab pattern - Changes ChevronIcon sizeClass from createMemo to plain arrow fn, consistent with ItemRow isCompact pattern - Adds aria-expanded to ActionsTab workflow-level toggle button - Formats PullRequestsTab FilterChips handlers to multi-line style - Adds unit tests for grouping boundary cases, PullRequestsTab page-reset, sort-order assertions, and ActionsTab aria-expanded tests
1 parent cdc0f60 commit 501f7b5

File tree

9 files changed

+415
-170
lines changed

9 files changed

+415
-170
lines changed

‎src/app/components/dashboard/ActionsTab.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,7 @@ export default function ActionsTab(props: ActionsTabProps) {
245245
{/* Workflow header */}
246246
<button
247247
onClick={() => toggleWorkflow(wfKey)}
248+
aria-expanded={!isWfCollapsed()}
248249
class="w-full flex items-center gap-2 px-4 py-1.5 text-left text-xs font-medium text-gray-500 dark:text-gray-400 hover:text-gray-700 dark:hover:text-gray-200 hover:bg-gray-50 dark:hover:bg-gray-800/50 transition-colors"
249250
>
250251
<ChevronIcon size="sm" rotated={isWfCollapsed()} />

‎src/app/components/dashboard/IssuesTab.tsx‎

Lines changed: 44 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import RoleBadge from "../shared/RoleBadge";
1414
import SkeletonRows from "../shared/SkeletonRows";
1515
import ChevronIcon from "../shared/ChevronIcon";
1616
import { deriveInvolvementRoles } from "../../lib/format";
17+
import { groupByRepo, computePageLayout, slicePageGroups } from "../../lib/grouping";
1718

1819
export interface IssuesTabProps {
1920
issues: Issue[];
@@ -43,57 +44,6 @@ const issueFilterGroups: FilterChipGroupDef[] = [
4344
},
4445
];
4546

46-
interface IssueRepoGroup {
47-
repoFullName: string;
48-
items: Issue[];
49-
}
50-
51-
function groupByRepo(items: Issue[]): IssueRepoGroup[] {
52-
const groups: IssueRepoGroup[] = [];
53-
const map = new Map<string, IssueRepoGroup>();
54-
for (const item of items) {
55-
let group = map.get(item.repoFullName);
56-
if (!group) {
57-
group = { repoFullName: item.repoFullName, items: [] };
58-
map.set(item.repoFullName, group);
59-
groups.push(group);
60-
}
61-
group.items.push(item);
62-
}
63-
return groups;
64-
}
65-
66-
function computePageLayout(
67-
groups: IssueRepoGroup[],
68-
approxPageSize: number,
69-
): { boundaries: number[]; pageCount: number } {
70-
if (groups.length === 0) return { boundaries: [0], pageCount: 1 };
71-
72-
const boundaries: number[] = [0];
73-
let currentPageItems = 0;
74-
for (let i = 0; i < groups.length; i++) {
75-
if (currentPageItems > 0 && currentPageItems + groups[i].items.length > approxPageSize) {
76-
boundaries.push(i);
77-
currentPageItems = 0;
78-
}
79-
currentPageItems += groups[i].items.length;
80-
}
81-
82-
return { boundaries, pageCount: Math.max(1, boundaries.length) };
83-
}
84-
85-
function slicePageGroups(
86-
groups: IssueRepoGroup[],
87-
boundaries: number[],
88-
pageCount: number,
89-
page: number,
90-
): IssueRepoGroup[] {
91-
const clampedPage = Math.max(0, Math.min(page, pageCount - 1));
92-
const start = boundaries[clampedPage];
93-
const end = clampedPage + 1 < boundaries.length ? boundaries[clampedPage + 1] : groups.length;
94-
return groups.slice(start, end);
95-
}
96-
9747
export default function IssuesTab(props: IssuesTabProps) {
9848
const [page, setPage] = createSignal(0);
9949
const [collapsedRepos, setCollapsedRepos] = createStore<Record<string, boolean>>({});
@@ -171,13 +121,11 @@ export default function IssuesTab(props: IssuesTabProps) {
171121
const filteredSorted = createMemo(() => filteredSortedWithMeta().items);
172122
const issueMeta = createMemo(() => filteredSortedWithMeta().meta);
173123

174-
const pageSize = createMemo(() => config.itemsPerPage);
175-
176124
const repoGroups = createMemo(() => groupByRepo(filteredSorted()));
177-
const pageLayout = createMemo(() => computePageLayout(repoGroups(), pageSize()));
178-
const pageCount = () => pageLayout().pageCount;
125+
const pageLayout = createMemo(() => computePageLayout(repoGroups(), config.itemsPerPage));
126+
const pageCount = createMemo(() => pageLayout().pageCount);
179127
const pageGroups = createMemo(() =>
180-
slicePageGroups(repoGroups(), pageLayout().boundaries, pageLayout().pageCount, page())
128+
slicePageGroups(repoGroups(), pageLayout().boundaries, pageCount(), page())
181129
);
182130

183131
createEffect(() => {
@@ -298,43 +246,46 @@ export default function IssuesTab(props: IssuesTabProps) {
298246
>
299247
<div class="divide-y divide-gray-100 dark:divide-gray-800">
300248
<For each={pageGroups()}>
301-
{(repoGroup) => (
302-
<div class="bg-white dark:bg-gray-900">
303-
<button
304-
onClick={() => toggleRepo(repoGroup.repoFullName)}
305-
aria-expanded={!collapsedRepos[repoGroup.repoFullName]}
306-
class="w-full flex items-center gap-2 px-4 py-2 text-left text-sm font-semibold text-gray-700 dark:text-gray-200 bg-gray-50 dark:bg-gray-800 hover:bg-gray-100 dark:hover:bg-gray-700 transition-colors"
307-
>
308-
<ChevronIcon size="md" rotated={collapsedRepos[repoGroup.repoFullName]} />
309-
{repoGroup.repoFullName}
310-
</button>
311-
<Show when={!collapsedRepos[repoGroup.repoFullName]}>
312-
<div role="list" class="divide-y divide-gray-200 dark:divide-gray-700">
313-
<For each={repoGroup.items}>
314-
{(issue) => (
315-
<div role="listitem">
316-
<ItemRow
317-
hideRepo={true}
318-
repo={issue.repoFullName}
319-
number={issue.number}
320-
title={issue.title}
321-
author={issue.userLogin}
322-
createdAt={issue.createdAt}
323-
url={issue.htmlUrl}
324-
labels={issue.labels}
325-
onIgnore={() => handleIgnore(issue)}
326-
density={config.viewDensity}
327-
commentCount={issue.comments}
328-
>
329-
<RoleBadge roles={issueMeta().get(issue.id)?.roles ?? []} />
330-
</ItemRow>
331-
</div>
332-
)}
333-
</For>
334-
</div>
335-
</Show>
336-
</div>
337-
)}
249+
{(repoGroup) => {
250+
const isRepoCollapsed = () => collapsedRepos[repoGroup.repoFullName];
251+
return (
252+
<div class="bg-white dark:bg-gray-900">
253+
<button
254+
onClick={() => toggleRepo(repoGroup.repoFullName)}
255+
aria-expanded={!isRepoCollapsed()}
256+
class="w-full flex items-center gap-2 px-4 py-2 text-left text-sm font-semibold text-gray-700 dark:text-gray-200 bg-gray-50 dark:bg-gray-800 hover:bg-gray-100 dark:hover:bg-gray-700 transition-colors"
257+
>
258+
<ChevronIcon size="md" rotated={isRepoCollapsed()} />
259+
{repoGroup.repoFullName}
260+
</button>
261+
<Show when={!isRepoCollapsed()}>
262+
<div role="list" class="divide-y divide-gray-200 dark:divide-gray-700">
263+
<For each={repoGroup.items}>
264+
{(issue) => (
265+
<div role="listitem">
266+
<ItemRow
267+
hideRepo={true}
268+
repo={issue.repoFullName}
269+
number={issue.number}
270+
title={issue.title}
271+
author={issue.userLogin}
272+
createdAt={issue.createdAt}
273+
url={issue.htmlUrl}
274+
labels={issue.labels}
275+
onIgnore={() => handleIgnore(issue)}
276+
density={config.viewDensity}
277+
commentCount={issue.comments}
278+
>
279+
<RoleBadge roles={issueMeta().get(issue.id)?.roles ?? []} />
280+
</ItemRow>
281+
</div>
282+
)}
283+
</For>
284+
</div>
285+
</Show>
286+
</div>
287+
);
288+
}}
338289
</For>
339290
</div>
340291
</Show>

‎src/app/components/dashboard/PullRequestsTab.tsx‎

Lines changed: 33 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import SizeBadge from "../shared/SizeBadge";
1717
import RoleBadge from "../shared/RoleBadge";
1818
import SkeletonRows from "../shared/SkeletonRows";
1919
import ChevronIcon from "../shared/ChevronIcon";
20+
import { groupByRepo, computePageLayout, slicePageGroups } from "../../lib/grouping";
2021

2122
export interface PullRequestsTabProps {
2223
pullRequests: PullRequest[];
@@ -103,57 +104,6 @@ const prFilterGroups: FilterChipGroupDef[] = [
103104
},
104105
];
105106

106-
interface PrRepoGroup {
107-
repoFullName: string;
108-
items: PullRequest[];
109-
}
110-
111-
function groupByRepo(items: PullRequest[]): PrRepoGroup[] {
112-
const groups: PrRepoGroup[] = [];
113-
const map = new Map<string, PrRepoGroup>();
114-
for (const item of items) {
115-
let group = map.get(item.repoFullName);
116-
if (!group) {
117-
group = { repoFullName: item.repoFullName, items: [] };
118-
map.set(item.repoFullName, group);
119-
groups.push(group);
120-
}
121-
group.items.push(item);
122-
}
123-
return groups;
124-
}
125-
126-
function computePageLayout(
127-
groups: PrRepoGroup[],
128-
approxPageSize: number,
129-
): { boundaries: number[]; pageCount: number } {
130-
if (groups.length === 0) return { boundaries: [0], pageCount: 1 };
131-
132-
const boundaries: number[] = [0];
133-
let currentPageItems = 0;
134-
for (let i = 0; i < groups.length; i++) {
135-
if (currentPageItems > 0 && currentPageItems + groups[i].items.length > approxPageSize) {
136-
boundaries.push(i);
137-
currentPageItems = 0;
138-
}
139-
currentPageItems += groups[i].items.length;
140-
}
141-
142-
return { boundaries, pageCount: Math.max(1, boundaries.length) };
143-
}
144-
145-
function slicePageGroups(
146-
groups: PrRepoGroup[],
147-
boundaries: number[],
148-
pageCount: number,
149-
page: number,
150-
): PrRepoGroup[] {
151-
const clampedPage = Math.max(0, Math.min(page, pageCount - 1));
152-
const start = boundaries[clampedPage];
153-
const end = clampedPage + 1 < boundaries.length ? boundaries[clampedPage + 1] : groups.length;
154-
return groups.slice(start, end);
155-
}
156-
157107
export default function PullRequestsTab(props: PullRequestsTabProps) {
158108
const [page, setPage] = createSignal(0);
159109
const [collapsedRepos, setCollapsedRepos] = createStore<Record<string, boolean>>({});
@@ -251,13 +201,11 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
251201
const filteredSorted = createMemo(() => filteredSortedWithMeta().items);
252202
const prMeta = createMemo(() => filteredSortedWithMeta().meta);
253203

254-
const pageSize = createMemo(() => config.itemsPerPage);
255-
256204
const repoGroups = createMemo(() => groupByRepo(filteredSorted()));
257-
const pageLayout = createMemo(() => computePageLayout(repoGroups(), pageSize()));
258-
const pageCount = () => pageLayout().pageCount;
205+
const pageLayout = createMemo(() => computePageLayout(repoGroups(), config.itemsPerPage));
206+
const pageCount = createMemo(() => pageLayout().pageCount);
259207
const pageGroups = createMemo(() =>
260-
slicePageGroups(repoGroups(), pageLayout().boundaries, pageLayout().pageCount, page())
208+
slicePageGroups(repoGroups(), pageLayout().boundaries, pageCount(), page())
261209
);
262210

263211
createEffect(() => {
@@ -330,9 +278,18 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
330278
<FilterChips
331279
groups={prFilterGroups}
332280
values={viewState.tabFilters.pullRequests}
333-
onChange={(field, value) => { setTabFilter("pullRequests", field as PullRequestFilterField, value); setPage(0); }}
334-
onReset={(field) => { resetTabFilter("pullRequests", field as PullRequestFilterField); setPage(0); }}
335-
onResetAll={() => { resetAllTabFilters("pullRequests"); setPage(0); }}
281+
onChange={(field, value) => {
282+
setTabFilter("pullRequests", field as PullRequestFilterField, value);
283+
setPage(0);
284+
}}
285+
onReset={(field) => {
286+
resetTabFilter("pullRequests", field as PullRequestFilterField);
287+
setPage(0);
288+
}}
289+
onResetAll={() => {
290+
resetAllTabFilters("pullRequests");
291+
setPage(0);
292+
}}
336293
/>
337294
</div>
338295

@@ -370,17 +327,19 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
370327
>
371328
<div class="divide-y divide-gray-100 dark:divide-gray-800">
372329
<For each={pageGroups()}>
373-
{(repoGroup) => (
374-
<div class="bg-white dark:bg-gray-900">
375-
<button
376-
onClick={() => toggleRepo(repoGroup.repoFullName)}
377-
aria-expanded={!collapsedRepos[repoGroup.repoFullName]}
378-
class="w-full flex items-center gap-2 px-4 py-2 text-left text-sm font-semibold text-gray-700 dark:text-gray-200 bg-gray-50 dark:bg-gray-800 hover:bg-gray-100 dark:hover:bg-gray-700 transition-colors"
379-
>
380-
<ChevronIcon size="md" rotated={collapsedRepos[repoGroup.repoFullName]} />
381-
{repoGroup.repoFullName}
382-
</button>
383-
<Show when={!collapsedRepos[repoGroup.repoFullName]}>
330+
{(repoGroup) => {
331+
const isRepoCollapsed = () => collapsedRepos[repoGroup.repoFullName];
332+
return (
333+
<div class="bg-white dark:bg-gray-900">
334+
<button
335+
onClick={() => toggleRepo(repoGroup.repoFullName)}
336+
aria-expanded={!isRepoCollapsed()}
337+
class="w-full flex items-center gap-2 px-4 py-2 text-left text-sm font-semibold text-gray-700 dark:text-gray-200 bg-gray-50 dark:bg-gray-800 hover:bg-gray-100 dark:hover:bg-gray-700 transition-colors"
338+
>
339+
<ChevronIcon size="md" rotated={isRepoCollapsed()} />
340+
{repoGroup.repoFullName}
341+
</button>
342+
<Show when={!isRepoCollapsed()}>
384343
<div role="list" class="divide-y divide-gray-200 dark:divide-gray-700">
385344
<For each={repoGroup.items}>
386345
{(pr) => (
@@ -421,9 +380,10 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
421380
)}
422381
</For>
423382
</div>
424-
</Show>
425-
</div>
426-
)}
383+
</Show>
384+
</div>
385+
);
386+
}}
427387
</For>
428388
</div>
429389
</Show>

‎src/app/components/shared/ChevronIcon.tsx‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
1-
import { createMemo } from "solid-js";
2-
31
export default function ChevronIcon(props: { size: "sm" | "md"; rotated: boolean }) {
4-
const sizeClass = createMemo(() => (props.size === "sm" ? "h-3 w-3" : "h-3.5 w-3.5"));
2+
const sizeClass = () => (props.size === "sm" ? "h-3 w-3" : "h-3.5 w-3.5");
53
return (
64
<svg
75
xmlns="http://www.w3.org/2000/svg"

‎src/app/lib/grouping.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
export interface RepoGroup<T> {
2+
repoFullName: string;
3+
items: T[];
4+
}
5+
6+
export function groupByRepo<T extends { repoFullName: string }>(items: T[]): RepoGroup<T>[] {
7+
const groups: RepoGroup<T>[] = [];
8+
const map = new Map<string, RepoGroup<T>>();
9+
for (const item of items) {
10+
let group = map.get(item.repoFullName);
11+
if (!group) {
12+
group = { repoFullName: item.repoFullName, items: [] };
13+
map.set(item.repoFullName, group);
14+
groups.push(group);
15+
}
16+
group.items.push(item);
17+
}
18+
return groups;
19+
}
20+
21+
export function computePageLayout<T>(
22+
groups: RepoGroup<T>[],
23+
approxPageSize: number,
24+
): { boundaries: number[]; pageCount: number } {
25+
if (groups.length === 0) return { boundaries: [0], pageCount: 1 };
26+
27+
const boundaries: number[] = [0];
28+
let currentPageItems = 0;
29+
for (let i = 0; i < groups.length; i++) {
30+
if (currentPageItems > 0 && currentPageItems + groups[i].items.length > approxPageSize) {
31+
boundaries.push(i);
32+
currentPageItems = 0;
33+
}
34+
currentPageItems += groups[i].items.length;
35+
}
36+
37+
return { boundaries, pageCount: Math.max(1, boundaries.length) };
38+
}
39+
40+
export function slicePageGroups<T>(
41+
groups: RepoGroup<T>[],
42+
boundaries: number[],
43+
pageCount: number,
44+
page: number,
45+
): RepoGroup<T>[] {
46+
const clampedPage = Math.max(0, Math.min(page, pageCount - 1));
47+
const start = boundaries[clampedPage];
48+
const end = clampedPage + 1 < boundaries.length ? boundaries[clampedPage + 1] : groups.length;
49+
return groups.slice(start, end);
50+
}

0 commit comments

Comments
 (0)