Skip to content

Commit 744cd81

Browse files
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 862ec58 commit 744cd81

File tree

9 files changed

+455
-210
lines changed

9 files changed

+455
-210
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: 73 additions & 113 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,60 +327,63 @@ 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]}>
384-
<div role="list" class="divide-y divide-gray-200 dark:divide-gray-700">
385-
<For each={repoGroup.items}>
386-
{(pr) => (
387-
<div role="listitem">
388-
<ItemRow
389-
hideRepo={true}
390-
repo={pr.repoFullName}
391-
number={pr.number}
392-
title={pr.title}
393-
author={pr.userLogin}
394-
createdAt={pr.createdAt}
395-
url={pr.htmlUrl}
396-
labels={pr.labels}
397-
commentCount={pr.comments + pr.reviewComments}
398-
onIgnore={() => handleIgnore(pr)}
399-
density={config.viewDensity}
400-
>
401-
<div class="flex items-center gap-2 flex-wrap">
402-
<RoleBadge roles={prMeta().get(pr.id)?.roles ?? []} />
403-
<ReviewBadge decision={pr.reviewDecision} />
404-
<SizeBadge additions={pr.additions} deletions={pr.deletions} changedFiles={pr.changedFiles} category={prMeta().get(pr.id)?.sizeCategory} />
405-
<StatusDot status={pr.checkStatus} />
406-
<Show when={pr.draft}>
407-
<span class="inline-flex items-center rounded-full bg-gray-100 dark:bg-gray-700 text-gray-600 dark:text-gray-300 text-xs px-2 py-0.5 font-medium">
408-
Draft
409-
</span>
410-
</Show>
411-
<Show when={pr.reviewerLogins.length > 0}>
412-
<span class="text-xs text-gray-500 dark:text-gray-400" title={pr.reviewerLogins.join(", ")}>
413-
Reviewers: {pr.reviewerLogins.slice(0, 5).join(", ")}
414-
{pr.reviewerLogins.length > 5 && ` +${pr.reviewerLogins.length - 5} more`}
415-
{pr.totalReviewCount > pr.reviewerLogins.length && ` (${pr.totalReviewCount} total)`}
416-
</span>
417-
</Show>
418-
</div>
419-
</ItemRow>
420-
</div>
421-
)}
422-
</For>
423-
</div>
424-
</Show>
425-
</div>
426-
)}
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()}>
343+
<div role="list" class="divide-y divide-gray-200 dark:divide-gray-700">
344+
<For each={repoGroup.items}>
345+
{(pr) => (
346+
<div role="listitem">
347+
<ItemRow
348+
hideRepo={true}
349+
repo={pr.repoFullName}
350+
number={pr.number}
351+
title={pr.title}
352+
author={pr.userLogin}
353+
createdAt={pr.createdAt}
354+
url={pr.htmlUrl}
355+
labels={pr.labels}
356+
commentCount={pr.comments + pr.reviewComments}
357+
onIgnore={() => handleIgnore(pr)}
358+
density={config.viewDensity}
359+
>
360+
<div class="flex items-center gap-2 flex-wrap">
361+
<RoleBadge roles={prMeta().get(pr.id)?.roles ?? []} />
362+
<ReviewBadge decision={pr.reviewDecision} />
363+
<SizeBadge additions={pr.additions} deletions={pr.deletions} changedFiles={pr.changedFiles} category={prMeta().get(pr.id)?.sizeCategory} />
364+
<StatusDot status={pr.checkStatus} />
365+
<Show when={pr.draft}>
366+
<span class="inline-flex items-center rounded-full bg-gray-100 dark:bg-gray-700 text-gray-600 dark:text-gray-300 text-xs px-2 py-0.5 font-medium">
367+
Draft
368+
</span>
369+
</Show>
370+
<Show when={pr.reviewerLogins.length > 0}>
371+
<span class="text-xs text-gray-500 dark:text-gray-400" title={pr.reviewerLogins.join(", ")}>
372+
Reviewers: {pr.reviewerLogins.slice(0, 5).join(", ")}
373+
{pr.reviewerLogins.length > 5 && ` +${pr.reviewerLogins.length - 5} more`}
374+
{pr.totalReviewCount > pr.reviewerLogins.length && ` (${pr.totalReviewCount} total)`}
375+
</span>
376+
</Show>
377+
</div>
378+
</ItemRow>
379+
</div>
380+
)}
381+
</For>
382+
</div>
383+
</Show>
384+
</div>
385+
);
386+
}}
427387
</For>
428388
</div>
429389
</Show>

0 commit comments

Comments
 (0)