Skip to content

Commit d0cfa32

Browse files
committed
fix(store): fixes resetViewState nested key leak
- fixes Zod+SolidJS interaction where .default({}) reuses refs - removes wrapper lambdas in ExpandCollapseButtons - adds pruning, empty-data, round-trip, and filter tests - adds resetViewState test proving expandedRepos cleared - renames view.test.ts helper delegating to resetViewState
1 parent 5422efb commit d0cfa32

File tree

6 files changed

+186
-18
lines changed

6 files changed

+186
-18
lines changed

src/app/components/shared/ExpandCollapseButtons.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ export default function ExpandCollapseButtons(props: ExpandCollapseButtonsProps)
1010
class="btn btn-ghost btn-xs"
1111
title="Expand all"
1212
aria-label="Expand all"
13-
onClick={() => props.onExpandAll()}
13+
onClick={props.onExpandAll}
1414
>
1515
<svg
1616
xmlns="http://www.w3.org/2000/svg"
@@ -31,7 +31,7 @@ export default function ExpandCollapseButtons(props: ExpandCollapseButtonsProps)
3131
class="btn btn-ghost btn-xs"
3232
title="Collapse all"
3333
aria-label="Collapse all"
34-
onClick={() => props.onCollapseAll()}
34+
onClick={props.onCollapseAll}
3535
>
3636
<svg
3737
xmlns="http://www.w3.org/2000/svg"

src/app/stores/view.ts

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,19 @@ export const [viewState, setViewState] = createStore<ViewState>(
102102
);
103103

104104
export function resetViewState(): void {
105-
const defaults = ViewStateSchema.parse({});
106-
setViewState(defaults);
105+
updateViewState({
106+
lastActiveTab: "issues",
107+
sortPreferences: {},
108+
ignoredItems: [],
109+
globalFilter: { org: null, repo: null },
110+
tabFilters: {
111+
issues: { role: "all", comments: "all" },
112+
pullRequests: { role: "all", reviewDecision: "all", draft: "all", checkStatus: "all", sizeCategory: "all" },
113+
actions: { conclusion: "all", event: "all" },
114+
},
115+
showPrRuns: false,
116+
expandedRepos: { issues: {}, pullRequests: {}, actions: {} },
117+
});
107118
}
108119

109120
export function updateViewState(partial: Partial<ViewState>): void {

tests/components/ActionsTab.test.tsx

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import { describe, it, expect, beforeEach } from "vitest";
22
import { render, screen } from "@solidjs/testing-library";
33
import userEvent from "@testing-library/user-event";
4+
import { createSignal } from "solid-js";
45
import ActionsTab from "../../src/app/components/dashboard/ActionsTab";
6+
import type { WorkflowRun } from "../../src/app/services/api";
57
import * as viewStore from "../../src/app/stores/view";
68
import { viewState } from "../../src/app/stores/view";
79
import { makeWorkflowRun, resetViewStore } from "../helpers/index";
@@ -355,10 +357,41 @@ describe("ActionsTab", () => {
355357
await user.click(screen.getByRole("button", { name: /Collapse all/i }));
356358
await user.click(screen.getByRole("button", { name: /Expand all/i }));
357359

358-
// Workflow card expansion is local state — it resets when repo collapses/re-renders
359-
// The key assertion: repo expand/collapse does not affect workflow-level state in viewState
360-
// (workflow state is local, not persisted)
360+
// Workflow card expansion is local component state (not persisted in viewState)
361+
// It survives collapse/expand within the same mount because expandedWorkflows
362+
// is at component scope, but would reset on full component remount
361363
expect(viewState.expandedRepos.actions["owner/repo"]).toBe(true);
364+
// Run row still visible — local store persists within same component instance
365+
screen.getByText("my-unique-run");
366+
});
367+
368+
it("prunes stale expanded keys when a repo disappears from data", () => {
369+
const [runs, setRuns] = createSignal<WorkflowRun[]>([
370+
makeWorkflowRun({ repoFullName: "owner/repo-a", workflowId: 1, name: "CI-A" }),
371+
makeWorkflowRun({ repoFullName: "owner/repo-b", workflowId: 2, name: "CI-B" }),
372+
]);
373+
viewStore.setAllExpanded("actions", ["owner/repo-a", "owner/repo-b"], true);
374+
render(() => <ActionsTab workflowRuns={runs()} />);
375+
376+
// Remove repo-b from data — pruning effect should fire
377+
setRuns([makeWorkflowRun({ repoFullName: "owner/repo-a", workflowId: 1, name: "CI-A" })]);
378+
expect(viewState.expandedRepos.actions["owner/repo-a"]).toBe(true);
379+
expect("owner/repo-b" in viewState.expandedRepos.actions).toBe(false);
380+
});
381+
382+
it("preserves expanded keys when data becomes empty and restores UI on re-population", () => {
383+
const [runs, setRuns] = createSignal<WorkflowRun[]>([
384+
makeWorkflowRun({ repoFullName: "owner/repo", workflowId: 1, name: "CI" }),
385+
]);
386+
viewStore.setAllExpanded("actions", ["owner/repo"], true);
387+
render(() => <ActionsTab workflowRuns={runs()} />);
388+
389+
setRuns([]);
390+
expect(viewState.expandedRepos.actions["owner/repo"]).toBe(true);
391+
392+
// Data returns — UI should use preserved expanded state
393+
setRuns([makeWorkflowRun({ repoFullName: "owner/repo", workflowId: 1, name: "CI" })]);
394+
expect(screen.getAllByText("CI").length).toBeGreaterThanOrEqual(1);
362395
});
363396

364397
it("expanded repo state persists in viewState", async () => {

tests/components/IssuesTab.test.tsx

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -390,6 +390,33 @@ describe("IssuesTab", () => {
390390
expect(screen.queryByText("Bob issue")).toBeNull();
391391
});
392392

393+
it("collapse all with active filter preserves hidden repos' expanded state", async () => {
394+
const user = userEvent.setup();
395+
const issues = [
396+
makeIssue({ id: 1, title: "Alice issue", repoFullName: "org/repo-a", userLogin: "alice" }),
397+
makeIssue({ id: 2, title: "Bob issue", repoFullName: "org/repo-b", userLogin: "bob" }),
398+
];
399+
setAllExpanded("issues", ["org/repo-a", "org/repo-b"], true);
400+
render(() => <IssuesTab issues={issues} userLogin="alice" />);
401+
screen.getByText("Alice issue");
402+
screen.getByText("Bob issue");
403+
404+
// Apply filter hiding repo-b (only alice's issues visible)
405+
viewStore.setTabFilter("issues", "role", "author");
406+
screen.getByText("Alice issue");
407+
expect(screen.queryByText("org/repo-b")).toBeNull();
408+
409+
// Collapse all — only affects visible (filtered) repos
410+
await user.click(screen.getByLabelText("Collapse all"));
411+
expect(screen.queryByText("Alice issue")).toBeNull();
412+
413+
// Remove filter — repo-b should still be expanded (was hidden during collapse-all)
414+
viewStore.resetTabFilter("issues", "role");
415+
screen.getByText("Bob issue");
416+
// repo-a was collapsed by collapse-all
417+
expect(screen.queryByText("Alice issue")).toBeNull();
418+
});
419+
393420
it("resets page when data shrinks below current page", async () => {
394421
const user = userEvent.setup();
395422
updateConfig({ itemsPerPage: 10 });
@@ -490,6 +517,54 @@ describe("IssuesTab", () => {
490517
screen.getByText("Survives remount");
491518
});
492519

520+
it("prunes stale expanded keys when a repo disappears from data", () => {
521+
const [issues, setIssues] = createSignal<Issue[]>([
522+
makeIssue({ id: 1, title: "Repo A issue", repoFullName: "org/repo-a" }),
523+
makeIssue({ id: 2, title: "Repo B issue", repoFullName: "org/repo-b" }),
524+
]);
525+
setAllExpanded("issues", ["org/repo-a", "org/repo-b"], true);
526+
render(() => <IssuesTab issues={issues()} userLogin="" />);
527+
// Both repos expanded
528+
screen.getByText("Repo A issue");
529+
screen.getByText("Repo B issue");
530+
531+
// Remove repo-b from data — pruning effect should fire
532+
setIssues([makeIssue({ id: 1, title: "Repo A issue", repoFullName: "org/repo-a" })]);
533+
expect(viewState.expandedRepos.issues["org/repo-a"]).toBe(true);
534+
expect("org/repo-b" in viewState.expandedRepos.issues).toBe(false);
535+
});
536+
537+
it("prunes the first repo key when it disappears (name-based, not positional)", () => {
538+
const [issues, setIssues] = createSignal<Issue[]>([
539+
makeIssue({ id: 1, title: "Repo A issue", repoFullName: "org/repo-a" }),
540+
makeIssue({ id: 2, title: "Repo B issue", repoFullName: "org/repo-b" }),
541+
]);
542+
setAllExpanded("issues", ["org/repo-a", "org/repo-b"], true);
543+
render(() => <IssuesTab issues={issues()} userLogin="" />);
544+
545+
// Remove repo-a (first), keep repo-b (second)
546+
setIssues([makeIssue({ id: 2, title: "Repo B issue", repoFullName: "org/repo-b" })]);
547+
expect("org/repo-a" in viewState.expandedRepos.issues).toBe(false);
548+
expect(viewState.expandedRepos.issues["org/repo-b"]).toBe(true);
549+
});
550+
551+
it("preserves expanded keys when data becomes empty and restores UI on re-population", () => {
552+
const [issues, setIssues] = createSignal<Issue[]>([
553+
makeIssue({ id: 1, title: "Issue A", repoFullName: "org/repo-a" }),
554+
]);
555+
setAllExpanded("issues", ["org/repo-a"], true);
556+
render(() => <IssuesTab issues={issues()} userLogin="" />);
557+
screen.getByText("Issue A");
558+
559+
// Data becomes empty (e.g. loading state) — expanded state should be preserved
560+
setIssues([]);
561+
expect(viewState.expandedRepos.issues["org/repo-a"]).toBe(true);
562+
563+
// Data returns — UI should use preserved expanded state
564+
setIssues([makeIssue({ id: 1, title: "Issue A", repoFullName: "org/repo-a" })]);
565+
screen.getByText("Issue A");
566+
});
567+
493568
it("clicking 'Expand all' expands repos on other pages too", async () => {
494569
const user = userEvent.setup();
495570
updateConfig({ itemsPerPage: 10 });

tests/components/PullRequestsTab.test.tsx

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -570,6 +570,38 @@ describe("PullRequestsTab", () => {
570570
screen.getByText("Persistent PR");
571571
});
572572

573+
it("prunes stale expanded keys when a repo disappears from data", () => {
574+
const [prs, setPrs] = createSignal<PullRequest[]>([
575+
makePullRequest({ id: 1, title: "Repo A PR", repoFullName: "org/repo-a" }),
576+
makePullRequest({ id: 2, title: "Repo B PR", repoFullName: "org/repo-b" }),
577+
]);
578+
setAllExpanded("pullRequests", ["org/repo-a", "org/repo-b"], true);
579+
render(() => <PullRequestsTab pullRequests={prs()} userLogin="" />);
580+
screen.getByText("Repo A PR");
581+
screen.getByText("Repo B PR");
582+
583+
// Remove repo-b from data — pruning effect should fire
584+
setPrs([makePullRequest({ id: 1, title: "Repo A PR", repoFullName: "org/repo-a" })]);
585+
expect(viewStore.viewState.expandedRepos.pullRequests["org/repo-a"]).toBe(true);
586+
expect("org/repo-b" in viewStore.viewState.expandedRepos.pullRequests).toBe(false);
587+
});
588+
589+
it("preserves expanded keys when data becomes empty and restores UI on re-population", () => {
590+
const [prs, setPrs] = createSignal<PullRequest[]>([
591+
makePullRequest({ id: 1, title: "PR A", repoFullName: "org/repo-a" }),
592+
]);
593+
setAllExpanded("pullRequests", ["org/repo-a"], true);
594+
render(() => <PullRequestsTab pullRequests={prs()} userLogin="" />);
595+
screen.getByText("PR A");
596+
597+
setPrs([]);
598+
expect(viewStore.viewState.expandedRepos.pullRequests["org/repo-a"]).toBe(true);
599+
600+
// Data returns — UI should use preserved expanded state
601+
setPrs([makePullRequest({ id: 1, title: "PR A", repoFullName: "org/repo-a" })]);
602+
screen.getByText("PR A");
603+
});
604+
573605
it("clicking 'Expand all' expands repos on other pages too", async () => {
574606
const user = userEvent.setup();
575607
updateConfig({ itemsPerPage: 10 });

tests/stores/view.test.ts

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { createRoot } from "solid-js";
33
import {
44
viewState,
55
updateViewState,
6+
resetViewState,
67
ignoreItem,
78
unignoreItem,
89
setSortPreference,
@@ -44,20 +45,12 @@ Object.defineProperty(globalThis, "localStorage", {
4445
configurable: true,
4546
});
4647

47-
const defaultState = ViewStateSchema.parse({});
48-
49-
function resetViewState() {
50-
updateViewState({
51-
lastActiveTab: defaultState.lastActiveTab,
52-
sortPreferences: {},
53-
ignoredItems: [],
54-
globalFilter: { org: null, repo: null },
55-
expandedRepos: { issues: {}, pullRequests: {}, actions: {} },
56-
});
48+
function resetForTest() {
49+
resetViewState();
5750
}
5851

5952
beforeEach(() => {
60-
resetViewState();
53+
resetForTest();
6154
localStorageMock.clear();
6255
});
6356

@@ -243,6 +236,14 @@ describe("expandedRepos helpers", () => {
243236
expect(viewState.expandedRepos.issues["owner/c"]).toBe(true);
244237
});
245238

239+
it("setAllExpanded with empty array is a no-op", () => {
240+
setAllExpanded("issues", ["owner/existing"], true);
241+
setAllExpanded("issues", [], true);
242+
expect(viewState.expandedRepos.issues["owner/existing"]).toBe(true);
243+
setAllExpanded("issues", [], false);
244+
expect(viewState.expandedRepos.issues["owner/existing"]).toBe(true);
245+
});
246+
246247
it("setAllExpanded with expanded=false deletes all keys (sparse record)", () => {
247248
setAllExpanded("issues", ["owner/a", "owner/b"], true);
248249
setAllExpanded("issues", ["owner/a", "owner/b"], false);
@@ -289,3 +290,19 @@ describe("expandedRepos helpers", () => {
289290
vi.useRealTimers();
290291
});
291292
});
293+
294+
describe("resetViewState", () => {
295+
it("clears dynamically-added expandedRepos keys", () => {
296+
setAllExpanded("issues", ["org/repo-a", "org/repo-b"], true);
297+
setAllExpanded("pullRequests", ["org/repo-c"], true);
298+
toggleExpandedRepo("actions", "org/repo-d");
299+
expect(viewState.expandedRepos.issues["org/repo-a"]).toBe(true);
300+
301+
resetViewState();
302+
303+
expect("org/repo-a" in viewState.expandedRepos.issues).toBe(false);
304+
expect("org/repo-b" in viewState.expandedRepos.issues).toBe(false);
305+
expect("org/repo-c" in viewState.expandedRepos.pullRequests).toBe(false);
306+
expect("org/repo-d" in viewState.expandedRepos.actions).toBe(false);
307+
});
308+
});

0 commit comments

Comments
 (0)