From 3890b074b3eb1735a1389e034ce32535c0895435 Mon Sep 17 00:00:00 2001 From: Joey Ballentine Date: Fri, 31 Jul 2026 12:29:02 -0500 Subject: [PATCH 1/3] fix: dataset edit mode on deployed, duplicate page shell, mobile section order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three reported bugs, all in the page layer. Dataset pages opened in edit mode on the deployed site. `useWebApi`'s argument means "allow editing even though we are deployed" — it exists for the add-model preview — so passing `IS_DEPLOYED` made being deployed the one condition that switched editing on. Dataset pages now call `useWebApi()` like every other page that renders existing content. The images notice rendered twice on dataset pages. `PageContainer` is the whole page shell (header, notice, footer), and this was the only page in the repo that nested two of them: `Page` wrapped `PageContent`, and both rendered one. Dropped the outer one and moved the not-found branch inside the remaining one so it keeps its shell. On mobile the model page put two card grids ahead of the model's own details. The page grid collapses to a single column below `lg` and renders in DOM order, and both grids lived at the bottom of the description column, so downloads and specs came after "Similar models". The sidebar is now the second grid child. Similar models is also capped at 4 (was 24) and spans the full width under both columns rather than continuing the description column. The card grid is `auto-fill minmax(280px, 1fr)`, so four is exactly one row at page width. Tests render both pages against the deployed origin. `vite-plugin-svgr` needed an explicit `include`: it only claims `*.svg?react` by default, and the app imports plain `*.svg`, so the logo resolved to a URL string and any page-level render died on it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TpogQiNgM8Lg8PTTfTGSNk --- src/pages/datasets/[id].tsx | 38 ++++++----- src/pages/models/[id].tsx | 94 ++++++++++++++++----------- tests/pages/dataset-page.test.tsx | 101 ++++++++++++++++++++++++++++++ tests/pages/model-page.test.tsx | 91 +++++++++++++++++++++++++++ vitest.config.ts | 6 +- 5 files changed, 275 insertions(+), 55 deletions(-) create mode 100644 tests/pages/dataset-page.test.tsx create mode 100644 tests/pages/model-page.test.tsx diff --git a/src/pages/datasets/[id].tsx b/src/pages/datasets/[id].tsx index 98ed0c49..efdbc46c 100644 --- a/src/pages/datasets/[id].tsx +++ b/src/pages/datasets/[id].tsx @@ -124,7 +124,10 @@ function PageContent({ datasetId, staticDatasetData }: Props) { const dataset = datasetData.get(realDatasetId); - const { webApi, editMode } = useWebApi(IS_DEPLOYED); + // No override argument. `useWebApi`'s parameter means "allow editing even + // though we're deployed", so passing `IS_DEPLOYED` — as this did — made + // being deployed the one condition that switched edit mode on. + const { webApi, editMode } = useWebApi(); const { updateDatasetProperty } = useUpdateDataset(webApi, realDatasetId); const authors = useMemo(() => { @@ -166,15 +169,17 @@ function PageContent({ datasetId, staticDatasetData }: Props) { if (!dataset) { return ( -
-

Dataset not found

- - Back to datasets - -
+ +
+

Dataset not found

+ + Back to datasets + +
+ ); } @@ -383,14 +388,15 @@ function PageContent({ datasetId, staticDatasetData }: Props) { ); } +// `PageContent` renders its own `PageContainer` — including the not-found +// branch — so this must not wrap it in a second one. The container is the whole +// page shell: header, site notice and footer. Nesting rendered all three twice. export default function Page({ datasetId, staticDatasetData }: Props) { return ( - - - + ); } diff --git a/src/pages/models/[id].tsx b/src/pages/models/[id].tsx index 98e452a1..6df4d515 100644 --- a/src/pages/models/[id].tsx +++ b/src/pages/models/[id].tsx @@ -51,7 +51,10 @@ import { } from '../../lib/util'; import { validateModel } from '../../lib/validate-model'; -const MAX_SIMILAR_MODELS = 12 * 2; +// One full row. The card grid is `auto-fill minmax(280px, 1fr)`, which lands on +// four columns at full page width, so four suggestions fill the strip across the +// bottom without leaving a ragged second row. +const MAX_SIMILAR_MODELS = 4; interface Params extends ParsedUrlQuery { id: ModelId; @@ -407,6 +410,8 @@ export default function Page({ return [...collectionData].filter(([, collection]) => collection.models.includes(modelId)).map(([id]) => id); }, [modelId, collectionData]); + const hasRelated = collections.length > 0 || similar.length > 0; + const router = useRouter(); const runModelValidation = useCallback(async () => { @@ -646,44 +651,14 @@ export default function Page({ - - {/* Related models live in this column so it always has - body: most descriptions are short, and the sidebar is - long, which otherwise left a tall void beside it. */} - {collections.length > 0 && ( -
-

- Collections that include this model -

- -
- )} - - {similar.length > 0 && ( -
-

Similar models

- {editMode && similarWithScores.length > 0 && ( -
- Show scores{' '} - 
-                                            {similarWithScores
-                                                .map(({ id, score }) => `${score.toFixed(2).padEnd(6)} ${id}`)
-                                                .join('\n')}
-
-
- )} - -
- )} - {/* Right column: Sidebar */} + + {/* Sidebar. Deliberately the second grid child rather than + the last: below `lg` the grid collapses to one column and + renders in DOM order, so anything after this would push + downloads and specs below it. The related-model grids + used to live in the column above, which put two full + card grids ahead of the model's own details on a phone. */}
+ + {/* A full-width strip under both columns rather than more + body for the description column, so the card grids get + the whole page width and read as a footer to the page + instead of a continuation of the article. */} + {hasRelated && ( +
+ {collections.length > 0 && ( +
+

+ Collections that include this model +

+ +
+ )} + + {similar.length > 0 && ( +
+

+ Similar models +

+ {editMode && similarWithScores.length > 0 && ( +
+ Show scores{' '} + 
+                                                {similarWithScores
+                                                    .map(({ id, score }) => `${score.toFixed(2).padEnd(6)} ${id}`)
+                                                    .join('\n')}
+
+
+ )} + +
+ )} +
+ )} {editMode && (
diff --git a/tests/pages/dataset-page.test.tsx b/tests/pages/dataset-page.test.tsx new file mode 100644 index 00000000..5335e661 --- /dev/null +++ b/tests/pages/dataset-page.test.tsx @@ -0,0 +1,101 @@ +/** + * `IS_DEPLOYED` is a module-level constant read from `location.host` at import + * time, so the only way to exercise the deployed build is to give the whole + * file the deployed origin before anything is imported. + * + * @vitest-environment-options { "url": "https://openmodeldb.info/" } + */ +import { render, screen } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { WebApiProvider } from '../../src/lib/hooks/use-web-api'; +import { Dataset, DatasetId } from '../../src/lib/schema'; + +vi.mock('next/router', () => ({ + useRouter: () => ({ push: vi.fn(), query: {}, asPath: '/', isReady: true, events: { on: vi.fn(), off: vi.fn() } }), +})); + +const fixture = vi.hoisted(() => ({ + datasetId: 'div2k', + dataset: { + name: 'DIV2K', + author: [], + license: null, + tags: [], + description: 'A dataset.', + date: '2024-01-01', + url: 'https://example.com/div2k', + images: [], + }, +})); + +// The real one talks to `/api/*` over fetch. The page only cares that *some* +// api resolved, which is precisely the condition that used to unlock editing. +// `useDatasets` overwrites its static props with whatever this returns, so the +// datasets collection has to actually contain the fixture. +vi.mock('../../src/lib/web-api', () => { + const collection = (entries: [string, unknown][] = []) => ({ + get: vi.fn(), + getAll: vi.fn(() => Promise.resolve(new Map(entries))), + update: vi.fn(() => Promise.resolve()), + delete: vi.fn(() => Promise.resolve()), + changeId: vi.fn(() => Promise.resolve()), + }); + + return { + getWebApi: vi.fn(() => + Promise.resolve({ + models: collection(), + users: collection(), + tags: collection(), + tagCategories: collection(), + architectures: collection(), + collections: collection(), + datasets: collection([[fixture.datasetId, fixture.dataset]]), + }) + ), + startListeningForUpdates: vi.fn(), + addUpdateListener: vi.fn(() => vi.fn()), + }; +}); + +const datasetId = fixture.datasetId as DatasetId; +const dataset = fixture.dataset as unknown as Dataset; + +async function renderDatasetPage() { + const { default: Page } = await import('../../src/pages/datasets/[id]'); + + return render( + + + + ); +} + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe('dataset page on the deployed site', () => { + it('stays read-only', async () => { + // The page used to pass `IS_DEPLOYED` as `useWebApi`'s override, which + // reads as "allow editing despite deployment" — so being deployed was + // the very thing that turned edit mode on. + await renderDatasetPage(); + + expect(await screen.findByText('DIV2K')).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: /delete dataset/i })).not.toBeInTheDocument(); + expect(screen.queryByRole('button', { name: /clipboard/i })).not.toBeInTheDocument(); + }); + + it('shows the site notice exactly once', async () => { + // The page nested two `PageContainer`s, and the notice lives in the + // shell, so every dataset page rendered the whole shell twice. + await renderDatasetPage(); + + expect(await screen.findByText('DIV2K')).toBeInTheDocument(); + expect(screen.getAllByRole('status')).toHaveLength(1); + }); +}); diff --git a/tests/pages/model-page.test.tsx b/tests/pages/model-page.test.tsx new file mode 100644 index 00000000..5e236a3d --- /dev/null +++ b/tests/pages/model-page.test.tsx @@ -0,0 +1,91 @@ +import { render, screen } from '@testing-library/react'; +import { describe, expect, it, vi } from 'vitest'; +import { WebApiProvider } from '../../src/lib/hooks/use-web-api'; +import { Model, ModelId } from '../../src/lib/schema'; + +vi.mock('next/router', () => ({ + useRouter: () => ({ push: vi.fn(), query: {}, asPath: '/', isReady: true, events: { on: vi.fn(), off: vi.fn() } }), +})); + +const fixture = vi.hoisted(() => { + const model = (name: string) => ({ + name, + author: [], + license: null, + tags: [], + description: 'A model.', + date: '2024-01-01', + architecture: 'esrgan', + size: null, + scale: 4, + inputChannels: 3, + outputChannels: 3, + resources: [], + images: [], + }); + + return { main: ['4x-Main', model('4x-Main')], other: ['4x-Other', model('4x-Other')] } as const; +}); + +vi.mock('../../src/lib/web-api', () => { + const collection = (entries: readonly (readonly [string, unknown])[] = []) => ({ + get: vi.fn(), + getAll: vi.fn(() => Promise.resolve(new Map(entries))), + update: vi.fn(() => Promise.resolve()), + delete: vi.fn(() => Promise.resolve()), + changeId: vi.fn(() => Promise.resolve()), + }); + + return { + getWebApi: vi.fn(() => + Promise.resolve({ + models: collection([fixture.main, fixture.other]), + users: collection(), + tags: collection(), + tagCategories: collection(), + architectures: collection(), + collections: collection(), + datasets: collection(), + }) + ), + startListeningForUpdates: vi.fn(), + addUpdateListener: vi.fn(() => vi.fn()), + }; +}); + +const mainId = fixture.main[0] as ModelId; +const otherId = fixture.other[0] as ModelId; + +async function renderModelPage() { + const { default: Page } = await import('../../src/pages/models/[id]'); + + return render( + + + } + staticSimilar={[otherId]} + /> + + ); +} + +describe('model page layout', () => { + it('puts the model details ahead of the related-model grids', async () => { + // Below `lg` the page grid collapses to a single column and renders in + // DOM order, so this ordering *is* the mobile layout: downloads and + // specs have to come before "Similar models", not after it. + await renderModelPage(); + + const specs = await screen.findByText('Architecture'); + const similar = await screen.findByText('Similar models'); + + expect(specs.compareDocumentPosition(similar) & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy(); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index fe372fee..1f304d0c 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -5,7 +5,11 @@ export default defineConfig({ // The app imports SVGs as React components (see the `@svgr/webpack` rule in // next.config.js). Without the equivalent here, anything that renders the // header or a download button fails on the logo import. - plugins: [svgr()], + // + // `include` is required: the plugin only claims `*.svg?react` by default, + // and the app imports plain `*.svg`. Without it the import resolves to the + // asset URL and React tries to render the string as a component. + plugins: [svgr({ include: '**/*.svg' })], // tsconfig says `jsx: preserve` because Next does its own transform, and // Vite honours that — which leaves raw JSX in the output and every .tsx From d1660adb3d059fde0bedfabe00885d555858ceb4 Mon Sep 17 00:00:00 2001 From: Joey Ballentine Date: Fri, 31 Jul 2026 12:29:15 -0500 Subject: [PATCH 2/3] test: guard edit mode against ever unlocking on a deployed build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two suites, because neither alone covers "any page". use-web-api covers the mechanism: both the `(!IS_DEPLOYED || override)` gate and `useEditModeToggle`'s separate check, including that the api object itself is withheld and not just the boolean. Each case awaits the provider's own promise and pairs its assertion with an overriding caller in the same render. On the first tick `webApi` is undefined and `editMode` is false, which is also what a correct deployed build looks like — so a `waitFor` here passes before the api ever arrives, and stays green with the deployment check deleted. The control makes "still locked" and "hasn't loaded yet" distinguishable. For the same reason the toggle case toggles twice: `enabled` starts true, so one flip only ever turns editing off. edit-mode-guard covers the reach: it scans every call site under src/ and requires the argument to be absent or listed as a documented exception. Rendering every page against the deployed origin is impractical and would only ever cover the pages that exist today; the scan covers a page added later the moment it is written. The exception list is real rather than a loophole — add-model and add-dataset do unlock editing when deployed, on purpose. That is the "propose a contribution" flow, and there `getWebApi` returns session-storage-backed collections, so the edits are a local scratch copy that cannot reach the database. Verified by mutation: reintroducing the dataset bug fails 4 tests, and breaking each gate in use-web-api.tsx separately fails 3 and 2. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TpogQiNgM8Lg8PTTfTGSNk --- tests/lib/edit-mode-guard.test.ts | 103 +++++++++++++++++++++ tests/lib/use-web-api.test.tsx | 147 ++++++++++++++++++++++++++++++ 2 files changed, 250 insertions(+) create mode 100644 tests/lib/edit-mode-guard.test.ts create mode 100644 tests/lib/use-web-api.test.tsx diff --git a/tests/lib/edit-mode-guard.test.ts b/tests/lib/edit-mode-guard.test.ts new file mode 100644 index 00000000..714e591c --- /dev/null +++ b/tests/lib/edit-mode-guard.test.ts @@ -0,0 +1,103 @@ +import { readFileSync, readdirSync } from 'node:fs'; +import { join, relative, sep } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +/** + * Rendering every page against the deployed origin is not practical, and it + * would only ever cover the pages that exist today. This scans the source + * instead, so a page added next month is covered the moment it is written. + * + * The rule being enforced: `useWebApi`'s argument means "allow editing even + * though we are deployed". Passing anything truthy is therefore a deliberate + * decision that has to be justified here, not something a page picks up by + * copying its neighbour — which is exactly how the dataset page ended up + * calling `useWebApi(IS_DEPLOYED)` and unlocking editing on the live site. + */ + +const SRC = join(__dirname, '..', '..', 'src'); + +/** + * Call sites allowed to unlock edit mode on the deployed site, and why. + * + * Both entries are the "propose a contribution" flow: they build a model or + * dataset that does not exist yet and render it editably so a contributor can + * check it over and open a GitHub issue. On the deployed site `getWebApi` + * returns session-storage-backed collections, so these edit a local scratch + * copy and cannot reach the real database. + */ +const ALLOWED = new Map([ + ['pages/add-model.tsx', 'IS_DEPLOYED'], + ['pages/add-dataset.tsx', 'IS_DEPLOYED'], + // Threaded from a prop that only `OMDB_ADDMODEL_DUMMY` sets. + ['pages/models/[id].tsx', 'editModeOverride'], +]); + +/** Where `useWebApi` is declared — its signature is not a call site. */ +const DEFINITION = 'lib/hooks/use-web-api.tsx'; + +function sourceFiles(dir: string): string[] { + return readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { + const full = join(dir, entry.name); + if (entry.isDirectory()) return sourceFiles(full); + return /\.tsx?$/.test(entry.name) ? [full] : []; + }); +} + +interface CallSite { + file: string; + argument: string; +} + +function callSites(): CallSite[] { + const found: CallSite[] = []; + + for (const file of sourceFiles(SRC)) { + const id = relative(SRC, file).split(sep).join('/'); + if (id === DEFINITION) continue; + + const source = readFileSync(file, 'utf8'); + for (const match of source.matchAll(/\buseWebApi\(([^)]*)\)/g)) { + found.push({ file: id, argument: match[1].trim() }); + } + } + + return found; +} + +describe('edit mode on the deployed site', () => { + it('finds the call sites at all', () => { + // Guards the guard: a regex that silently matches nothing would make + // every assertion below pass without checking anything. + const sites = callSites(); + + expect(sites.length).toBeGreaterThan(5); + expect(sites.some(({ file }) => file === 'pages/models/[id].tsx')).toBe(true); + }); + + it('is not unlocked by any page outside the documented exceptions', () => { + const offenders = callSites() + .filter(({ argument }) => argument !== '') + .filter(({ file, argument }) => ALLOWED.get(file) !== argument); + + expect(offenders).toEqual([]); + }); + + it('keeps the dataset page read-only', () => { + // The regression this suite was written for. + const sites = callSites().filter(({ file }) => file === 'pages/datasets/[id].tsx'); + + expect(sites).not.toEqual([]); + for (const { argument } of sites) { + expect(argument).toBe(''); + } + }); + + it('never passes IS_DEPLOYED from a page that renders existing content', () => { + // `useWebApi(IS_DEPLOYED)` reads as "unlock when deployed", which is + // only ever right for the add-* flows. On a page that displays + // something already in the database it is always a bug. + const offenders = callSites().filter(({ file, argument }) => argument === 'IS_DEPLOYED' && !ALLOWED.has(file)); + + expect(offenders).toEqual([]); + }); +}); diff --git a/tests/lib/use-web-api.test.tsx b/tests/lib/use-web-api.test.tsx new file mode 100644 index 00000000..68828819 --- /dev/null +++ b/tests/lib/use-web-api.test.tsx @@ -0,0 +1,147 @@ +import { act, renderHook } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +/** + * `IS_DEPLOYED` is a module-level constant, so each case has to load + * `use-web-api` against a freshly mocked `site-data`. + */ +async function load(isDeployed: boolean) { + vi.resetModules(); + + const getWebApi = vi.fn(() => Promise.resolve({ tags: { update: vi.fn(() => Promise.resolve()) } })); + + vi.doMock('../../src/lib/site-data', () => ({ + IS_DEPLOYED: isDeployed, + SITE_URL: 'https://openmodeldb.info', + ADSENSE_PUBLISHER_ID: 'test', + })); + vi.doMock('../../src/lib/web-api', () => ({ + getWebApi, + startListeningForUpdates: vi.fn(), + addUpdateListener: vi.fn(() => vi.fn()), + })); + + return { ...(await import('../../src/lib/hooks/use-web-api')), getWebApi }; +} + +/** + * Await the exact promise the provider awaited, so assertions run against the + * settled state. + * + * Without this, `webApi` is still undefined and `editMode` still false on the + * first tick — which is also what a correct deployed build looks like — so a + * `waitFor` would pass before the api ever arrived and the test would hold + * even with the deployment check deleted. + */ +async function settle(getWebApi: { mock: { results: { value: unknown }[] } }) { + await act(async () => { + await getWebApi.mock.results[0]?.value; + }); +} + +afterEach(() => { + vi.doUnmock('../../src/lib/site-data'); + vi.doUnmock('../../src/lib/web-api'); +}); + +describe('useWebApi when deployed', () => { + it('refuses edit mode to a caller that does not override', async () => { + const { WebApiProvider, useWebApi, getWebApi } = await load(true); + + // Both callers in one render, against one loaded api. The overriding + // one is the control: it proves the api really did arrive, so the + // plain one staying locked is a refusal rather than a slow start. + const { result } = renderHook(() => ({ plain: useWebApi(), overriding: useWebApi(true) }), { + wrapper: WebApiProvider, + }); + await settle(getWebApi); + + expect(result.current.overriding.editMode).toBe(true); + expect(result.current.plain.editMode).toBe(false); + }); + + it('withholds the api itself, not just the flag', async () => { + // Anything that slipped past the flag still has nothing to write with. + const { WebApiProvider, useWebApi, getWebApi } = await load(true); + + const { result } = renderHook(() => ({ plain: useWebApi(), overriding: useWebApi(true) }), { + wrapper: WebApiProvider, + }); + await settle(getWebApi); + + expect(result.current.overriding.webApi).toBeDefined(); + expect(result.current.plain.webApi).toBeUndefined(); + }); + + it('allows edit mode only for a caller that explicitly overrides', async () => { + // The add-model / add-dataset flow. On the deployed site `getWebApi` + // hands back session-storage-backed collections, so this edits a local + // scratch copy and never reaches the real database. + const { WebApiProvider, useWebApi, getWebApi } = await load(true); + + const { result } = renderHook(() => useWebApi(true), { wrapper: WebApiProvider }); + await settle(getWebApi); + + expect(result.current.editMode).toBe(true); + }); + + it('never offers the header toggle', async () => { + const { WebApiProvider, useWebApi, useEditModeToggle, getWebApi } = await load(true); + + const { result } = renderHook(() => ({ toggle: useEditModeToggle(), control: useWebApi(true) }), { + wrapper: WebApiProvider, + }); + await settle(getWebApi); + + expect(result.current.control.editMode).toBe(true); + expect(result.current.toggle.editModeAvailable).toBe(false); + expect(result.current.toggle.editMode).toBe(false); + }); + + it('stays read-only however the toggle is driven', async () => { + // `toggleEditMode` flips the shared `enabled` flag, which is also what + // `useWebApi` reads. Being deployed has to win either way round. + // + // Twice, not once: `enabled` starts true, so a single toggle only ever + // turns editing *off* and would pass even with the deployment check + // gone. The second toggle is the one that asks the real question. + const { WebApiProvider, useWebApi, useEditModeToggle, getWebApi } = await load(true); + + const { result } = renderHook(() => ({ toggle: useEditModeToggle(), page: useWebApi() }), { + wrapper: WebApiProvider, + }); + await settle(getWebApi); + + for (let i = 0; i < 2; i++) { + act(() => result.current.toggle.toggleEditMode()); + + expect(result.current.toggle.editMode).toBe(false); + expect(result.current.page.editMode).toBe(false); + expect(result.current.page.webApi).toBeUndefined(); + } + }); +}); + +describe('useWebApi when running locally', () => { + it('enables edit mode without any override', async () => { + // The positive control for the suite above: same harness, same mocked + // api, only `IS_DEPLOYED` differs. + const { WebApiProvider, useWebApi, getWebApi } = await load(false); + + const { result } = renderHook(() => useWebApi(), { wrapper: WebApiProvider }); + await settle(getWebApi); + + expect(result.current.editMode).toBe(true); + expect(result.current.webApi).toBeDefined(); + }); + + it('offers the header toggle', async () => { + const { WebApiProvider, useEditModeToggle, getWebApi } = await load(false); + + const { result } = renderHook(() => useEditModeToggle(), { wrapper: WebApiProvider }); + await settle(getWebApi); + + expect(result.current.editModeAvailable).toBe(true); + expect(result.current.editMode).toBe(true); + }); +}); From aa27baf7f138a4ffb7baa3abe5e6679dea1b921e Mon Sep 17 00:00:00 2001 From: Joey Ballentine Date: Fri, 31 Jul 2026 12:35:02 -0500 Subject: [PATCH 3/3] fix: space out the tag buttons in the advanced tag selector Each category's buttons sat in a bare
. They are inline-flex boxes and JSX drops the whitespace between them, so a category rendered as one unbroken strip of touching pills. The simple selector already solved this with `.groupTags`; the advanced one never got the equivalent, and now uses the same 0.375rem gap. Also covers the selector with tests, which it had none of despite being the site's main filtering control: single-select and auto-deselect in simple mode, the required/forbidden/any cycle in advanced mode, clear-all, the datasets context filter, and the automatic switch to advanced when a selection has no simple representation. Tag ids and names are read from data/tags.json rather than hard-coded, so editing the tag list does not break the suite. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TpogQiNgM8Lg8PTTfTGSNk --- src/elements/tag-selector.module.scss | 10 ++ src/elements/tag-selector.tsx | 2 +- tests/components/tag-selector.test.tsx | 235 +++++++++++++++++++++++++ 3 files changed, 246 insertions(+), 1 deletion(-) create mode 100644 tests/components/tag-selector.test.tsx diff --git a/src/elements/tag-selector.module.scss b/src/elements/tag-selector.module.scss index 6e4ce09c..33850ec3 100644 --- a/src/elements/tag-selector.module.scss +++ b/src/elements/tag-selector.module.scss @@ -63,6 +63,16 @@ gap: 0.375rem; } +// The advanced selector's equivalent of `.groupTags`, and deliberately the same +// gap. Without it the buttons are inline-flex boxes in a plain block, and JSX +// drops the whitespace between them, so a whole category renders as one +// unbroken strip of touching pills. +.advancedTags { + display: flex; + flex-flow: row wrap; + gap: 0.375rem; +} + .tagButton { font: inherit; cursor: pointer; diff --git a/src/elements/tag-selector.tsx b/src/elements/tag-selector.tsx index 5dc9fe9f..f95c23fe 100644 --- a/src/elements/tag-selector.tsx +++ b/src/elements/tag-selector.tsx @@ -163,7 +163,7 @@ function AdvancedTagSelector({ selection, onChange, context = 'models' }: TagSel return (

{category.name}

-
+
{category.tags.map((tagId) => { const tag = tagData.get(tagId); const state = getState(tagId, selection); diff --git a/tests/components/tag-selector.test.tsx b/tests/components/tag-selector.test.tsx new file mode 100644 index 00000000..2ed8ee97 --- /dev/null +++ b/tests/components/tag-selector.test.tsx @@ -0,0 +1,235 @@ +import { render, screen, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { useState } from 'react'; +import { describe, expect, it, vi } from 'vitest'; +import { TagSelector, TagSelectorStyle } from '../../src/elements/tag-selector'; +import { TagId } from '../../src/lib/schema'; +import { STATIC_TAG_CATEGORY_DATA, STATIC_TAG_DATA } from '../../src/lib/static-data'; +import { SelectionState, TagSelection } from '../../src/lib/tag-condition'; + +/** + * Derived from the real tag data rather than hard-coded, so editing + * data/tags.json does not break these tests. + */ +function tagsOf(categoryId: string): { id: TagId; name: string }[] { + const category = STATIC_TAG_CATEGORY_DATA.get(categoryId as never); + if (!category) throw new Error(`no such tag category: ${categoryId}`); + + return category.tags.map((id) => ({ id, name: STATIC_TAG_DATA.get(id)?.name ?? id })); +} + +const SUBJECT = tagsOf('subject'); +const ARCHITECTURE = tagsOf('architecture'); +const DATASET = tagsOf('dataset'); + +const EMPTY: TagSelection = new Map(); + +function required(...ids: TagId[]): TagSelection { + return new Map(ids.map((id) => [id, SelectionState.Required])); +} + +/** Reports what the selector asked for without re-rendering it. */ +function renderStatic(selection: TagSelection = EMPTY, context?: 'models' | 'datasets') { + const onChange = vi.fn<(selection: TagSelection, style: TagSelectorStyle) => void>(); + + render( + + ); + + return { onChange, user: userEvent.setup() }; +} + +/** Feeds `onChange` back in, so multi-click flows behave like the real page. */ +function renderLive(initial: TagSelection = EMPTY) { + const seen: TagSelection[] = []; + + function Harness() { + const [selection, setSelection] = useState(initial); + + return ( + { + seen.push(next); + setSelection(next); + }} + /> + ); + } + + render(); + + return { seen, user: userEvent.setup() }; +} + +const toAdvanced = (user: ReturnType) => + user.click(screen.getByRole('button', { name: /advanced tag selector/i })); + +describe('TagSelector in simple mode', () => { + it('starts simple, showing everything and no tag chosen', () => { + renderStatic(); + + expect(screen.getByRole('button', { name: /all models/i })).toHaveAttribute('aria-pressed', 'true'); + expect(screen.getByRole('button', { name: /advanced tag selector/i })).toBeInTheDocument(); + }); + + it('hides the categories that only make sense in advanced mode', () => { + // Architecture is `simple: false` — 33 buttons that would swamp the + // handful of subject and purpose options. + renderStatic(); + + expect(screen.getByRole('button', { name: SUBJECT[0].name })).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: ARCHITECTURE[0].name })).not.toBeInTheDocument(); + }); + + it('requires the tag that was clicked', async () => { + const { onChange, user } = renderStatic(); + + await user.click(screen.getByRole('button', { name: SUBJECT[1].name })); + + expect(onChange).toHaveBeenCalledWith(required(SUBJECT[1].id), 'simple'); + }); + + it('replaces the previous tag rather than adding to it', async () => { + // Simple mode is single-select across every group, not per group. + const { seen, user } = renderLive(required(SUBJECT[0].id)); + + await user.click(screen.getByRole('button', { name: SUBJECT[1].name })); + + expect(seen.at(-1)).toEqual(required(SUBJECT[1].id)); + }); + + it('clears the selection through "All models"', async () => { + const { onChange, user } = renderStatic(required(SUBJECT[0].id)); + + await user.click(screen.getByRole('button', { name: /all models/i })); + + expect(onChange).toHaveBeenCalledWith(EMPTY, 'simple'); + }); + + it('ignores a click on the tag that is already chosen', async () => { + const { onChange, user } = renderStatic(required(SUBJECT[0].id)); + + await user.click(screen.getByRole('button', { name: SUBJECT[0].name })); + + expect(onChange).not.toHaveBeenCalled(); + }); + + it('switches itself to advanced for a selection it cannot show', async () => { + // A forbidden tag has no simple representation. Staying simple would + // display a filter that silently disagrees with the results. + render( + + ); + + expect(await screen.findByRole('button', { name: /simple tag selector/i })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: ARCHITECTURE[0].name })).toBeInTheDocument(); + }); +}); + +describe('TagSelector in advanced mode', () => { + it('reveals the categories simple mode holds back', async () => { + const { user } = renderStatic(); + + await toAdvanced(user); + + expect(screen.getByRole('button', { name: ARCHITECTURE[0].name })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: SUBJECT[0].name })).toBeInTheDocument(); + }); + + it('cycles a tag through required, forbidden and back to any', async () => { + const { seen, user } = renderLive(); + + await toAdvanced(user); + const tag = () => screen.getByRole('button', { name: ARCHITECTURE[0].name }); + + await user.click(tag()); + expect(seen.at(-1)).toEqual(new Map([[ARCHITECTURE[0].id, SelectionState.Required]])); + + await user.click(tag()); + expect(seen.at(-1)).toEqual(new Map([[ARCHITECTURE[0].id, SelectionState.Forbidden]])); + + await user.click(tag()); + expect(seen.at(-1)?.size).toBe(0); + }); + + it('keeps several tags at once, unlike simple mode', async () => { + const { seen, user } = renderLive(); + + await toAdvanced(user); + await user.click(screen.getByRole('button', { name: ARCHITECTURE[0].name })); + await user.click(screen.getByRole('button', { name: ARCHITECTURE[1].name })); + + expect(seen.at(-1)).toEqual(required(ARCHITECTURE[0].id, ARCHITECTURE[1].id)); + }); + + it('marks a required tag as pressed', async () => { + // No mode switch needed: an architecture tag has no simple + // representation, so the selector opens in advanced mode already. + renderStatic(required(ARCHITECTURE[0].id)); + + expect(await screen.findByRole('button', { name: ARCHITECTURE[0].name })).toHaveAttribute( + 'aria-pressed', + 'true' + ); + }); + + it('offers "Clear all tags" only when there is something to clear', async () => { + const { user } = renderStatic(); + + await toAdvanced(user); + + expect(screen.getByRole('button', { name: /clear all tags/i })).toBeDisabled(); + }); + + it('clears everything at once', async () => { + // Opens in advanced mode, as above. + const { onChange, user } = renderStatic(required(ARCHITECTURE[0].id, SUBJECT[0].id)); + + await user.click(await screen.findByRole('button', { name: /clear all tags/i })); + + expect(onChange).toHaveBeenCalledWith(EMPTY, 'simple'); + }); + + it('groups each category into a single spacing container', async () => { + // jsdom does no layout, so the gap itself is unmeasurable here. What is + // checkable is the thing that was actually missing: the buttons sat in + // a bare
, and since they are inline-flex and JSX drops the + // whitespace between them, a whole category rendered as one unbroken + // strip of touching pills. + const { user } = renderStatic(); + + await toAdvanced(user); + const button = screen.getByRole('button', { name: ARCHITECTURE[0].name }); + const container = button.parentElement; + + expect(container).toHaveClass('advancedTags'); + expect(within(container as HTMLElement).getAllByRole('button').length).toBe(ARCHITECTURE.length); + }); +}); + +describe('TagSelector on the datasets page', () => { + it('shows dataset tags and nothing else', () => { + renderStatic(EMPTY, 'datasets'); + + expect(screen.getByRole('button', { name: /all datasets/i })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: DATASET[0].name })).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: SUBJECT[0].name })).not.toBeInTheDocument(); + }); + + it('keeps model categories out of advanced mode too', async () => { + const { user } = renderStatic(EMPTY, 'datasets'); + + await toAdvanced(user); + + expect(screen.getByRole('button', { name: DATASET[0].name })).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: ARCHITECTURE[0].name })).not.toBeInTheDocument(); + }); +});