Skip to content

Commit 4f60e87

Browse files
committed
fix(ui): addresses PR review findings for tracked items feature
1 parent 030f6cf commit 4f60e87

File tree

12 files changed

+221
-53
lines changed

12 files changed

+221
-53
lines changed

e2e/smoke.spec.ts

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,9 @@ test("unknown path redirects to dashboard when authenticated", async ({ page })
192192
// ── Tracked items ─────────────────────────────────────────────────────────────
193193

194194
test("tracked items tab appears when enabled", async ({ page }) => {
195-
// Override the GraphQL mock to return an issue
195+
// setupAuth FIRST — registers catch-all and default GraphQL handler
196+
await setupAuth(page, { enableTracking: true });
197+
// Override GraphQL AFTER setupAuth — Playwright matches routes LIFO, so this wins
196198
await page.route("https://api.github.com/graphql", (route) =>
197199
route.fulfill({
198200
status: 200,
@@ -224,11 +226,27 @@ test("tracked items tab appears when enabled", async ({ page }) => {
224226
},
225227
})
226228
);
227-
await setupAuth(page, { enableTracking: true });
228229
await page.goto("/dashboard");
229230

230231
// Verify Tracked tab is visible
231232
await expect(page.getByRole("tab", { name: /tracked/i })).toBeVisible();
233+
234+
// Wait for issue data to render, then expand the repo group
235+
await page.getByText("testorg/testrepo").click();
236+
await expect(page.getByText("Test tracked issue")).toBeVisible();
237+
238+
// Hover the row's group container to reveal the pin button (opacity-0 → group-hover:opacity-100)
239+
const row = page.locator(".group").filter({ hasText: "Test tracked issue" });
240+
await row.hover();
241+
const pinBtn = page.getByRole("button", { name: /^Pin #42/i });
242+
await expect(pinBtn).toBeVisible();
243+
await pinBtn.click();
244+
245+
// Switch to Tracked tab
246+
await page.getByRole("tab", { name: /tracked/i }).click();
247+
248+
// Verify the pinned item appears in the Tracked tab
249+
await expect(page.getByText("Test tracked issue")).toBeVisible();
232250
});
233251

234252
test("tracked items tab hidden when disabled", async ({ page }) => {

src/app/components/dashboard/DashboardPage.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -273,8 +273,8 @@ export default function DashboardPage() {
273273

274274
// Auto-prune tracked items that are closed/merged (absent from is:open results)
275275
createEffect(() => {
276-
// IMPORTANT: Access reactive store fields BEFORE non-reactive guards
277-
// so SolidJS registers them as dependencies
276+
// IMPORTANT: Access reactive store fields BEFORE early-return guards
277+
// so SolidJS registers them as dependencies even when the guard short-circuits
278278
const issues = dashboardData.issues;
279279
const prs = dashboardData.pullRequests;
280280
if (!config.enableTracking || viewState.trackedItems.length === 0 || !hasFetchedFresh()) return;

src/app/components/dashboard/IssuesTab.tsx

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,14 @@ export default function IssuesTab(props: IssuesTabProps) {
244244
if (config.enableTracking) untrackItem(issue.id, "issue");
245245
}
246246

247+
function handleTrack(issue: Issue) {
248+
if (trackedIssueIds().has(issue.id)) {
249+
untrackItem(issue.id, "issue");
250+
} else {
251+
trackItem({ id: issue.id, number: issue.number, type: "issue", repoFullName: issue.repoFullName, title: issue.title, addedAt: Date.now() });
252+
}
253+
}
254+
247255
return (
248256
<div class="flex flex-col h-full">
249257
{/* Filter chips + ignore badge toolbar */}
@@ -401,13 +409,7 @@ export default function IssuesTab(props: IssuesTabProps) {
401409
url={issue.htmlUrl}
402410
labels={issue.labels}
403411
onIgnore={() => handleIgnore(issue)}
404-
onTrack={config.enableTracking ? () => {
405-
if (trackedIssueIds().has(issue.id)) {
406-
untrackItem(issue.id, "issue");
407-
} else {
408-
trackItem({ id: issue.id, type: "issue", repoFullName: issue.repoFullName, title: issue.title, addedAt: Date.now() });
409-
}
410-
} : undefined}
412+
onTrack={config.enableTracking ? () => handleTrack(issue) : undefined}
411413
isTracked={config.enableTracking ? trackedIssueIds().has(issue.id) : undefined}
412414
density={config.viewDensity}
413415
commentCount={issue.comments}

src/app/components/dashboard/PullRequestsTab.tsx

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,14 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
350350
if (config.enableTracking) untrackItem(pr.id, "pullRequest");
351351
}
352352

353+
function handleTrack(pr: PullRequest) {
354+
if (trackedPrIds().has(pr.id)) {
355+
untrackItem(pr.id, "pullRequest");
356+
} else {
357+
trackItem({ id: pr.id, number: pr.number, type: "pullRequest", repoFullName: pr.repoFullName, title: pr.title, addedAt: Date.now() });
358+
}
359+
}
360+
353361
return (
354362
<div class="flex flex-col h-full">
355363
{/* Filter toolbar */}
@@ -560,13 +568,7 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
560568
labels={pr.labels}
561569
commentCount={pr.enriched !== false ? pr.comments + pr.reviewThreads : undefined}
562570
onIgnore={() => handleIgnore(pr)}
563-
onTrack={config.enableTracking ? () => {
564-
if (trackedPrIds().has(pr.id)) {
565-
untrackItem(pr.id, "pullRequest");
566-
} else {
567-
trackItem({ id: pr.id, type: "pullRequest", repoFullName: pr.repoFullName, title: pr.title, addedAt: Date.now() });
568-
}
569-
} : undefined}
571+
onTrack={config.enableTracking ? () => handleTrack(pr) : undefined}
570572
isTracked={config.enableTracking ? trackedPrIds().has(pr.id) : undefined}
571573
density={config.viewDensity}
572574
surfacedByBadge={

src/app/components/dashboard/TrackedTab.tsx

Lines changed: 30 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
1-
import { For, Show, createMemo } from "solid-js";
1+
import { For, Show, Switch, Match, createMemo } from "solid-js";
22
import { config } from "../../stores/config";
33
import { viewState, ignoreItem, untrackItem, moveTrackedItem } from "../../stores/view";
44
import type { Issue, PullRequest } from "../../services/api";
55
import ItemRow from "./ItemRow";
6+
import { Tooltip } from "../shared/Tooltip";
67

78
export interface TrackedTabProps {
89
issues: Issue[];
@@ -23,28 +24,26 @@ export default function TrackedTab(props: TrackedTabProps) {
2324
return { issueMap, prMap };
2425
});
2526

26-
const trackedItems = () => viewState.trackedItems;
27-
2827
return (
2928
<div class="flex flex-col h-full">
3029
<Show
31-
when={trackedItems().length > 0}
30+
when={viewState.trackedItems.length > 0}
3231
fallback={
3332
<div class="flex items-center justify-center py-16 text-center text-base-content/50 text-sm px-4">
3433
No tracked items. Pin issues or PRs from the Issues and Pull Requests tabs.
3534
</div>
3635
}
3736
>
3837
<div class="divide-y divide-base-300">
39-
<For each={trackedItems()}>
38+
<For each={viewState.trackedItems}>
4039
{(item, index) => {
4140
const liveData = () =>
4241
item.type === "issue"
4342
? maps().issueMap.get(item.id)
4443
: maps().prMap.get(item.id);
4544

4645
const isFirst = () => index() === 0;
47-
const isLast = () => index() === trackedItems().length - 1;
46+
const isLast = () => index() === viewState.trackedItems.length - 1;
4847

4948
return (
5049
<div class="flex items-center gap-1">
@@ -80,27 +79,30 @@ export default function TrackedTab(props: TrackedTabProps) {
8079
<span class="font-medium text-sm text-base-content truncate">
8180
{item.title}
8281
</span>
83-
<Show when={item.type === "issue"}>
84-
<span class="badge badge-outline badge-sm badge-info">Issue</span>
85-
</Show>
86-
<Show when={item.type === "pullRequest"}>
87-
<span class="badge badge-outline badge-sm badge-success">PR</span>
88-
</Show>
82+
<Switch>
83+
<Match when={item.type === "issue"}>
84+
<span class="badge badge-outline badge-sm badge-info">Issue</span>
85+
</Match>
86+
<Match when={item.type === "pullRequest"}>
87+
<span class="badge badge-outline badge-sm badge-success">PR</span>
88+
</Match>
89+
</Switch>
8990
</div>
9091
<div class="text-xs text-base-content/60 mt-0.5">
9192
{item.repoFullName}{" "}
9293
<span class="text-base-content/40">(not in current data)</span>
9394
</div>
9495
</div>
95-
<button
96-
class="relative z-10 shrink-0 self-center rounded p-1 text-primary transition-opacity focus:outline-none focus:ring-2 focus:ring-primary"
97-
aria-label={`Unpin ${item.title}`}
98-
title="Untrack this item"
99-
onClick={() => untrackItem(item.id, item.type)}
100-
>
96+
<Tooltip content="Untrack this item">
97+
<button
98+
class="relative z-10 shrink-0 self-center rounded p-1 text-primary transition-opacity focus:outline-none focus:ring-2 focus:ring-primary"
99+
aria-label={`Unpin #${item.number} ${item.title}`}
100+
onClick={() => untrackItem(item.id, item.type)}
101+
>
101102
{/* Solid bookmark */}
102-
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" fill="currentColor" class="h-4 w-4"><path fill-rule="evenodd" d="M6.32 2.577a49.255 49.255 0 0 1 11.36 0c1.497.174 2.57 1.46 2.57 2.93V21a.75.75 0 0 1-1.085.67L12 18.089l-7.165 3.583A.75.75 0 0 1 3.75 21V5.507c0-1.47 1.073-2.756 2.57-2.93Z" clip-rule="evenodd" /></svg>
103-
</button>
103+
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" fill="currentColor" class="h-4 w-4"><path fill-rule="evenodd" d="M6.32 2.577a49.255 49.255 0 0 1 11.36 0c1.497.174 2.57 1.46 2.57 2.93V21a.75.75 0 0 1-1.085.67L12 18.089l-7.165 3.583A.75.75 0 0 1 3.75 21V5.507c0-1.47 1.073-2.756 2.57-2.93Z" clip-rule="evenodd" /></svg>
104+
</button>
105+
</Tooltip>
104106
</div>
105107
}
106108
>
@@ -130,12 +132,14 @@ export default function TrackedTab(props: TrackedTabProps) {
130132
}}
131133
density={config.viewDensity}
132134
>
133-
<Show when={item.type === "issue"}>
134-
<span class="badge badge-outline badge-sm badge-info">Issue</span>
135-
</Show>
136-
<Show when={item.type === "pullRequest"}>
137-
<span class="badge badge-outline badge-sm badge-success">PR</span>
138-
</Show>
135+
<Switch>
136+
<Match when={item.type === "issue"}>
137+
<span class="badge badge-outline badge-sm badge-info">Issue</span>
138+
</Match>
139+
<Match when={item.type === "pullRequest"}>
140+
<span class="badge badge-outline badge-sm badge-success">PR</span>
141+
</Match>
142+
</Switch>
139143
</ItemRow>
140144
)}
141145
</Show>

src/app/stores/view.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ const TRACKED_ITEMS_CAP = 200;
99

1010
export const TrackedItemSchema = z.object({
1111
id: z.number(),
12+
number: z.number(),
1213
type: z.enum(["issue", "pullRequest"]),
1314
repoFullName: z.string(),
1415
title: z.string(),

tests/components/DashboardPage.test.tsx

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
2-
import { render, screen, waitFor } from "@solidjs/testing-library";
2+
import { render, screen, waitFor, fireEvent } from "@solidjs/testing-library";
33
import userEvent from "@testing-library/user-event";
44
import { makeIssue, makePullRequest, makeWorkflowRun } from "../helpers/index";
55
import type { DashboardData } from "../../src/app/services/poll";
@@ -839,6 +839,7 @@ describe("DashboardPage — tracked tab", () => {
839839
viewStore.updateViewState({
840840
trackedItems: [{
841841
id: 999,
842+
number: 99,
842843
type: "issue" as const,
843844
repoFullName: "org/repo",
844845
title: "Will be pruned",
@@ -872,6 +873,7 @@ describe("DashboardPage — tracked tab", () => {
872873
viewStore.updateViewState({
873874
trackedItems: [{
874875
id: 888,
876+
number: 88,
875877
type: "issue" as const,
876878
repoFullName: "org/deselected-repo",
877879
title: "Should be kept",
@@ -896,4 +898,94 @@ describe("DashboardPage — tracked tab", () => {
896898
expect(viewStore.viewState.trackedItems[0].id).toBe(888);
897899
});
898900
});
901+
902+
it("does not prune tracked items when hasFetchedFresh is false (cold start)", async () => {
903+
render(() => <DashboardPage />);
904+
configStore.updateConfig({
905+
enableTracking: true,
906+
selectedRepos: [{ owner: "org", name: "repo", fullName: "org/repo" }],
907+
});
908+
viewStore.updateViewState({
909+
trackedItems: [{
910+
id: 777,
911+
number: 77,
912+
type: "issue" as const,
913+
repoFullName: "org/repo",
914+
title: "Should survive cold start",
915+
addedAt: Date.now(),
916+
}],
917+
});
918+
// hasFetchedFresh stays false (its initial state) — do NOT call _resetHasFetchedFresh(true)
919+
// Do NOT trigger a poll (which would set hasFetchedFresh=true internally).
920+
// The prune effect should not fire against stale cached data.
921+
922+
// Allow reactive effects to settle
923+
await waitFor(() => {
924+
// Item should NOT be pruned — hasFetchedFresh is false
925+
expect(viewStore.viewState.trackedItems.length).toBe(1);
926+
expect(viewStore.viewState.trackedItems[0].id).toBe(777);
927+
});
928+
});
929+
930+
it("prunes tracked items from upstream repos", async () => {
931+
render(() => <DashboardPage />);
932+
configStore.updateConfig({
933+
enableTracking: true,
934+
selectedRepos: [],
935+
upstreamRepos: [{ owner: "ext", name: "upstream", fullName: "ext/upstream" }],
936+
});
937+
viewStore.updateViewState({
938+
trackedItems: [{
939+
id: 666,
940+
number: 66,
941+
type: "issue" as const,
942+
repoFullName: "ext/upstream",
943+
title: "Upstream item closed",
944+
addedAt: Date.now(),
945+
}],
946+
});
947+
_resetHasFetchedFresh(true);
948+
949+
if (capturedFetchAll) {
950+
vi.mocked(pollService.fetchAllData).mockResolvedValue({
951+
issues: [],
952+
pullRequests: [],
953+
workflowRuns: [],
954+
errors: [],
955+
});
956+
await capturedFetchAll();
957+
}
958+
959+
await waitFor(() => {
960+
expect(viewStore.viewState.trackedItems.length).toBe(0);
961+
});
962+
});
963+
964+
it("resolveInitialTab falls back to issues when tracked tab disabled", () => {
965+
viewStore.updateViewState({ lastActiveTab: "tracked" });
966+
configStore.updateConfig({ rememberLastTab: true, enableTracking: false });
967+
render(() => <DashboardPage />);
968+
// Should show Issues content, not Tracked content
969+
expect(screen.queryByText("No tracked items")).toBeNull();
970+
});
971+
972+
it("redirects away from tracked tab when tracking disabled at runtime", async () => {
973+
configStore.updateConfig({ enableTracking: true });
974+
render(() => <DashboardPage />);
975+
976+
// Switch to tracked tab
977+
const trackedTab = screen.getByText("Tracked");
978+
fireEvent.click(trackedTab);
979+
980+
await waitFor(() => {
981+
expect(viewStore.viewState.lastActiveTab).toBe("tracked");
982+
});
983+
984+
// Disable tracking — should redirect to issues
985+
configStore.updateConfig({ enableTracking: false });
986+
987+
await waitFor(() => {
988+
expect(viewStore.viewState.lastActiveTab).toBe("issues");
989+
});
990+
});
899991
});

tests/components/ItemRow.test.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ describe("ItemRow", () => {
101101
expect(onIgnore).toHaveBeenCalledOnce();
102102
});
103103

104-
it("ignore button has relative z-10 to sit above overlay link", () => {
104+
it("ignore button wrapper has relative z-10 to sit above overlay link", () => {
105105
render(() => <ItemRow {...defaultProps} />);
106106
const ignoreBtn = screen.getByLabelText(/Ignore #42/i);
107107
// The Tooltip wrapper span carries relative z-10; the button itself is inside it

0 commit comments

Comments
 (0)