diff --git a/.changeset/tidy-pandas-repeat.md b/.changeset/tidy-pandas-repeat.md new file mode 100644 index 0000000000..ccac80e7e8 --- /dev/null +++ b/.changeset/tidy-pandas-repeat.md @@ -0,0 +1,5 @@ +--- +"emdash": patch +--- + +Fixes the content editor recomputing taxonomy term usage counts every time it opens. The editor's taxonomy picker never shows counts, but it shares the terms endpoint with the Taxonomies settings page, which does — so opening an entry aggregated the whole content–term assignment table once per applicable taxonomy. `GET /_emdash/api/taxonomies/:name/terms` now takes an `includeCounts` query param (default `true`, so existing callers are unaffected); pass `includeCounts=false` to skip the aggregate, and `count` is then omitted from each term in the response. diff --git a/packages/admin/src/components/TaxonomyManager.tsx b/packages/admin/src/components/TaxonomyManager.tsx index 872375544b..fb99a2b7d4 100644 --- a/packages/admin/src/components/TaxonomyManager.tsx +++ b/packages/admin/src/components/TaxonomyManager.tsx @@ -749,8 +749,10 @@ export function TaxonomyManager({ taxonomyName }: TaxonomyManagerProps) { queryFn: () => fetchTaxonomyDef(taxonomyName), }); + // The count mode belongs in the key: the editor's taxonomy picker reads the + // same endpoint without counts, and this page renders them. const { data: terms = [], isLoading: termsLoading } = useQuery({ - queryKey: ["taxonomy-terms", taxonomyName, activeLocale], + queryKey: ["taxonomy-terms", taxonomyName, activeLocale, { includeCounts: true }], queryFn: () => fetchTerms(taxonomyName, { locale: activeLocale }), }); diff --git a/packages/admin/src/components/TaxonomySidebar.tsx b/packages/admin/src/components/TaxonomySidebar.tsx index 03a2773d9e..bbee14370b 100644 --- a/packages/admin/src/components/TaxonomySidebar.tsx +++ b/packages/admin/src/components/TaxonomySidebar.tsx @@ -78,10 +78,13 @@ export function useHasApplicableTaxonomies(collection: string): boolean { /** * Fetch terms for a taxonomy, scoped to the entry's locale so only the matching - * translation variants are offered. + * translation variants are offered. The picker shows no usage counts, so it + * opts out of the per-collection count aggregate the endpoint runs by default. */ async function fetchTerms(taxonomyName: string, locale?: string): Promise { - const res = await apiFetch(withLocale(`/_emdash/api/taxonomies/${taxonomyName}/terms`, locale)); + const res = await apiFetch( + withLocale(`/_emdash/api/taxonomies/${taxonomyName}/terms?includeCounts=false`, locale), + ); const data = await parseApiResponse<{ terms: TaxonomyTerm[] }>( res, i18n._(msg`Failed to fetch terms`), @@ -334,8 +337,10 @@ function TaxonomySection({ const [newCategoryLabel, setNewCategoryLabel] = React.useState(""); const [showCategoryInput, setShowCategoryInput] = React.useState(false); + // The count mode belongs in the key: the Taxonomies settings page reads the + // same endpoint with counts and must not be served this count-free list. const { data: terms = EMPTY_TERMS } = useQuery({ - queryKey: ["taxonomy-terms", taxonomy.name, entryLocale], + queryKey: ["taxonomy-terms", taxonomy.name, entryLocale, { includeCounts: false }], queryFn: () => fetchTerms(taxonomy.name, entryLocale), }); diff --git a/packages/admin/tests/components/TaxonomySidebar.test.tsx b/packages/admin/tests/components/TaxonomySidebar.test.tsx index 0a0a223301..0f758eb2c3 100644 --- a/packages/admin/tests/components/TaxonomySidebar.test.tsx +++ b/packages/admin/tests/components/TaxonomySidebar.test.tsx @@ -86,21 +86,22 @@ function mockApiFetch({ } = {}) { vi.mocked(apiFetch).mockImplementation((url: string | URL | Request, init?: RequestInit) => { const urlString = typeof url === "string" ? url : url instanceof URL ? url.toString() : url.url; + const path = new URL(urlString, "http://localhost").pathname; const method = init?.method ?? "GET"; - if (method === "GET" && urlString === "/_emdash/api/taxonomies") { + if (method === "GET" && path === "/_emdash/api/taxonomies") { return dataResponse({ taxonomies }); } - if (method === "GET" && urlString === "/_emdash/api/taxonomies/tags/terms") { + if (method === "GET" && path === "/_emdash/api/taxonomies/tags/terms") { return dataResponse({ terms }); } - if (method === "GET" && urlString === "/_emdash/api/taxonomies/categories/terms") { + if (method === "GET" && path === "/_emdash/api/taxonomies/categories/terms") { return dataResponse({ terms }); } - if (method === "GET" && urlString === "/_emdash/api/content/products/entry_1/terms/tags") { + if (method === "GET" && path === "/_emdash/api/content/products/entry_1/terms/tags") { return dataResponse({ terms: entryTerms }); } diff --git a/packages/admin/tests/components/taxonomy-term-cache.test.tsx b/packages/admin/tests/components/taxonomy-term-cache.test.tsx new file mode 100644 index 0000000000..9b4cd18176 --- /dev/null +++ b/packages/admin/tests/components/taxonomy-term-cache.test.tsx @@ -0,0 +1,113 @@ +import { Toasty } from "@cloudflare/kumo"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import * as React from "react"; +import { describe, it, expect, vi, beforeEach } from "vitest"; + +import { TaxonomyManager } from "../../src/components/TaxonomyManager"; +import { TaxonomySidebar } from "../../src/components/TaxonomySidebar"; +import { render } from "../utils/render.tsx"; + +vi.mock("../../src/lib/api/client.js", async () => { + const actual = await vi.importActual("../../src/lib/api/client.js"); + return { + ...actual, + apiFetch: vi.fn(), + }; +}); + +import { apiFetch } from "../../src/lib/api/client.js"; + +const categoriesTaxonomy = { + id: "tax_categories", + name: "categories", + label: "Categories", + labelSingular: "Category", + hierarchical: true, + collections: ["posts"], +}; + +const terms = [ + { id: "1", name: "tech", slug: "tech", label: "Technology", parentId: null, children: [] }, +]; + +function dataResponse(data: unknown) { + return Promise.resolve( + new Response(JSON.stringify({ data }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }), + ); +} + +/** Mirrors the endpoint: counts unless the caller opts out. */ +function mockApiFetch() { + vi.mocked(apiFetch).mockImplementation((url: string | URL | Request, init?: RequestInit) => { + const urlString = typeof url === "string" ? url : url instanceof URL ? url.toString() : url.url; + const { pathname, searchParams } = new URL(urlString, "http://localhost"); + const method = init?.method ?? "GET"; + + if (method === "GET" && pathname === "/_emdash/api/taxonomies") { + return dataResponse({ taxonomies: [categoriesTaxonomy] }); + } + + if (method === "GET" && pathname === "/_emdash/api/taxonomies/categories/terms") { + const withCounts = searchParams.get("includeCounts") !== "false"; + return dataResponse({ + terms: terms.map((term) => (withCounts ? { ...term, count: 5 } : term)), + }); + } + + return dataResponse({}); + }); +} + +function makeWrapper() { + // staleTime mirrors App.tsx: inside that window a mounting consumer is served + // the cached list without refetching. + const queryClient = new QueryClient({ + defaultOptions: { + queries: { retry: false, staleTime: 1000 * 60 }, + mutations: { retry: false }, + }, + }); + return function Wrapper({ children }: { children: React.ReactNode }) { + return ( + + {children} + + ); + }; +} + +/** The editor sidebar renders first; the settings page mounts afterwards, as it + * would when the user navigates to it in the same SPA session. */ +function EditorThenSettings() { + const [settingsOpen, setSettingsOpen] = React.useState(false); + return ( + <> + + + {settingsOpen ? : null} + + ); +} + +describe("taxonomy term cache", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockApiFetch(); + }); + + it("shows real counts in the manager after the editor cached a count-free list", async () => { + const screen = await render(, { wrapper: makeWrapper() }); + + await expect.element(screen.getByText("Technology")).toBeInTheDocument(); + + await screen.getByRole("button", { name: "Open settings" }).click(); + + await expect.element(screen.getByRole("heading", { name: "Categories" })).toBeInTheDocument(); + await expect.element(screen.getByText("5", { exact: true })).toBeInTheDocument(); + }); +}); diff --git a/packages/core/src/api/handlers/taxonomies.ts b/packages/core/src/api/handlers/taxonomies.ts index 7e61a3be15..27852274c3 100644 --- a/packages/core/src/api/handlers/taxonomies.ts +++ b/packages/core/src/api/handlers/taxonomies.ts @@ -50,7 +50,8 @@ export interface TermData { } export interface TermWithCount extends TermData { - count: number; + /** Absent when the caller opted out of counts (`includeCounts: false`). */ + count?: number; children: TermWithCount[]; } @@ -384,7 +385,7 @@ export async function handleTaxonomyDefTranslations( export async function handleTermList( db: Kysely, taxonomyName: string, - options: { locale?: string } = {}, + options: { locale?: string; includeCounts?: boolean } = {}, ): Promise> { try { // Definitions are per-locale but terms aren't bound to the def's locale — @@ -401,11 +402,10 @@ export async function handleTermList( // taxonomy's declared collections — one query for the whole list. // content_taxonomies.taxonomy_id stores the translation_group, so we // look up by group and map back to each term's id. - const countsByGroup = await fetchVisibleTermCounts( - db, - taxonomyName, - defCollections(lookup.def), - ); + const includeCounts = options.includeCounts ?? true; + const countsByGroup = includeCounts + ? await fetchVisibleTermCounts(db, taxonomyName, defCollections(lookup.def)) + : undefined; const termData: TermWithCount[] = terms.map((term) => ({ id: term.id, @@ -415,7 +415,7 @@ export async function handleTermList( parentId: term.parentId, description: typeof term.data?.description === "string" ? term.data.description : undefined, children: [], - count: countsByGroup.get(term.translationGroup ?? term.id) ?? 0, + ...(countsByGroup && { count: countsByGroup.get(term.translationGroup ?? term.id) ?? 0 }), locale: term.locale, translationGroup: term.translationGroup, })); diff --git a/packages/core/src/api/openapi/document.ts b/packages/core/src/api/openapi/document.ts index 98ca1ce2f5..ddf7e90696 100644 --- a/packages/core/src/api/openapi/document.ts +++ b/packages/core/src/api/openapi/document.ts @@ -114,6 +114,7 @@ import { createTermBody, taxonomyListResponseSchema, termGetResponseSchema, + termListQuery, termListResponseSchema, termResponseSchema, updateTermBody, @@ -1327,6 +1328,7 @@ const taxonomyPaths = { tags: ["Taxonomies"], requestParams: { path: z.object({ name: z.string().meta({ description: "Taxonomy name" }) }), + query: termListQuery, }, responses: { "200": { diff --git a/packages/core/src/api/schemas/taxonomies.ts b/packages/core/src/api/schemas/taxonomies.ts index da76974cfa..83cf3244da 100644 --- a/packages/core/src/api/schemas/taxonomies.ts +++ b/packages/core/src/api/schemas/taxonomies.ts @@ -55,6 +55,21 @@ export const updateTermBody = z }) .meta({ id: "UpdateTermBody" }); +export const termListQuery = z + .object({ + locale: localeCode.optional(), + includeCounts: z + .enum(["true", "false"]) + .transform((v) => v === "true") + .optional() + .default(true) + .meta({ + description: + "Include each term's visible-usage count. Pass false to skip the aggregate; `count` is then absent from every term.", + }), + }) + .meta({ id: "TermListQuery" }); + // --------------------------------------------------------------------------- // Taxonomies: Response schemas // --------------------------------------------------------------------------- @@ -125,7 +140,7 @@ export const termWithCountSchema: z.ZodType = z label: z.string(), parentId: z.string().nullable(), description: z.string().optional(), - count: z.number().int(), + count: z.number().int().optional(), children: z.array(z.lazy(() => termWithCountSchema)), locale: z.string(), translationGroup: z.string().nullable(), diff --git a/packages/core/src/astro/routes/api/taxonomies/[name]/terms/index.ts b/packages/core/src/astro/routes/api/taxonomies/[name]/terms/index.ts index 26f3940a68..2ba88e2e75 100644 --- a/packages/core/src/astro/routes/api/taxonomies/[name]/terms/index.ts +++ b/packages/core/src/astro/routes/api/taxonomies/[name]/terms/index.ts @@ -11,7 +11,7 @@ import { requirePerm } from "#api/authorize.js"; import { apiError, handleError, requireDb, unwrapResult } from "#api/error.js"; import { handleTermCreate, handleTermList } from "#api/handlers/taxonomies.js"; import { isParseError, parseBody, parseQuery } from "#api/parse.js"; -import { createTermBody, localeFilterQuery } from "#api/schemas.js"; +import { createTermBody, termListQuery } from "#api/schemas.js"; export const prerender = false; @@ -29,11 +29,14 @@ export const GET: APIRoute = async ({ params, request, locals }) => { const denied = requirePerm(user, "taxonomies:read"); if (denied) return denied; - const query = parseQuery(new URL(request.url), localeFilterQuery); + const query = parseQuery(new URL(request.url), termListQuery); if (isParseError(query)) return query; try { - const result = await handleTermList(emdash.db, name, { locale: query.locale }); + const result = await handleTermList(emdash.db, name, { + locale: query.locale, + includeCounts: query.includeCounts, + }); return unwrapResult(result); } catch (error) { return handleError(error, "Failed to list terms", "TERM_LIST_ERROR"); diff --git a/packages/core/tests/unit/taxonomies/term-list-counts.test.ts b/packages/core/tests/unit/taxonomies/term-list-counts.test.ts new file mode 100644 index 0000000000..b24e4c00a5 --- /dev/null +++ b/packages/core/tests/unit/taxonomies/term-list-counts.test.ts @@ -0,0 +1,134 @@ +/** + * The admin terms-list endpoint aggregates visible counts only when asked. + * + * The content editor's taxonomy sidebar lists terms to pick from and never + * renders a count, but it shares an endpoint with the Taxonomies settings page, + * which does. Every editor open therefore paid one `content_taxonomies × ec_*` + * aggregate per applicable taxonomy. Assertions are on the SQL actually + * executed — the response shape alone can't tell whether the work was done. + */ + +import { Role, type RoleLevel } from "@emdash-cms/auth"; +import type { APIContext } from "astro"; +import Database from "better-sqlite3"; +import { Kysely, SqliteDialect } from "kysely"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { handleTermList } from "../../../src/api/handlers/taxonomies.js"; +import { GET as getTerms } from "../../../src/astro/routes/api/taxonomies/[name]/terms/index.js"; +import { runMigrations } from "../../../src/database/migrations/runner.js"; +import { ContentRepository } from "../../../src/database/repositories/content.js"; +import { TaxonomyRepository } from "../../../src/database/repositories/taxonomy.js"; +import type { Database as DatabaseSchema } from "../../../src/database/types.js"; +import { SchemaRegistry } from "../../../src/schema/registry.js"; + +/** SQL of every query executed against the test database. */ +let queries: string[] = []; + +/** `per_collection` is the visible-count aggregate's subquery alias. */ +function countAggregateQueries(): string[] { + return queries.filter((q) => q.includes("per_collection")); +} + +const adminUser = { + id: "u-admin", + email: "a@example.com", + name: "Admin", + role: Role.ADMIN as RoleLevel, +}; + +function buildGetContext(db: Kysely, name: string, search = ""): APIContext { + const url = new URL(`http://localhost/_emdash/api/taxonomies/${name}/terms${search}`); + return { + params: { name }, + url, + request: new Request(url, { headers: { "X-EmDash-Request": "1" } }), + locals: { emdash: { db }, user: adminUser }, + // eslint-disable-next-line typescript/no-unsafe-type-assertion -- minimal stub for tests + } as unknown as APIContext; +} + +interface TermsResponse { + data?: { terms?: Array<{ slug: string; count?: number }> }; +} + +describe("term list counts are only aggregated on demand", () => { + let db: Kysely; + + beforeEach(async () => { + queries = []; + db = new Kysely({ + dialect: new SqliteDialect({ database: new Database(":memory:") }), + log(event) { + if (event.level === "query") queries.push(event.query.sql); + }, + }); + await runMigrations(db); + + // Migrations seed the `category` def declaring a `posts` collection; point + // it at the collection this test creates so the aggregate has a real table. + await new SchemaRegistry(db).createCollection({ + slug: "post", + label: "Posts", + labelSingular: "Post", + }); + await db + .updateTable("_emdash_taxonomy_defs") + .set({ collections: JSON.stringify(["post"]) }) + .where("name", "=", "category") + .execute(); + + const taxRepo = new TaxonomyRepository(db); + const contentRepo = new ContentRepository(db); + const term = await taxRepo.create({ name: "category", slug: "tech", label: "Technology" }); + for (const slug of ["published-one", "published-two"]) { + const entry = await contentRepo.create({ type: "post", slug, status: "published", data: {} }); + await taxRepo.attachToEntry("post", entry.id, term.id); + } + }); + + afterEach(async () => { + await db.destroy(); + }); + + it("counts by default, so existing callers are unaffected", async () => { + queries = []; + const result = await handleTermList(db, "category"); + + expect(result.success).toBe(true); + if (!result.success) return; + expect(result.data.terms[0]!.count).toBe(2); + expect(countAggregateQueries()).toHaveLength(1); + }); + + it("omits the aggregate and the count field when the caller opts out", async () => { + queries = []; + const result = await handleTermList(db, "category", { includeCounts: false }); + + expect(result.success).toBe(true); + if (!result.success) return; + expect(result.data.terms[0]!.slug).toBe("tech"); + expect(result.data.terms[0]).not.toHaveProperty("count"); + expect(countAggregateQueries()).toEqual([]); + }); + + it("honours ?includeCounts=false on the route", async () => { + queries = []; + const response = await getTerms(buildGetContext(db, "category", "?includeCounts=false")); + const body = (await response.json()) as TermsResponse; + + expect(response.status).toBe(200); + expect(body.data?.terms?.[0]?.slug).toBe("tech"); + expect(body.data?.terms?.[0]).not.toHaveProperty("count"); + expect(countAggregateQueries()).toEqual([]); + }); + + it("still counts on the route when the param is absent", async () => { + queries = []; + const response = await getTerms(buildGetContext(db, "category")); + const body = (await response.json()) as TermsResponse; + + expect(body.data?.terms?.[0]?.count).toBe(2); + expect(countAggregateQueries()).toHaveLength(1); + }); +});