From faf38be91aecd56c667e9ccc3334d01f7b22e23b Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 24 Aug 2026 16:02:33 -0500 Subject: [PATCH 1/3] feat(filters): make the storefront filter panel searchable and multi-select (#139) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The filter drawer did not scale with the taxonomy behind it. Categories were a bare antd `Tree` rendered at whatever depth it had grown to, with no search and single selection, and tags were a wall of every tag in the system. Neither said what was selected except through highlighting and chip colour. Both are now searchable multi-selects. Categories keep their hierarchy in a `TreeSelect`, matching the admin's `CategoryTreeSelect` so the two screens behave alike; tags become a multiple `Select` whose selected pills keep their colours, which is the only place a tag's colour was load-bearing. Several categories combine as OR. A customer picking Furniture and Decor wants both, not the empty intersection, and each selected id still expands to its descendants, so the answer is the union of the subtrees. That is deliberately the opposite of the tag rule, which stays AND, and both headings now state their rule rather than leaving it to be discovered. `ItemFilters.categoryId` becomes `categoryIds` end to end. The recursive CTE is seeded with `= ANY($n::int[])` rather than one id, which walks every selected root in one recursion and gives the OR for free; matching on `IN` keeps it a set test, so an item under two selected branches still appears once. The query parameter keeps its singular name and becomes comma-separated, the shape `tags` and `status` already use, so every `?category=1` link written before this still parses as a list of one. A list containing anything unreadable is still a 400, per decision 9 — honouring the readable half would answer a narrower question than the one asked and look indistinguishable from a filter that worked. The admin's inventory filter stays single-select, since it asks what is in a category rather than in any of several, but reads and writes a list of at most one so there is one shared filter type rather than two that drift. Closes #139 --- backend/src/itemFilters.ts | 55 ++++++++-- .../categoriesTags.integration.test.ts | 38 +++++++ backend/tests/unit/itemFilters.test.ts | 44 +++++++- frontend/src/admin/InventoryFilters.tsx | 12 +- frontend/src/components/ActiveFilterChips.tsx | 17 +-- frontend/src/components/FilterDrawer.tsx | 103 +++++++++++------- frontend/src/filters.ts | 24 +++- frontend/tests/e2e/filters.spec.ts | 35 ++++++ frontend/tests/e2e/pages/FilterDrawer.ts | 59 ++++++++-- 9 files changed, 308 insertions(+), 79 deletions(-) diff --git a/backend/src/itemFilters.ts b/backend/src/itemFilters.ts index f223f88..ad98c98 100644 --- a/backend/src/itemFilters.ts +++ b/backend/src/itemFilters.ts @@ -8,7 +8,14 @@ export type { ItemStatus }; export class FilterError extends Error {} export interface ItemFilters { - categoryId: number | null; + // Several categories, combined as OR — a customer picking Furniture and + // Decor wants both, not the empty intersection. Deliberately the opposite of + // how tagIds combine below, which is AND; the two controls say so in the UI + // rather than leaving it to be discovered. + // + // Each selected id still expands to its descendants, so choosing two parents + // means "anything filed under either of them". + categoryIds: number[]; tagIds: number[]; minPriceCents: number | null; maxPriceCents: number | null; @@ -110,12 +117,33 @@ function parsePrice(value: unknown, name: string): number | null { return parseNonNegativeInteger(raw, name); } -function parseCategoryId(value: unknown): number | null { +/** + * Comma-separated, the same shape `tags` and `status` already use. + * + * The parameter keeps its singular name so that every `?category=1` link, + * bookmark and shared URL written before this went multi-valued still parses — + * as a list of one, needing no alias and leaving no way to give the filter + * twice with different meanings. + */ +function parseCategoryIds(value: unknown): number[] { const raw = singleValue(value, 'category'); - if (raw === null || raw === '') { - return null; + const categoryIds: number[] = []; + if (!raw) { + return categoryIds; } - return parseId(raw, 'category'); + for (const part of raw.split(',')) { + const trimmed = part.trim(); + if (trimmed === '') { + continue; + } + const id = parseId(trimmed, 'category'); + // Duplicates are harmless to the OR below, but they would show twice in + // any caller that renders the parsed filter back. + if (!categoryIds.includes(id)) { + categoryIds.push(id); + } + } + return categoryIds; } function parseTagIds(value: unknown): number[] { @@ -193,7 +221,7 @@ function parseFavoritesOnly(value: unknown): boolean { // deliberate and observable: a query wrong in two ways reports the first // field, so moving these lines around changes which error a caller sees. export function parseItemFilters(query: Record): ItemFilters { - const categoryId = parseCategoryId(query.category); + const categoryIds = parseCategoryIds(query.category); const tagIds = parseTagIds(query.tags); const minPriceCents = parsePrice(query.min_price, 'min_price'); @@ -205,7 +233,7 @@ export function parseItemFilters(query: Record): ItemFilters { const status = parseStatus(query.status); const favoritesOnly = parseFavoritesOnly(query.favorites); - return { categoryId, tagIds, minPriceCents, maxPriceCents, status, favoritesOnly }; + return { categoryIds, tagIds, minPriceCents, maxPriceCents, status, favoritesOnly }; } // Returns WHERE fragments plus their parameters, with placeholders numbered @@ -226,15 +254,20 @@ export function buildItemFilterSql( const params: unknown[] = []; let next = startIndex; - if (filters.categoryId !== null) { - params.push(filters.categoryId); + if (filters.categoryIds.length) { + params.push(filters.categoryIds); // Selecting a category means "and everything filed beneath it", so walk the - // tree down from the chosen node. A recursive CTE keeps the tree + // tree down from each chosen node. A recursive CTE keeps the tree // un-denormalized: reparenting stays a single UPDATE with no stored paths // to rewrite. + // + // Seeded with `= ANY(...)` rather than one id, so every selected root is + // walked in the same recursion. That also gives the OR for free: the union + // of the subtrees is exactly "filed under any of these", and an item filed + // under two selected branches appears once because IN is a set test. clauses.push(`i.category_id IN ( WITH RECURSIVE subtree AS ( - SELECT id FROM categories WHERE id = $${next} + SELECT id FROM categories WHERE id = ANY($${next}::int[]) UNION ALL SELECT c.id FROM categories c JOIN subtree s ON c.parent_id = s.id ) diff --git a/backend/tests/integration/categoriesTags.integration.test.ts b/backend/tests/integration/categoriesTags.integration.test.ts index bbbd16f..161d791 100644 --- a/backend/tests/integration/categoriesTags.integration.test.ts +++ b/backend/tests/integration/categoriesTags.integration.test.ts @@ -371,6 +371,34 @@ describe('GET /api/items filtering', () => { expect(res.body.map((i: { name: string }) => i.name).sort()).toEqual(['Deep', 'Mid', 'Top']); }); + it('combines several categories as any-of rather than all-of', async () => { + const furniture = await createCategory('Furniture'); + const tables = await createCategory('Tables', furniture); + const decor = await createCategory('Decor'); + const art = await createCategory('Art'); + + await createItem('Nested', 1000, tables); + await createItem('Elsewhere', 1000, decor); + await createItem('Unrelated', 1000, art); + + // Two branches sharing no items, so an AND would answer with nothing at + // all — which is the failure this exists to catch. + const res = await request(app).get(`/api/items?category=${furniture},${decor}`); + expect(res.body.map((i: { name: string }) => i.name).sort()).toEqual(['Elsewhere', 'Nested']); + }); + + it('lists an item once when its category sits under two selected branches', async () => { + const furniture = await createCategory('Furniture'); + const tables = await createCategory('Tables', furniture); + await createItem('Nested', 1000, tables); + + // The subtrees overlap: Tables is walked from both seeds, so the recursion + // yields its id twice. Selecting on IN makes that a set test rather than a + // join, so the item still appears once. + const res = await request(app).get(`/api/items?category=${furniture},${tables}`); + expect(res.body.map((i: { name: string }) => i.name)).toEqual(['Nested']); + }); + it('excludes uncategorized items from a category filter', async () => { const furniture = await createCategory('Furniture'); await createItem('Filed', 1000, furniture); @@ -444,6 +472,16 @@ describe('GET /api/items filtering', () => { expect(res.status).toBe(400); }); + it('rejects a category list with one unreadable entry rather than honouring the rest', async () => { + const furniture = await createCategory('Furniture'); + await createItem('A', 1000, furniture); + + // Filtering on the readable half would answer a narrower question than the + // one asked, and look indistinguishable from a filter that worked. + const res = await request(app).get(`/api/items?category=${furniture},furniture`); + expect(res.status).toBe(400); + }); + it('rejects an inverted price range', async () => { const res = await request(app).get('/api/items?min_price=5000&max_price=1000'); expect(res.status).toBe(400); diff --git a/backend/tests/unit/itemFilters.test.ts b/backend/tests/unit/itemFilters.test.ts index 7c5eb5a..845aaf6 100644 --- a/backend/tests/unit/itemFilters.test.ts +++ b/backend/tests/unit/itemFilters.test.ts @@ -3,7 +3,7 @@ import { parseItemFilters, FilterError, buildItemFilterSql } from '../../src/ite describe('parseItemFilters', () => { it('returns empty filters for an empty query', () => { expect(parseItemFilters({})).toEqual({ - categoryId: null, + categoryIds: [], tagIds: [], minPriceCents: null, maxPriceCents: null, @@ -12,8 +12,30 @@ describe('parseItemFilters', () => { }); }); - it('parses a category id', () => { - expect(parseItemFilters({ category: '7' }).categoryId).toBe(7); + // The parameter stayed singular when it went multi-valued (#139), so every + // link written before that still parses — as a list of one. + it('parses a single category id, as older links still send it', () => { + expect(parseItemFilters({ category: '7' }).categoryIds).toEqual([7]); + }); + + it('parses a comma-separated category list', () => { + expect(parseItemFilters({ category: '4,9,2' }).categoryIds).toEqual([4, 9, 2]); + }); + + it('collapses duplicate category ids', () => { + expect(parseItemFilters({ category: '3,3,8' }).categoryIds).toEqual([3, 8]); + }); + + it('treats an empty category list as no category filter', () => { + expect(parseItemFilters({ category: '' }).categoryIds).toEqual([]); + }); + + // Decision 9: a malformed filter shows itself rather than quietly returning + // the whole catalogue, and going multi-valued must not weaken that. + it('refuses a category list containing anything unreadable', () => { + expect(() => parseItemFilters({ category: '4,nope' })).toThrow(FilterError); + expect(() => parseItemFilters({ category: '0' })).toThrow(FilterError); + expect(() => parseItemFilters({ category: '-1,2' })).toThrow(FilterError); }); it('parses a comma-separated tag list', () => { @@ -158,7 +180,19 @@ describe('buildItemFilterSql', () => { it('matches a category and all of its descendants', () => { const built = buildItemFilterSql(parseItemFilters({ category: '4' }), 1, null); expect(built.clauses.join(' ')).toContain('RECURSIVE'); - expect(built.params).toEqual([4]); + // One array parameter rather than one id: the CTE is seeded with ANY so + // several selected roots are walked in the same recursion. + expect(built.params).toEqual([[4]]); + }); + + it('seeds the descendant walk with every selected category', () => { + const built = buildItemFilterSql(parseItemFilters({ category: '4,9' }), 1, null); + const sql = built.clauses.join(' '); + expect(sql).toContain('RECURSIVE'); + // ANY over the seeds is what makes several categories combine as OR: the + // result is the union of their subtrees. + expect(sql).toContain('= ANY($1::int[])'); + expect(built.params).toEqual([[4, 9]]); }); it('requires every listed tag rather than any of them', () => { @@ -215,7 +249,7 @@ describe('buildItemFilterSql', () => { 1, null ); - expect(built.params).toEqual([4, 100, 900]); + expect(built.params).toEqual([[4], 100, 900]); const sql = built.clauses.join(' '); expect(sql).toContain('$1'); expect(sql).toContain('$2'); diff --git a/frontend/src/admin/InventoryFilters.tsx b/frontend/src/admin/InventoryFilters.tsx index a2a57fe..44f898a 100644 --- a/frontend/src/admin/InventoryFilters.tsx +++ b/frontend/src/admin/InventoryFilters.tsx @@ -65,8 +65,16 @@ export default function InventoryFilters({ categories, tags, filters, onChange, aria-label="Filter by category" style={{ minWidth: 200 }} treeData={treeData} - value={filters.categoryId ?? undefined} - onChange={(value) => onChange({ ...filters, categoryId: value ?? null })} + // The filter shape went multi-valued for the storefront (#139). This + // control stays single-select — the admin asks "what is in this + // category", not "in any of these" — so it reads and writes a list of + // at most one rather than growing a second shape. + value={filters.categoryIds[0] ?? undefined} + // Annotated nullable because allowClear hands back undefined, which + // the control's own onChange type does not admit. + onChange={(value: number | undefined) => + onChange({ ...filters, categoryIds: value === undefined ? [] : [value] }) + } /> onChange({ ...filters, tagIds })} + options={tags.map((tag) => ({ value: tag.id, label: tag.name }))} + tagRender={({ value, label, closable, onClose }) => ( + { + event.preventDefault(); + event.stopPropagation(); + }} + style={{ marginInlineEnd: 4 }} + > + {label} + + )} + /> ) : ( )} diff --git a/frontend/src/filters.ts b/frontend/src/filters.ts index 1b984f9..dbd86f1 100644 --- a/frontend/src/filters.ts +++ b/frontend/src/filters.ts @@ -3,7 +3,14 @@ import type { Category } from './api'; export type ItemStatus = 'pending' | 'available' | 'reserved' | 'sold'; export interface ItemFilters { - categoryId: number | null; + // Several categories, combined as OR — picking Furniture and Decor means + // either, not the empty intersection. Deliberately the opposite of tagIds + // below, which is AND, and both controls label their rule so the difference + // is stated rather than discovered. + // + // The admin's inventory filter is single-select and holds a list of one; the + // type is shared, and one shape is better than two that drift. + categoryIds: number[]; tagIds: number[]; minPriceCents: number | null; maxPriceCents: number | null; @@ -69,7 +76,7 @@ export function saleStateFromStatuses( } export const EMPTY_FILTERS: ItemFilters = { - categoryId: null, + categoryIds: [], tagIds: [], minPriceCents: null, maxPriceCents: null, @@ -82,7 +89,9 @@ export const EMPTY_FILTERS: ItemFilters = { // what GET /api/items accepts, so the same object serializes for both. export function filtersToSearchParams(filters: ItemFilters): URLSearchParams { const params = new URLSearchParams(); - if (filters.categoryId !== null) params.set('category', String(filters.categoryId)); + // Comma-separated under the singular name it has always had, so a link + // written before this went multi-valued still means what it meant. + if (filters.categoryIds.length) params.set('category', filters.categoryIds.join(',')); if (filters.tagIds.length) params.set('tags', filters.tagIds.join(',')); if (filters.minPriceCents !== null) params.set('min_price', String(filters.minPriceCents)); if (filters.maxPriceCents !== null) params.set('max_price', String(filters.maxPriceCents)); @@ -98,6 +107,11 @@ function readInt(raw: string | null): number | null { } export function filtersFromSearchParams(params: URLSearchParams): ItemFilters { + const categories = (params.get('category') || '') + .split(',') + .map((part) => readInt(part)) + .filter((id): id is number => id !== null && id > 0); + const tags = (params.get('tags') || '') .split(',') .map((part) => readInt(part)) @@ -127,7 +141,7 @@ export function filtersFromSearchParams(params: URLSearchParams): ItemFilters { const favorites = params.get('favorites'); return { - categoryId: readInt(params.get('category')), + categoryIds: categories, tagIds: tags, minPriceCents: readInt(params.get('min_price')), maxPriceCents: readInt(params.get('max_price')), @@ -145,7 +159,7 @@ export function filtersFromSearchParams(params: URLSearchParams): ItemFilters { // displays its own position. export function activeFilterCount(filters: ItemFilters): number { let count = 0; - if (filters.categoryId !== null) count++; + count += filters.categoryIds.length; count += filters.tagIds.length; if (filters.minPriceCents !== null || filters.maxPriceCents !== null) count++; if (filters.favoritesOnly) count++; diff --git a/frontend/tests/e2e/filters.spec.ts b/frontend/tests/e2e/filters.spec.ts index c9f1a4d..c8dabe3 100644 --- a/frontend/tests/e2e/filters.spec.ts +++ b/frontend/tests/e2e/filters.spec.ts @@ -89,6 +89,41 @@ test.describe('Storefront filters', () => { await expect(storefront.card(NAMES.midItem)).toBeHidden(); }); + test('several categories combine as any-of rather than all-of', async ({ + page, + storefront, + filterDrawer + }) => { + await storefront.goto(); + await storefront.openFilters(); + + // Different branches with no items in common, so an AND would show nothing. + await filterDrawer.chooseCategories(NAMES.tables, NAMES.decor); + + await expect(storefront.card(NAMES.deepItem)).toBeVisible(); + await expect(storefront.card(NAMES.otherItem)).toBeVisible(); + // Filed directly in Furniture, which was not among the selections. + await expect(storefront.card(NAMES.midItem)).toBeHidden(); + + // Both ride in the one parameter the filter has always used, so links + // written before it went multi-valued still mean what they meant. + await expect(page).toHaveURL(/category=\d+,\d+/); + }); + + test('each selected category gets its own removable chip', async ({ storefront, filterDrawer }) => { + await storefront.goto(); + await storefront.openFilters(); + await filterDrawer.chooseCategories(NAMES.tables, NAMES.decor); + await filterDrawer.close(); + + await storefront.removeFilterChip(NAMES.decor).click(); + + // Removing one chip narrows the filter to the other rather than clearing + // the category filter outright. + await expect(storefront.card(NAMES.deepItem)).toBeVisible(); + await expect(storefront.card(NAMES.otherItem)).toBeHidden(); + }); + test('requires every selected tag rather than any of them', async ({ storefront, filterDrawer }) => { await storefront.goto(); await storefront.openFilters(); diff --git a/frontend/tests/e2e/pages/FilterDrawer.ts b/frontend/tests/e2e/pages/FilterDrawer.ts index 63077e6..b4fc4bb 100644 --- a/frontend/tests/e2e/pages/FilterDrawer.ts +++ b/frontend/tests/e2e/pages/FilterDrawer.ts @@ -3,13 +3,17 @@ import { Locator, Page, expect } from '@playwright/test'; /** * The storefront's filter drawer. * - * Categories are a tree rather than a list, because a category filter matches - * the node and everything filed beneath it — so the locator is `treeitem`, and - * a nested category is only reachable once the tree has loaded its data. + * Both taxonomy filters are searchable multi-selects whose options only exist + * while the list is open, so every choose/toggle here opens the list, acts, and + * closes it again. * - * Tags are buttons that toggle. The rule they follow is AND, not OR: selecting - * two tags means "must have both", which is deliberately different from how - * categories combine and is the thing several tests exist to pin down. + * Categories keep their hierarchy — a category filter matches the node and + * everything filed beneath it — so their options are `treeitem`. Tags are flat, + * so theirs are `option`. + * + * The two follow opposite rules, which is the thing several tests exist to pin + * down: categories are OR (any of the selected branches), tags are AND (the + * item must carry all of them). */ export class FilterDrawer { readonly minimumPrice: Locator; @@ -17,6 +21,9 @@ export class FilterDrawer { readonly closeButton: Locator; readonly clearAllButton: Locator; readonly favoritesOnlySwitch: Locator; + readonly categorySelect: Locator; + readonly tagSelect: Locator; + readonly title: Locator; constructor(private readonly page: Page) { this.minimumPrice = page.getByLabel('Minimum price'); @@ -24,6 +31,9 @@ export class FilterDrawer { this.closeButton = page.getByRole('button', { name: 'Close' }); this.clearAllButton = page.getByRole('button', { name: 'Clear all' }); this.favoritesOnlySwitch = page.getByRole('switch', { name: 'Only my favorites' }); + this.categorySelect = page.getByRole('combobox', { name: 'Filter by category' }); + this.tagSelect = page.getByRole('combobox', { name: 'Filter by tags' }); + this.title = page.getByRole('heading', { name: 'Filters' }); } category(name: string): Locator { @@ -31,15 +41,50 @@ export class FilterDrawer { } tag(name: string): Locator { - return this.page.getByRole('button', { name }); + return this.page.getByRole('option', { name }); + } + + /** + * Opens the category list and leaves it open. + * + * Separate from choosing, so a test that picks two categories opens once — + * which is also what a customer does, the list staying open being the point + * of a multi-select. + */ + async openCategoryList(): Promise { + await this.categorySelect.click(); + } + + /** + * Closes whichever option list is open, without closing the drawer. + * + * Escape would do both — the drawer listens for it too — so this clicks off + * the control instead, onto the one part of the drawer nothing overlaps. + */ + async closeOptionList(): Promise { + await this.title.click(); } async chooseCategory(name: string): Promise { + await this.openCategoryList(); await this.category(name).click(); + await this.closeOptionList(); } + /** Picks several categories from one opening of the list, as the UI intends. */ + async chooseCategories(...names: string[]): Promise { + await this.openCategoryList(); + for (const name of names) { + await this.category(name).click(); + } + await this.closeOptionList(); + } + + /** Clicking a selected option in a multi-select deselects it, so this toggles. */ async toggleTag(name: string): Promise { + await this.tagSelect.click(); await this.tag(name).click(); + await this.closeOptionList(); } async setPriceRange(minimum?: string, maximum?: string): Promise { From 856d8c4511aaa26ce3c2206408c0c3315a486417 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 24 Aug 2026 16:26:40 -0500 Subject: [PATCH 2/3] fix(filters): give the category tree selectable values, and drive both controls by search (#139) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things the e2e run found. `toTreeData` still emitted `key`, which is how an antd `Tree` identifies a node and not how a `TreeSelect` selects one. Nothing could be picked, and `treeNodeFilterProp="title"` had nothing to filter against. It now emits `value`, matching the admin's CategoryTreeSelect. The page object typed the name before clicking it, in both controls. Not for realism: the option lists are virtualized, so against a database holding hundreds of categories the wanted row never renders until a search narrows to it, and scrolling to it would be testing the virtual list rather than the filter. Tag options are matched by class rather than by role, for the reason AdminInventory.toggleStatus already records — antd renders an invisible role="listbox" shim beside the real list, so getByRole('option') resolves to something zero-sized that can never be clicked. The drawer's own title is a plain element rather than a heading, so the click that closes an option list lands on the Favorites heading instead. The multi-category URL assertion accepts the comma either percent-encoded or literal. URLSearchParams encodes it, which is how the `tags` parameter has always looked, and both spellings parse. --- frontend/src/components/FilterDrawer.tsx | 14 ++++++++-- frontend/tests/e2e/filters.spec.ts | 7 ++++- frontend/tests/e2e/pages/FilterDrawer.ts | 35 ++++++++++++++++++++---- 3 files changed, 46 insertions(+), 10 deletions(-) diff --git a/frontend/src/components/FilterDrawer.tsx b/frontend/src/components/FilterDrawer.tsx index 8626fa1..8ffff8c 100644 --- a/frontend/src/components/FilterDrawer.tsx +++ b/frontend/src/components/FilterDrawer.tsx @@ -8,7 +8,6 @@ import InputNumber from 'antd/es/input-number'; import Empty from 'antd/es/empty'; import Switch from 'antd/es/switch'; import Grid from 'antd/es/grid'; -import type { DataNode } from 'antd/es/tree'; import type { FilterOptions } from '../api'; import { ItemFilters, buildCategoryTree, CategoryNode } from '../filters'; @@ -22,9 +21,18 @@ type Props = Readonly<{ resultCount: number; }>; -function toTreeData(nodes: CategoryNode[]): DataNode[] { +// `value` rather than `key`: this fed an antd `Tree`, which identifies nodes by +// key, and now feeds a `TreeSelect`, which selects and searches by value. The +// shape matches the admin's CategoryTreeSelect so the two stay comparable. +interface CategoryTreeOption { + value: number; + title: string; + children?: CategoryTreeOption[]; +} + +function toTreeData(nodes: CategoryNode[]): CategoryTreeOption[] { return nodes.map((node) => ({ - key: node.id, + value: node.id, title: node.name, children: node.children.length ? toTreeData(node.children) : undefined })); diff --git a/frontend/tests/e2e/filters.spec.ts b/frontend/tests/e2e/filters.spec.ts index c8dabe3..e37686a 100644 --- a/frontend/tests/e2e/filters.spec.ts +++ b/frontend/tests/e2e/filters.spec.ts @@ -107,7 +107,12 @@ test.describe('Storefront filters', () => { // Both ride in the one parameter the filter has always used, so links // written before it went multi-valued still mean what they meant. - await expect(page).toHaveURL(/category=\d+,\d+/); + // + // The separator arrives percent-encoded because URLSearchParams encodes a + // comma, which is how the `tags` parameter has always looked too. Either + // spelling parses, so the assertion accepts both rather than pinning the + // encoding. + await expect(page).toHaveURL(/category=\d+(,|%2C)\d+/); }); test('each selected category gets its own removable chip', async ({ storefront, filterDrawer }) => { diff --git a/frontend/tests/e2e/pages/FilterDrawer.ts b/frontend/tests/e2e/pages/FilterDrawer.ts index b4fc4bb..e26fdf0 100644 --- a/frontend/tests/e2e/pages/FilterDrawer.ts +++ b/frontend/tests/e2e/pages/FilterDrawer.ts @@ -7,9 +7,18 @@ import { Locator, Page, expect } from '@playwright/test'; * while the list is open, so every choose/toggle here opens the list, acts, and * closes it again. * + * Each one types the name before clicking it. Not for realism — the lists are + * virtualized, so against a database holding hundreds of categories the wanted + * row is never rendered until a search narrows to it. Scrolling to it would be + * testing the virtual list rather than the filter. + * * Categories keep their hierarchy — a category filter matches the node and - * everything filed beneath it — so their options are `treeitem`. Tags are flat, - * so theirs are `option`. + * everything filed beneath it — so their options are `treeitem`. + * + * Tags are flat, and their options are matched by class rather than by role, + * for the reason AdminInventory.toggleStatus records: antd renders an invisible + * role="listbox" shim beside the real list, so getByRole('option') resolves to + * something zero-sized that can never be clicked. * * The two follow opposite rules, which is the thing several tests exist to pin * down: categories are OR (any of the selected branches), tags are AND (the @@ -33,7 +42,9 @@ export class FilterDrawer { this.favoritesOnlySwitch = page.getByRole('switch', { name: 'Only my favorites' }); this.categorySelect = page.getByRole('combobox', { name: 'Filter by category' }); this.tagSelect = page.getByRole('combobox', { name: 'Filter by tags' }); - this.title = page.getByRole('heading', { name: 'Filters' }); + // Somewhere inside the drawer that is inert and never covered by an + // option list, which opens downward from the controls below it. + this.title = page.getByRole('heading', { name: 'Favorites' }); } category(name: string): Locator { @@ -41,7 +52,7 @@ export class FilterDrawer { } tag(name: string): Locator { - return this.page.getByRole('option', { name }); + return this.page.locator(`.ant-select-item-option[title="${name}"]`); } /** @@ -55,6 +66,17 @@ export class FilterDrawer { await this.categorySelect.click(); } + /** + * Narrows to one category and clicks it, with the list already open. + * + * Selecting clears the search box, so consecutive calls each start from the + * unfiltered list. + */ + private async pickCategory(name: string): Promise { + await this.categorySelect.fill(name); + await this.category(name).click(); + } + /** * Closes whichever option list is open, without closing the drawer. * @@ -67,7 +89,7 @@ export class FilterDrawer { async chooseCategory(name: string): Promise { await this.openCategoryList(); - await this.category(name).click(); + await this.pickCategory(name); await this.closeOptionList(); } @@ -75,7 +97,7 @@ export class FilterDrawer { async chooseCategories(...names: string[]): Promise { await this.openCategoryList(); for (const name of names) { - await this.category(name).click(); + await this.pickCategory(name); } await this.closeOptionList(); } @@ -83,6 +105,7 @@ export class FilterDrawer { /** Clicking a selected option in a multi-select deselects it, so this toggles. */ async toggleTag(name: string): Promise { await this.tagSelect.click(); + await this.tagSelect.fill(name); await this.tag(name).click(); await this.closeOptionList(); } From 9db3c6d94c9e2e07a3ba773ae4af41755e7e5f2a Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 24 Aug 2026 16:41:24 -0500 Subject: [PATCH 3/3] feat(admin): filter inventory through the same flyout the storefront uses (#169) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two screens asked the same questions through different UI. The storefront had searchable multi-selects in a flyout; the admin still had an always-visible row of controls with a single-select category that held a list of at most one, which is what #139 left behind so the shared filter type would not have to change shape twice. The drawer is now one component with the sections that differ driven by props rather than a second copy that would drift. Favorites is storefront-only. Status is admin-only, since pending is excluded from every public read and Published or Unpublished are not distinctions a customer can draw — the storefront keeps its three-way preset outside the drawer. The price slider needs real catalogue-wide bounds to be honest about where the prices are, and the admin has none, so there it is the two number inputs alone. What is shared is not only the markup but the phrasing: that categories are OR and tags are AND has to read the same on both screens or it stops being one rule. This reverses a decision `InventoryFilters.tsx` argued for in a comment — that hiding controls above a data table costs more than the space it saves, and that a drawer overlays the very rows being filtered. Both are true and both are traded for consistency between the panels. The active-filter chips are what makes the trade bearable: the current filter stays readable beside the button without opening anything, which is the part the always-visible row was really protecting. Status gets chips too, since it is now behind the button and is the filter most likely to empty a table. `STATUS_OPTIONS` moves beside the filter type, because the drawer and the chips both need to turn a status into a label and a second copy is a second place for a new status to be forgotten. The admin page object opens the flyout, acts, and closes it again — closing matters, because the drawer overlays the table every assertion in those specs is about. Closes #169 --- frontend/src/App.tsx | 8 +- frontend/src/admin/Admin.tsx | 1 + frontend/src/admin/InventoryFilters.tsx | 180 ++++++------------ frontend/src/components/ActiveFilterChips.tsx | 38 +++- frontend/src/components/FilterDrawer.tsx | 91 +++++++-- frontend/src/filters.ts | 18 ++ .../tests/e2e/admin-inventory-filters.spec.ts | 13 +- frontend/tests/e2e/pages/AdminInventory.ts | 74 +++++-- 8 files changed, 261 insertions(+), 162 deletions(-) diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index f87ddda..cad4bdb 100755 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -230,7 +230,8 @@ export default function App() { Filters{activeCount ? ` (${activeCount})` : ''} setDrawerOpen(false)} - options={options} + categories={options?.categories ?? []} + tags={options?.tags ?? []} + priceRange={options?.priceRange ?? null} filters={filters} onChange={applyFilters} onClear={clearFilters} resultCount={items.length} + showFavorites /> {/* The same prompt the heart button and Add to Cart use. Signing in diff --git a/frontend/src/admin/Admin.tsx b/frontend/src/admin/Admin.tsx index 0e95270..7bcb6b9 100755 --- a/frontend/src/admin/Admin.tsx +++ b/frontend/src/admin/Admin.tsx @@ -275,6 +275,7 @@ function Inventory() { filters={filters} onChange={applyFilters} onClear={clearFilters} + resultCount={items.length} /> diff --git a/frontend/src/admin/InventoryFilters.tsx b/frontend/src/admin/InventoryFilters.tsx index 44f898a..36f83d7 100644 --- a/frontend/src/admin/InventoryFilters.tsx +++ b/frontend/src/admin/InventoryFilters.tsx @@ -1,40 +1,10 @@ -import { useMemo } from 'react'; -import TreeSelect from 'antd/es/tree-select'; -import Select from 'antd/es/select'; -import InputNumber from 'antd/es/input-number'; +import { useState } from 'react'; import Button from 'antd/es/button'; +import { FilterOutlined } from '@ant-design/icons'; import type { Category, Tag } from '../api'; -import { - ItemFilters, - ItemStatus, - buildCategoryTree, - CategoryNode, - hasActiveFilters -} from '../filters'; - -// Named individually rather than grouped, because grouping is what the preset -// this replaces did. Pending is listed first: "what is waiting to be published" -// is the question that prompted #132. -const STATUS_OPTIONS: { value: ItemStatus; label: string }[] = [ - { value: 'pending', label: 'Pending' }, - { value: 'available', label: 'Available' }, - { value: 'reserved', label: 'Reserved' }, - { value: 'sold', label: 'Sold' } -]; - -interface CategoryTreeOption { - value: number; - title: string; - children?: CategoryTreeOption[]; -} - -function toTreeData(nodes: CategoryNode[]): CategoryTreeOption[] { - return nodes.map((node) => ({ - value: node.id, - title: node.name, - children: node.children.length ? toTreeData(node.children) : undefined - })); -} +import { ItemFilters, activeFilterCount } from '../filters'; +import FilterDrawer from '../components/FilterDrawer'; +import ActiveFilterChips from '../components/ActiveFilterChips'; type Props = Readonly<{ categories: Category[]; @@ -42,103 +12,67 @@ type Props = Readonly<{ filters: ItemFilters; onChange: (filters: ItemFilters) => void; onClear: () => void; + resultCount: number; }>; -const centsToDollars = (cents: number | null): number | null => (cents === null ? null : cents / 100); -const dollarsToCents = (dollars: number | null): number | null => - dollars === null || Number.isNaN(dollars) ? null : Math.round(dollars * 100); +// The same flyout the storefront uses, rather than the row of controls this +// used to be (#169). +// +// That row was deliberate — it carried a comment arguing that hiding the +// controls above a data table costs more than the space it saves, and that a +// drawer overlays the very rows being filtered. Both are true, and both are +// traded for the two screens asking the same questions through the same UI. +// The chips are what makes the trade bearable: the active filter stays readable +// beside the button without opening anything, which is what the always-visible +// row was really protecting. +export default function InventoryFilters({ + categories, + tags, + filters, + onChange, + onClear, + resultCount +}: Props) { + const [drawerOpen, setDrawerOpen] = useState(false); -// An always-visible row rather than the storefront's drawer: this sits above a -// data table, where hiding the controls behind a click costs more than the -// space it saves, and a drawer would overlay the very rows being filtered. -export default function InventoryFilters({ categories, tags, filters, onChange, onClear }: Props) { - const treeData = useMemo(() => toTreeData(buildCategoryTree(categories)), [categories]); + // Status lives in the drawer here, unlike on the storefront, so it belongs in + // the button's tally — activeFilterCount leaves it out precisely because the + // storefront filters status outside the drawer. + const activeCount = activeFilterCount(filters) + (filters.status === null ? 0 : 1); return (
- - onChange({ ...filters, categoryIds: value === undefined ? [] : [value] }) - } + + + - - // Empty means no filter, not "no statuses". A multi-select cleared - // back to nothing should show everything rather than an empty table. - onChange({ ...filters, status: value.length ? value : null }) - } - options={STATUS_OPTIONS} - /> - - {hasActiveFilters(filters) && }
); } diff --git a/frontend/src/components/ActiveFilterChips.tsx b/frontend/src/components/ActiveFilterChips.tsx index 6ddbd71..32a5afe 100644 --- a/frontend/src/components/ActiveFilterChips.tsx +++ b/frontend/src/components/ActiveFilterChips.tsx @@ -1,21 +1,30 @@ import Tag from 'antd/es/tag'; import Button from 'antd/es/button'; -import type { FilterOptions } from '../api'; -import { ItemFilters, categoryPath, formatPriceRange, hasActiveFilters } from '../filters'; +import type { Category, Tag as ItemTag } from '../api'; +import { ItemFilters, categoryPath, formatPriceRange, hasActiveFilters, statusLabel } from '../filters'; type Props = Readonly<{ - options: FilterOptions | null; + categories: Category[]; + tags: ItemTag[]; filters: ItemFilters; onChange: (filters: ItemFilters) => void; onClear: () => void; + // Where status is one of the drawer's controls rather than a preset beside + // it (#169), it needs a chip too — otherwise the one filter most likely to + // empty a table is the one filter invisible without opening the drawer. + showStatus?: boolean; }>; -export default function ActiveFilterChips({ options, filters, onChange, onClear }: Props) { +export default function ActiveFilterChips({ + categories, + tags, + filters, + onChange, + onClear, + showStatus = false +}: Props) { if (!hasActiveFilters(filters)) return null; - const categories = options?.categories ?? []; - const tags = options?.tags ?? []; - const chips: { key: string; label: string; onRemove: () => void }[] = []; // Listed first so it matches the drawer's ordering, and because it is the @@ -54,6 +63,21 @@ export default function ActiveFilterChips({ options, filters, onChange, onClear }); } + if (showStatus && filters.status !== null) { + for (const status of filters.status) { + chips.push({ + key: `status-${status}`, + label: statusLabel(status), + onRemove: () => { + const rest = (filters.status ?? []).filter((value) => value !== status); + // Back to null rather than an empty list: emptying the control means + // "no status filter", not "no statuses", which would empty the table. + onChange({ ...filters, status: rest.length ? rest : null }); + } + }); + } + } + if (filters.minPriceCents !== null || filters.maxPriceCents !== null) { chips.push({ key: 'price', diff --git a/frontend/src/components/FilterDrawer.tsx b/frontend/src/components/FilterDrawer.tsx index 8ffff8c..efb7b03 100644 --- a/frontend/src/components/FilterDrawer.tsx +++ b/frontend/src/components/FilterDrawer.tsx @@ -8,17 +8,33 @@ import InputNumber from 'antd/es/input-number'; import Empty from 'antd/es/empty'; import Switch from 'antd/es/switch'; import Grid from 'antd/es/grid'; -import type { FilterOptions } from '../api'; -import { ItemFilters, buildCategoryTree, CategoryNode } from '../filters'; +import type { Category, Tag as ItemTag } from '../api'; +import { ItemFilters, ItemStatus, STATUS_OPTIONS, buildCategoryTree, CategoryNode } from '../filters'; +// One drawer for the storefront and the admin, with the sections that differ +// driven by props rather than by a second component that would drift (#169). +// What is shared is not just the markup but the phrasing of the rules — that +// categories are OR and tags are AND has to read the same on both screens or it +// stops being one rule. type Props = Readonly<{ open: boolean; onClose: () => void; - options: FilterOptions | null; + categories: Category[]; + tags: ItemTag[]; + // Bounds for the price slider, or null on a screen with no catalogue-wide + // range to draw one from, where the two number inputs stand alone. A slider + // needs real bounds: invented ones would misreport where the prices are. + priceRange: { min_cents: number; max_cents: number } | null; filters: ItemFilters; onChange: (filters: ItemFilters) => void; onClear: () => void; resultCount: number; + // Storefront only — signing in is what makes favorites mean anything. + showFavorites?: boolean; + // Admin only. The storefront keeps its three-way preset outside the drawer: + // pending is excluded from every public read, so Published and Unpublished + // are not distinctions a customer can draw. + showStatus?: boolean; }>; // `value` rather than `key`: this fed an antd `Tree`, which identifies nodes by @@ -38,6 +54,14 @@ function toTreeData(nodes: CategoryNode[]): CategoryTreeOption[] { })); } +const sectionHeading: React.CSSProperties = { + margin: '0 0 8px', + fontSize: 12, + letterSpacing: '.06em', + textTransform: 'uppercase', + opacity: 0.65 +}; + const centsToDollars = (cents: number | null): number | null => (cents === null ? null : cents / 100); const dollarsToCents = (dollars: number | null): number | null => dollars === null || Number.isNaN(dollars) ? null : Math.round(dollars * 100); @@ -45,16 +69,18 @@ const dollarsToCents = (dollars: number | null): number | null => export default function FilterDrawer({ open, onClose, - options, + categories, + tags, + priceRange, filters, onChange, onClear, - resultCount + resultCount, + showFavorites = false, + showStatus = false }: Props) { const screens = Grid.useBreakpoint(); - const categories = options?.categories ?? []; - const tags = options?.tags ?? []; - const bounds = options?.priceRange ?? { min_cents: 0, max_cents: 0 }; + const bounds = priceRange ?? { min_cents: 0, max_cents: 0 }; function selectCategories(ids: number[]) { onChange({ ...filters, categoryIds: ids }); @@ -89,8 +115,9 @@ export default function FilterDrawer({ here for their favorites should not have to scroll past the catalogue controls to find it. Shown to signed-out visitors too: switching it on prompts them to sign in, which is how they learn favorites exist. */} + {showFavorites && (
-

+

Favorites

{/* Deliberately not wrapped in a
+ )}
-

+

Categories — any of these

{categories.length ? ( @@ -137,7 +165,7 @@ export default function FilterDrawer({
-

+

Tags — must have all of these

{tags.length ? ( @@ -181,10 +209,11 @@ export default function FilterDrawer({ )}
-
-

+
+

Price

+ {priceRange && ( + )}
+ + {showStatus && ( +
+

+ Status — any of these +

+ {/* The status dimension itself rather than presets over it, which + #105's Sold / Not sold / All control was. Presets could not express + Published or Unpublished, could not isolate Reserved, and would + have grown a new button for every new question. Selecting statuses + answers all of them: Unpublished is Pending, Published is the other + three, and Not sold is everything except Sold. + + A second control for publication would have read more naturally and + reintroduced what #105 avoided — Sold and Unpublished is an + impossible pair, since a sold item is necessarily published. One + dimension cannot contradict itself. See #132. */} +