Skip to content

Commit c326fc0

Browse files
committed
refactor(view): pr-review quality fixes
- Extract REPO_STATE_TAB_IDS constant for resetViewState loops - Export LOCKED_REPOS_CAP for test reference (no hardcoded 50) - Fix misleading comment on migrateLockedRepos passthrough - Reformat lock functions + moveTrackedItem to expanded style - Replace algorithm-copy cap-guard test with integration tests - Add custom tab lock mechanics tests for IssuesTab - Add migrateLockedRepos mixed-type array filter test
1 parent 5bd037f commit c326fc0

4 files changed

Lines changed: 146 additions & 41 deletions

File tree

src/app/stores/view.ts

Lines changed: 52 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { pushNotification } from "../lib/errors";
66
export const VIEW_STORAGE_KEY = "github-tracker:view";
77
const IGNORED_ITEMS_CAP = 500;
88
const TRACKED_ITEMS_CAP = 200;
9-
const LOCKED_REPOS_CAP = 50;
9+
export const LOCKED_REPOS_CAP = 50;
1010

1111
export const TrackedItemSchema = z.object({
1212
id: z.number(),
@@ -102,6 +102,8 @@ export const ViewStateSchema = z.object({
102102
export type ViewState = z.infer<typeof ViewStateSchema>;
103103
export type IgnoredItem = ViewState["ignoredItems"][number];
104104

105+
const REPO_STATE_TAB_IDS = ["issues", "pullRequests", "actions"] as const;
106+
105107
export function migrateLockedRepos(raw: unknown): unknown {
106108
if (raw == null) return { issues: [], pullRequests: [], actions: [] };
107109
if (Array.isArray(raw)) {
@@ -110,7 +112,7 @@ export function migrateLockedRepos(raw: unknown): unknown {
110112
return { issues: [...arr], pullRequests: [...arr], actions: [...arr] };
111113
}
112114
if (typeof raw === "object") {
113-
// Object → pass through as-is (already per-tab record shape)
115+
// Object → pass through as-is; loadViewState cap-guard sanitizes malformed entries
114116
return raw;
115117
}
116118
return { issues: [], pullRequests: [], actions: [] };
@@ -156,15 +158,15 @@ export function resetViewState(): void {
156158
produce((draft) => {
157159
// Delete dynamic custom tab keys that Object.assign wouldn't clear
158160
for (const key of Object.keys(draft.expandedRepos)) {
159-
if (!["issues", "pullRequests", "actions"].includes(key)) {
161+
if (!(REPO_STATE_TAB_IDS as readonly string[]).includes(key)) {
160162
delete draft.expandedRepos[key];
161163
}
162164
}
163165
for (const key of Object.keys(draft.customTabFilters)) {
164166
delete draft.customTabFilters[key];
165167
}
166168
for (const key of Object.keys(draft.lockedRepos)) {
167-
if (!["issues", "pullRequests", "actions"].includes(key)) {
169+
if (!(REPO_STATE_TAB_IDS as readonly string[]).includes(key)) {
168170
delete draft.lockedRepos[key];
169171
}
170172
}
@@ -371,38 +373,44 @@ export function removeCustomTabState(tabId: string): void {
371373
}
372374

373375
export function lockRepo(tabKey: string, repoFullName: string): void {
374-
setViewState(produce((draft) => {
375-
if (!draft.lockedRepos[tabKey]) draft.lockedRepos[tabKey] = [];
376-
const arr = draft.lockedRepos[tabKey];
377-
if (!arr.includes(repoFullName) && arr.length < LOCKED_REPOS_CAP) {
378-
arr.push(repoFullName);
379-
}
380-
}));
376+
setViewState(
377+
produce((draft) => {
378+
if (!draft.lockedRepos[tabKey]) draft.lockedRepos[tabKey] = [];
379+
const arr = draft.lockedRepos[tabKey];
380+
if (!arr.includes(repoFullName) && arr.length < LOCKED_REPOS_CAP) {
381+
arr.push(repoFullName);
382+
}
383+
})
384+
);
381385
}
382386

383387
export function unlockRepo(tabKey: string, repoFullName: string): void {
384-
setViewState(produce((draft) => {
385-
if (!draft.lockedRepos[tabKey]) return;
386-
draft.lockedRepos[tabKey] = draft.lockedRepos[tabKey].filter(r => r !== repoFullName);
387-
}));
388+
setViewState(
389+
produce((draft) => {
390+
if (!draft.lockedRepos[tabKey]) return;
391+
draft.lockedRepos[tabKey] = draft.lockedRepos[tabKey].filter((r) => r !== repoFullName);
392+
})
393+
);
388394
}
389395

390396
export function moveLockedRepo(
391397
tabKey: string,
392398
repoFullName: string,
393399
direction: "up" | "down"
394400
): void {
395-
setViewState(produce((draft) => {
396-
if (!draft.lockedRepos[tabKey]) return;
397-
const arr = draft.lockedRepos[tabKey];
398-
const idx = arr.indexOf(repoFullName);
399-
if (idx === -1) return;
400-
const targetIdx = direction === "up" ? idx - 1 : idx + 1;
401-
if (targetIdx < 0 || targetIdx >= arr.length) return;
402-
const tmp = arr[idx];
403-
arr[idx] = arr[targetIdx];
404-
arr[targetIdx] = tmp;
405-
}));
401+
setViewState(
402+
produce((draft) => {
403+
if (!draft.lockedRepos[tabKey]) return;
404+
const arr = draft.lockedRepos[tabKey];
405+
const idx = arr.indexOf(repoFullName);
406+
if (idx === -1) return;
407+
const targetIdx = direction === "up" ? idx - 1 : idx + 1;
408+
if (targetIdx < 0 || targetIdx >= arr.length) return;
409+
const tmp = arr[idx];
410+
arr[idx] = arr[targetIdx];
411+
arr[targetIdx] = tmp;
412+
})
413+
);
406414
}
407415

408416
export function pruneLockedRepos(
@@ -412,11 +420,13 @@ export function pruneLockedRepos(
412420
const current = untrack(() => viewState.lockedRepos[tabKey] ?? []);
413421
if (current.length === 0) return;
414422
const activeSet = new Set(activeRepoNames);
415-
const filtered = current.filter(name => activeSet.has(name));
423+
const filtered = current.filter((name) => activeSet.has(name));
416424
if (filtered.length === current.length) return;
417-
setViewState(produce((draft) => {
418-
draft.lockedRepos[tabKey] = filtered;
419-
}));
425+
setViewState(
426+
produce((draft) => {
427+
draft.lockedRepos[tabKey] = filtered;
428+
})
429+
);
420430
}
421431

422432
export function trackItem(item: TrackedItem): void {
@@ -451,16 +461,18 @@ export function moveTrackedItem(
451461
type: "issue" | "pullRequest",
452462
direction: "up" | "down"
453463
): void {
454-
setViewState(produce((draft) => {
455-
const arr = draft.trackedItems;
456-
const idx = arr.findIndex((i) => i.id === id && i.type === type);
457-
if (idx === -1) return;
458-
const targetIdx = direction === "up" ? idx - 1 : idx + 1;
459-
if (targetIdx < 0 || targetIdx >= arr.length) return;
460-
const tmp = arr[idx];
461-
arr[idx] = arr[targetIdx];
462-
arr[targetIdx] = tmp;
463-
}));
464+
setViewState(
465+
produce((draft) => {
466+
const arr = draft.trackedItems;
467+
const idx = arr.findIndex((i) => i.id === id && i.type === type);
468+
if (idx === -1) return;
469+
const targetIdx = direction === "up" ? idx - 1 : idx + 1;
470+
if (targetIdx < 0 || targetIdx >= arr.length) return;
471+
const tmp = arr[idx];
472+
arr[idx] = arr[targetIdx];
473+
arr[targetIdx] = tmp;
474+
})
475+
);
464476
}
465477

466478
export function pruneClosedTrackedItems(pruneKeys: Set<string>): void {

tests/components/dashboard/IssuesTab.test.tsx

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -797,6 +797,49 @@ describe("IssuesTab — empty-repo state preservation", () => {
797797
});
798798
});
799799

800+
// ── customTabId lock mechanics ───────────────────────────────────────────────
801+
802+
describe("IssuesTab — customTabId lock mechanics", () => {
803+
it("renders stub row for a locked empty repo under a custom tab key", () => {
804+
setViewState(produce((s) => {
805+
s.lockedRepos["my-tab"] = ["owner/locked-empty"];
806+
}));
807+
808+
const { container } = render(() => (
809+
<IssuesTab
810+
issues={[makeIssue({ id: 1, repoFullName: "owner/active-repo", surfacedBy: ["me"] })]}
811+
userLogin="me"
812+
customTabId="my-tab"
813+
configRepoNames={["owner/active-repo", "owner/locked-empty"]}
814+
/>
815+
));
816+
817+
const stub = container.querySelector('[data-repo-group="owner/locked-empty"]');
818+
expect(stub).not.toBeNull();
819+
expect(stub?.textContent).toContain("owner/locked-empty");
820+
expect(stub?.querySelector("[aria-expanded]")).toBeNull();
821+
expect(container.querySelector('[data-repo-group="owner/active-repo"]')).not.toBeNull();
822+
});
823+
824+
it("prunes locked repos outside configRepoNames under a custom tab key", () => {
825+
setViewState(produce((s) => {
826+
s.lockedRepos["my-tab"] = ["owner/kept", "owner/pruned"];
827+
}));
828+
829+
render(() => (
830+
<IssuesTab
831+
issues={[makeIssue({ id: 1, repoFullName: "owner/kept", surfacedBy: ["me"] })]}
832+
userLogin="me"
833+
customTabId="my-tab"
834+
configRepoNames={["owner/kept"]}
835+
/>
836+
));
837+
838+
expect(viewState.lockedRepos["my-tab"]).toContain("owner/kept");
839+
expect(viewState.lockedRepos["my-tab"]).not.toContain("owner/pruned");
840+
});
841+
});
842+
800843
// ── customTabId filter preset ────────────────────────────────────────────────
801844

802845
describe("IssuesTab — customTabId filter preset", () => {

tests/stores/view-lock.test.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,13 @@ describe("view lock store (per-tab)", () => {
288288
expect(migrateLockedRepos(42)).toEqual({ issues: [], pullRequests: [], actions: [] });
289289
expect(migrateLockedRepos("bad")).toEqual({ issues: [], pullRequests: [], actions: [] });
290290
});
291+
292+
it("filters out non-string elements from a flat mixed-type array", () => {
293+
const result = migrateLockedRepos([42, "org/repo", null]) as Record<string, string[]>;
294+
expect(result["issues"]).toEqual(["org/repo"]);
295+
expect(result["pullRequests"]).toEqual(["org/repo"]);
296+
expect(result["actions"]).toEqual(["org/repo"]);
297+
});
291298
});
292299

293300
describe("resetViewState — lockedRepos", () => {

tests/stores/view.test.ts

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, it, expect, beforeEach, vi } from "vitest";
1+
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
22
import { createRoot } from "solid-js";
33
import {
44
viewState,
@@ -765,3 +765,46 @@ describe("expandedRepos — dynamic tab keys", () => {
765765
expect(viewState.expandedRepos["tab-new"]["owner/repo"]).toBe(true);
766766
});
767767
});
768+
769+
describe("loadViewState — cap-guard integration", () => {
770+
afterEach(() => {
771+
localStorageMock.clear();
772+
});
773+
774+
it("deletes non-array lockedRepos tab values and preserves valid ones", async () => {
775+
localStorageMock.setItem(VIEW_KEY, JSON.stringify({
776+
lastActiveTab: "actions",
777+
lockedRepos: { issues: ["org/repo"], pullRequests: "bad-value" },
778+
}));
779+
780+
vi.resetModules();
781+
const mod = await import("../../src/app/stores/view");
782+
783+
expect(mod.viewState.lastActiveTab).toBe("actions");
784+
expect(mod.viewState.lockedRepos["issues"]).toEqual(["org/repo"]);
785+
expect(mod.viewState.lockedRepos["pullRequests"]).toBeUndefined();
786+
});
787+
788+
it("truncates oversized lockedRepos arrays to LOCKED_REPOS_CAP", async () => {
789+
const bigArray = Array.from({ length: 60 }, (_, i) => `org/repo-${i}`);
790+
localStorageMock.setItem(VIEW_KEY, JSON.stringify({
791+
lockedRepos: { issues: bigArray },
792+
}));
793+
794+
vi.resetModules();
795+
const mod = await import("../../src/app/stores/view");
796+
797+
expect(mod.viewState.lockedRepos["issues"].length).toBe(mod.LOCKED_REPOS_CAP);
798+
});
799+
800+
it("filters non-string elements from lockedRepos arrays", async () => {
801+
localStorageMock.setItem(VIEW_KEY, JSON.stringify({
802+
lockedRepos: { issues: [42, "org/repo", null, true, "org/other"] },
803+
}));
804+
805+
vi.resetModules();
806+
const mod = await import("../../src/app/stores/view");
807+
808+
expect(mod.viewState.lockedRepos["issues"]).toEqual(["org/repo", "org/other"]);
809+
});
810+
});

0 commit comments

Comments
 (0)