Skip to content

Commit 4cd3f66

Browse files
wgordon17wgordon17
committed
fix(filters): addresses PR review findings
- removes orphaned sortPreferences schema field, SortPreference type, and _tabId parameter - adds globalSort to resetViewState, hoists globalSortOptions to module scope - replaces invalid role=listbox/option ARIA with aria-pressed on filter buttons - makes FilterBar sort props optional with Show guard - removes Tooltip wrapper from ScopeToggle, changes Reset all to btn-xs - inlines isValidUserFilter using local tabFilter snapshot - removes redundant sortPref createMemo passthrough in both tabs - adds tests for user-filter auto-reset, globalSort defaults, migration, scope-only active filter, stale value fallback, FilterBar sort rendering
1 parent 8776359 commit 4cd3f66

File tree

12 files changed

+137
-85
lines changed

12 files changed

+137
-85
lines changed

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

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,18 @@ import { formatCount } from "../../lib/format";
2828
import { setsEqual } from "../../lib/collections";
2929
import { Tooltip } from "../shared/Tooltip";
3030

31+
const globalSortOptions: SortOption[] = [
32+
{ label: "Repo", field: "repo", type: "text" },
33+
{ label: "Title", field: "title", type: "text" },
34+
{ label: "Author", field: "author", type: "text" },
35+
{ label: "Comments", field: "comments", type: "number" },
36+
{ label: "Checks", field: "checkStatus", type: "text" },
37+
{ label: "Review", field: "reviewDecision", type: "text" },
38+
{ label: "Size", field: "size", type: "number" },
39+
{ label: "Created", field: "createdAt", type: "date" },
40+
{ label: "Updated", field: "updatedAt", type: "date" },
41+
];
42+
3143
// ── Shared dashboard store (module-level to survive navigation) ─────────────
3244

3345
// Bump only for breaking schema changes (renames, type changes). Additive optional
@@ -373,18 +385,6 @@ export default function DashboardPage() {
373385
];
374386
});
375387

376-
const globalSortOptions: SortOption[] = [
377-
{ label: "Repo", field: "repo", type: "text" },
378-
{ label: "Title", field: "title", type: "text" },
379-
{ label: "Author", field: "author", type: "text" },
380-
{ label: "Comments", field: "comments", type: "number" },
381-
{ label: "Checks", field: "checkStatus", type: "text" },
382-
{ label: "Review", field: "reviewDecision", type: "text" },
383-
{ label: "Size", field: "size", type: "number" },
384-
{ label: "Created", field: "createdAt", type: "date" },
385-
{ label: "Updated", field: "updatedAt", type: "date" },
386-
];
387-
388388
return (
389389
<div class="min-h-screen bg-base-200">
390390
<Header />
@@ -413,7 +413,7 @@ export default function DashboardPage() {
413413
sortOptions={globalSortOptions}
414414
sortValue={viewState.globalSort.field}
415415
sortDirection={viewState.globalSort.direction}
416-
onSortChange={(field, dir) => setSortPreference(activeTab(), field, dir)}
416+
onSortChange={(field, dir) => setSortPreference(field, dir)}
417417
/>
418418
</div>
419419

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

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -72,10 +72,6 @@ export default function IssuesTab(props: IssuesTabProps) {
7272
(props.monitoredRepos ?? []).length > 0 || (props.allUsers?.length ?? 0) > 1
7373
);
7474

75-
const isValidUserFilter = createMemo(() =>
76-
!props.allUsers || props.allUsers.some(u => u.login === viewState.tabFilters.issues.user)
77-
);
78-
7975
const ignoredIssues = createMemo(() =>
8076
viewState.ignoredItems.filter(i => i.type === "issue")
8177
);
@@ -114,8 +110,6 @@ export default function IssuesTab(props: IssuesTabProps) {
114110
const isInvolvedItem = (item: Issue) =>
115111
isUserInvolved(item, userLoginLower(), monitoredRepoNameSet());
116112

117-
const sortPref = createMemo(() => viewState.globalSort);
118-
119113
const filteredSortedWithMeta = createMemo(() => {
120114
const filter = viewState.globalFilter;
121115
const tabFilter = viewState.tabFilters.issues;
@@ -151,7 +145,7 @@ export default function IssuesTab(props: IssuesTabProps) {
151145
if (tabFilter.user !== "all") {
152146
// Items from monitored repos bypass the surfacedBy filter (all activity is shown)
153147
if (!monitoredRepoNameSet().has(issue.repoFullName)) {
154-
const validUser = isValidUserFilter();
148+
const validUser = !props.allUsers || props.allUsers.some(u => u.login === tabFilter.user);
155149
if (validUser) {
156150
const surfacedBy = issue.surfacedBy ?? [userLoginLower()];
157151
if (!surfacedBy.includes(tabFilter.user)) return false;
@@ -163,7 +157,7 @@ export default function IssuesTab(props: IssuesTabProps) {
163157
return true;
164158
});
165159

166-
const { field, direction } = sortPref();
160+
const { field, direction } = viewState.globalSort;
167161
items = [...items].sort((a, b) => {
168162
let cmp = 0;
169163
switch (field as SortField) {

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

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,6 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
140140
(props.monitoredRepos ?? []).length > 0 || (props.allUsers?.length ?? 0) > 1
141141
);
142142

143-
const isValidUserFilter = createMemo(() =>
144-
!props.allUsers || props.allUsers.some(u => u.login === viewState.tabFilters.pullRequests.user)
145-
);
146-
147143
const ignoredPullRequests = createMemo(() =>
148144
viewState.ignoredItems.filter(i => i.type === "pullRequest")
149145
);
@@ -183,8 +179,6 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
183179
isUserInvolved(item, userLoginLower(), monitoredRepoNameSet(),
184180
item.enriched !== false ? item.reviewerLogins : undefined);
185181

186-
const sortPref = createMemo(() => viewState.globalSort);
187-
188182
const filteredSortedWithMeta = createMemo(() => {
189183
const filter = viewState.globalFilter;
190184
const tabFilters = viewState.tabFilters.pullRequests;
@@ -242,7 +236,7 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
242236
if (tabFilters.user !== "all") {
243237
// Items from monitored repos bypass the surfacedBy filter (all activity is shown)
244238
if (!monitoredRepoNameSet().has(pr.repoFullName)) {
245-
const validUser = isValidUserFilter();
239+
const validUser = !props.allUsers || props.allUsers.some(u => u.login === tabFilters.user);
246240
if (validUser) {
247241
const surfacedBy = pr.surfacedBy ?? [userLoginLower()];
248242
if (!surfacedBy.includes(tabFilters.user)) return false;
@@ -254,7 +248,7 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
254248
return true;
255249
});
256250

257-
const { field, direction } = sortPref();
251+
const { field, direction } = viewState.globalSort;
258252
items = [...items].sort((a, b) => {
259253
let cmp = 0;
260254
switch (field as SortField) {

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

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,11 @@ export default function FilterPopover(props: FilterPopoverProps) {
4848
</svg>
4949
</Popover.Trigger>
5050
<Popover.Portal>
51-
<Popover.Content role="listbox" aria-label={props.group.label} class="filter-popover-content bg-base-100 border border-base-300 rounded-lg shadow-lg z-50 p-2 min-w-40 max-h-64 overflow-y-auto">
51+
<Popover.Content aria-label={props.group.label} class="filter-popover-content bg-base-100 border border-base-300 rounded-lg shadow-lg z-50 p-2 min-w-40 max-h-64 overflow-y-auto">
5252
<Show when={!props.group.defaultValue}>
5353
<button
5454
type="button"
55-
role="option"
56-
aria-selected={value() === "all"}
55+
aria-pressed={value() === "all"}
5756
class={`w-full text-left px-3 py-1.5 rounded hover:bg-base-200 text-sm focus-visible:ring-2 focus-visible:ring-primary focus-visible:outline-none ${value() === "all" ? "font-medium" : ""}`}
5857
onClick={() => {
5958
props.onChange(props.group.field, "all");
@@ -67,8 +66,7 @@ export default function FilterPopover(props: FilterPopoverProps) {
6766
{(opt) => (
6867
<button
6968
type="button"
70-
role="option"
71-
aria-selected={value() === opt.value}
69+
aria-pressed={value() === opt.value}
7270
class={`w-full text-left px-3 py-1.5 rounded hover:bg-base-200 text-sm focus-visible:ring-2 focus-visible:ring-primary focus-visible:outline-none ${value() === opt.value ? "font-medium" : ""}`}
7371
onClick={() => {
7472
props.onChange(props.group.field, opt.value);

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,10 @@ export default function FilterToolbar(props: FilterToolbarProps) {
2929
return (
3030
<div class="flex items-center gap-2 flex-wrap">
3131
<Show when={showScope()}>
32-
<Tooltip content="Toggle between your activity and all activity">
33-
<ScopeToggle
34-
value={props.values["scope"] ?? scopeFilterGroup.defaultValue ?? "all"}
35-
onChange={props.onChange}
36-
/>
37-
</Tooltip>
32+
<ScopeToggle
33+
value={props.values["scope"] ?? scopeFilterGroup.defaultValue ?? "all"}
34+
onChange={props.onChange}
35+
/>
3836
<div class="w-px h-5 bg-base-300" />
3937
</Show>
4038
<For each={popoverGroups()}>
@@ -50,7 +48,7 @@ export default function FilterToolbar(props: FilterToolbarProps) {
5048
<Tooltip content="Reset all filters to defaults">
5149
<button
5250
type="button"
53-
class="btn btn-ghost btn-sm"
51+
class="btn btn-ghost btn-xs"
5452
onClick={props.onResetAll}
5553
>
5654
Reset all

‎src/app/stores/view.ts‎

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -39,15 +39,6 @@ export const ViewStateSchema = z.object({
3939
lastActiveTab: z
4040
.enum(["issues", "pullRequests", "actions"])
4141
.default("issues"),
42-
sortPreferences: z
43-
.record(
44-
z.string(),
45-
z.object({
46-
field: z.string(),
47-
direction: z.enum(["asc", "desc"]),
48-
})
49-
)
50-
.default({}),
5142
globalSort: z.object({
5243
field: z.string(),
5344
direction: z.enum(["asc", "desc"]),
@@ -103,7 +94,6 @@ export const ViewStateSchema = z.object({
10394

10495
export type ViewState = z.infer<typeof ViewStateSchema>;
10596
export type IgnoredItem = ViewState["ignoredItems"][number];
106-
export type SortPreference = ViewState["sortPreferences"][string];
10797
export type LockedReposTab = keyof ViewState["lockedRepos"];
10898

10999
function loadViewState(): ViewState {
@@ -126,7 +116,7 @@ export const [viewState, setViewState] = createStore<ViewState>(
126116
export function resetViewState(): void {
127117
updateViewState({
128118
lastActiveTab: "issues",
129-
sortPreferences: {},
119+
globalSort: { field: "updatedAt", direction: "desc" },
130120
ignoredItems: [],
131121
globalFilter: { org: null, repo: null },
132122
tabFilters: {
@@ -184,7 +174,6 @@ export function pruneStaleIgnoredItems(): void {
184174
}
185175

186176
export function setSortPreference(
187-
_tabId: string,
188177
field: string,
189178
direction: "asc" | "desc"
190179
): void {

‎tests/components/dashboard/IssuesTab.test.tsx‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -581,6 +581,19 @@ describe("IssuesTab — scope toggle visibility", () => {
581581
expect(screen.queryByRole("checkbox", { name: /Scope filter/i })).not.toBeNull();
582582
});
583583

584+
it("auto-resets user filter to 'all' when allUsers drops to 1", () => {
585+
setTabFilter("issues", "user", "tracked1");
586+
expect(viewState.tabFilters.issues.user).toBe("tracked1");
587+
588+
render(() => (
589+
<IssuesTab issues={[]} userLogin="me" monitoredRepos={[]}
590+
allUsers={[{ login: "me", label: "Me" }]}
591+
/>
592+
));
593+
594+
expect(viewState.tabFilters.issues.user).toBe("all");
595+
});
596+
584597
it("auto-resets scope to involves_me when scope toggle becomes hidden", () => {
585598
setTabFilter("issues", "scope", "all");
586599
expect(viewState.tabFilters.issues.scope).toBe("all");

‎tests/components/dashboard/PullRequestsTab.test.tsx‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -511,6 +511,19 @@ describe("PullRequestsTab — scope toggle visibility", () => {
511511
expect(screen.queryByRole("checkbox", { name: /Scope filter/i })).not.toBeNull();
512512
});
513513

514+
it("auto-resets user filter to 'all' when allUsers drops to 1", () => {
515+
setTabFilter("pullRequests", "user", "tracked1");
516+
expect(viewState.tabFilters.pullRequests.user).toBe("tracked1");
517+
518+
render(() => (
519+
<PullRequestsTab pullRequests={[]} userLogin="me" monitoredRepos={[]}
520+
allUsers={[{ login: "me", label: "Me" }]}
521+
/>
522+
));
523+
524+
expect(viewState.tabFilters.pullRequests.user).toBe("all");
525+
});
526+
514527
it("auto-resets scope to involves_me when scope toggle becomes hidden", () => {
515528
setTabFilter("pullRequests", "scope", "all");
516529
expect(viewState.tabFilters.pullRequests.scope).toBe("all");

‎tests/components/layout/FilterBar.test.tsx‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,21 @@ describe("FilterBar", () => {
166166
expect(viewStore.setGlobalFilter).toHaveBeenCalledWith(null, "myorg/repo-a");
167167
});
168168

169+
it("renders SortDropdown trigger button", () => {
170+
render(() => <FilterBar {...defaultSortProps} />);
171+
screen.getByRole("button", { name: /Sort by/i });
172+
});
173+
174+
it("SortDropdown is not rendered when sort props are omitted", () => {
175+
render(() => <FilterBar />);
176+
expect(screen.queryByRole("button", { name: /Sort by/i })).toBeNull();
177+
});
178+
179+
it("SortDropdown is not rendered when sortOptions is provided but onSortChange is omitted", () => {
180+
render(() => <FilterBar sortOptions={defaultSortProps.sortOptions} sortValue="updatedAt" sortDirection="desc" />);
181+
expect(screen.queryByRole("button", { name: /Sort by/i })).toBeNull();
182+
});
183+
169184
it("selecting 'All orgs' option calls setGlobalFilter with null org", async () => {
170185
const user = userEvent.setup({ delay: null });
171186
render(() => <FilterBar {...defaultSortProps} />);

‎tests/components/shared/FilterPopover.test.tsx‎

Lines changed: 38 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,10 @@ describe("FilterPopover", () => {
6868
const trigger = screen.getByRole("button", { name: /Filter by Review/i });
6969
fireEvent.click(trigger);
7070
vi.advanceTimersByTime(0);
71-
// "All" option should appear (now role="option")
72-
const allOption = screen.getAllByRole("option").find((o) => o.textContent?.includes("All"));
73-
expect(allOption).toBeDefined();
71+
// "All" option should appear as a button in the popover
72+
const buttons = screen.getAllByRole("button").filter(b => b !== trigger);
73+
const allBtn = buttons.find(b => b.textContent?.includes("All"));
74+
expect(allBtn).toBeDefined();
7475
screen.getByText("Approved");
7576
screen.getByText("Changes requested");
7677
});
@@ -99,9 +100,9 @@ describe("FilterPopover", () => {
99100
const trigger = screen.getByRole("button", { name: /Filter by Review/i });
100101
fireEvent.click(trigger);
101102
vi.advanceTimersByTime(0);
102-
const options = screen.getAllByRole("option");
103-
const approvedOpt = options.find((o) => o.textContent?.includes("Approved"));
104-
expect(approvedOpt?.textContent).toContain("✓");
103+
const buttons = screen.getAllByRole("button").filter(b => b !== trigger);
104+
const approvedBtn = buttons.find(b => b.textContent?.includes("Approved"));
105+
expect(approvedBtn?.textContent).toContain("✓");
105106
});
106107

107108
it("clicking option calls onChange and closes popover", () => {
@@ -122,9 +123,9 @@ describe("FilterPopover", () => {
122123
const trigger = screen.getByRole("button", { name: /Filter by Review/i });
123124
fireEvent.click(trigger);
124125
vi.advanceTimersByTime(0);
125-
const options = screen.getAllByRole("option");
126-
const allOption = options.find((o) => o.textContent?.includes("All"));
127-
fireEvent.click(allOption!);
126+
const buttons = screen.getAllByRole("button").filter(b => b !== trigger);
127+
const allBtn = buttons.find(b => b.textContent?.includes("All"));
128+
fireEvent.click(allBtn!);
128129
expect(onChange).toHaveBeenCalledWith("reviewDecision", "all");
129130
});
130131

@@ -150,26 +151,43 @@ describe("FilterPopover", () => {
150151
expect(trigger.getAttribute("aria-expanded")).toBe("false");
151152
});
152153

153-
it("option buttons use role='option' and aria-selected", () => {
154+
it("selected option has aria-pressed='true', unselected has 'false'", () => {
154155
render(() => <FilterPopover group={reviewGroup} value="APPROVED" onChange={() => {}} />);
155156
const trigger = screen.getByRole("button", { name: /Filter by Review/i });
156157
fireEvent.click(trigger);
157158
vi.advanceTimersByTime(0);
158-
const options = screen.getAllByRole("option");
159-
expect(options.length).toBeGreaterThan(0);
160-
const approvedOpt = options.find(o => o.textContent?.includes("Approved"));
161-
expect(approvedOpt?.getAttribute("aria-selected")).toBe("true");
162-
const changesOpt = options.find(o => o.textContent?.includes("Changes requested"));
163-
expect(changesOpt?.getAttribute("aria-selected")).toBe("false");
159+
const buttons = screen.getAllByRole("button").filter(b => b !== trigger);
160+
const approvedBtn = buttons.find(b => b.textContent?.includes("Approved"));
161+
expect(approvedBtn?.getAttribute("aria-pressed")).toBe("true");
162+
const changesBtn = buttons.find(b => b.textContent?.includes("Changes requested"));
163+
expect(changesBtn?.getAttribute("aria-pressed")).toBe("false");
164164
});
165165

166-
it("popover content has role='listbox' with group label", () => {
166+
it("'All' button has aria-pressed='true' when value is 'all'", () => {
167167
render(() => <FilterPopover group={reviewGroup} value="all" onChange={() => {}} />);
168168
const trigger = screen.getByRole("button", { name: /Filter by Review/i });
169169
fireEvent.click(trigger);
170170
vi.advanceTimersByTime(0);
171-
const listbox = document.querySelector("[role='listbox']");
172-
expect(listbox).toBeTruthy();
173-
expect(listbox?.getAttribute("aria-label")).toBe("Review");
171+
const buttons = screen.getAllByRole("button").filter(b => b !== trigger);
172+
const allBtn = buttons.find(b => b.textContent?.includes("All"));
173+
expect(allBtn?.getAttribute("aria-pressed")).toBe("true");
174+
const approvedBtn = buttons.find(b => b.textContent?.includes("Approved"));
175+
expect(approvedBtn?.getAttribute("aria-pressed")).toBe("false");
176+
});
177+
178+
it("popover content has aria-label with group label", () => {
179+
render(() => <FilterPopover group={reviewGroup} value="all" onChange={() => {}} />);
180+
const trigger = screen.getByRole("button", { name: /Filter by Review/i });
181+
fireEvent.click(trigger);
182+
vi.advanceTimersByTime(0);
183+
const content = document.querySelector("[aria-label='Review']");
184+
expect(content).toBeTruthy();
185+
});
186+
187+
it("shows raw value as label for unknown/stale filter values", () => {
188+
render(() => <FilterPopover group={reviewGroup} value="STALE_VALUE" onChange={() => {}} />);
189+
const trigger = screen.getByRole("button", { name: /Filter by Review/i });
190+
expect(trigger.className).toContain("btn-primary");
191+
expect(trigger.textContent).toContain("Review: STALE_VALUE");
174192
});
175193
});

0 commit comments

Comments
 (0)