diff --git a/.changelog/next/changed-issue-4148.md b/.changelog/next/changed-issue-4148.md new file mode 100644 index 0000000000..d49362f890 --- /dev/null +++ b/.changelog/next/changed-issue-4148.md @@ -0,0 +1 @@ +- Creative Commission detail page resolves its render history through a new `GET /creative-director?ids=` batch filter instead of fetching every Creative Director project diff --git a/client/src/pages/CreativeCommissionDetail.jsx b/client/src/pages/CreativeCommissionDetail.jsx index 9fa797a02f..bb19de48e1 100644 --- a/client/src/pages/CreativeCommissionDetail.jsx +++ b/client/src/pages/CreativeCommissionDetail.jsx @@ -27,7 +27,7 @@ import { } from '../components/creative-commission/commissionForm.js'; import { getCommission, updateCommission, deleteCommission, - submitCommissionFeedback, runCommissionNow, listCreativeDirectorProjects, + submitCommissionFeedback, runCommissionNow, getCreativeDirectorProjectsByIds, } from '../services/api'; export default function CreativeCommissionDetail() { @@ -89,10 +89,11 @@ export default function CreativeCommissionDetail() { } }, [commission]); - // The set of CD projects referenced by this commission's runs. Fetch the full - // project list once (the list route returns non-slim payloads, so previews - // compute with no per-card fetch) and index the referenced ones. Re-runs when - // the projectId set changes (e.g. a Run Now appends a new render). + // The set of CD projects referenced by this commission's runs. Fetch ONLY + // those (#4148) — the batch `?ids=` filter costs one round trip sized to this + // commission's ≤50 persisted runs rather than to the install's total project + // count, and still returns the full non-slim payload previews compute from. + // Re-runs when the projectId set changes (e.g. a Run Now appends a render). const projectIdsKey = useMemo(() => { const ids = (commission?.runs || []).map((r) => r.projectId).filter(Boolean); return [...new Set(ids)].sort().join(','); @@ -100,17 +101,15 @@ export default function CreativeCommissionDetail() { useEffect(() => { if (!projectIdsKey) { setProjectsById(new Map()); setProjectsLoading(false); return; } - const wanted = new Set(projectIdsKey.split(',')); let cancelled = false; setProjectsLoading(true); - listCreativeDirectorProjects() + getCreativeDirectorProjectsByIds(projectIdsKey.split(','), { silent: true }) .then((projects) => { if (cancelled) return; - const map = new Map(); - for (const p of Array.isArray(projects) ? projects : []) { - if (wanted.has(p.id)) map.set(p.id, p); - } - setProjectsById(map); + // Index by id, not position: an id that no longer resolves (pruned + // project) is absent from the response, and its card degrades to the + // status-only placeholder. + setProjectsById(new Map((Array.isArray(projects) ? projects : []).map((p) => [p.id, p]))); }) .catch(() => { /* status-only cards degrade gracefully */ }) .finally(() => { if (!cancelled) setProjectsLoading(false); }); diff --git a/client/src/pages/CreativeCommissionDetail.test.jsx b/client/src/pages/CreativeCommissionDetail.test.jsx new file mode 100644 index 0000000000..e119d70b1f --- /dev/null +++ b/client/src/pages/CreativeCommissionDetail.test.jsx @@ -0,0 +1,107 @@ +/** + * Creative Commission detail page — render-history project resolution (#4148). + * + * The page used to pull EVERY Creative Director project just to index the ones + * its runs reference, so its cost scaled with the install's total project count. + * These cases pin the batch-by-id fetch: only the referenced ids go out, the + * whole-list route is never touched, and an id the batch can't resolve still + * degrades to the status-only card. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, waitFor } from '@testing-library/react'; +import { MemoryRouter } from 'react-router'; + +vi.mock('../services/api', async (importOriginal) => ({ + ...(await importOriginal()), + getCommission: vi.fn(), + updateCommission: vi.fn(), + deleteCommission: vi.fn(), + submitCommissionFeedback: vi.fn(), + runCommissionNow: vi.fn(), + getCreativeDirectorProjectsByIds: vi.fn(() => Promise.resolve([])), + listCreativeDirectorProjects: vi.fn(() => Promise.resolve([])), +})); +// The config form loads model catalogs on mount — out of scope here, and it +// would put real requests behind the assertions about which projects load. +vi.mock('../components/creative-commission/CommissionConfigForm.jsx', () => ({ default: () => null })); +vi.mock('../components/ui/Toast', () => ({ + default: { success: vi.fn(), error: vi.fn(), info: vi.fn(), warning: vi.fn() }, +})); +// ProjectPreview reaches into the media/job graph; the assertions here are about +// which projects resolved, so stub it down to an identifiable marker. +vi.mock('../components/creative-director/ProjectPreview.jsx', () => ({ + default: ({ project }) =>
, +})); + +import * as api from '../services/api'; +import CreativeCommissionDetail from './CreativeCommissionDetail'; + +const COMMISSION = { + id: 'cc-1', + name: 'Example commission', + enabled: true, + targetAbility: 'video', + schedule: { kind: 'cron', cron: '0 9 * * *' }, + assignment: {}, + feedback: [], + runs: [ + { id: 'run-1', projectId: 'cd-1', status: 'started', ranAt: '2026-05-01T10:00:00.000Z' }, + { id: 'run-2', projectId: 'cd-2', status: 'started', ranAt: '2026-05-02T10:00:00.000Z' }, + // Same project as run-1 — the batch must de-duplicate it. + { id: 'run-3', projectId: 'cd-1', status: 'started', ranAt: '2026-05-03T10:00:00.000Z' }, + // No render at all — contributes no id. + { id: 'run-4', projectId: null, status: 'skipped', ranAt: '2026-05-04T10:00:00.000Z' }, + ], +}; + +const renderPage = async () => { + render(); + await screen.findByRole('heading', { name: COMMISSION.name }); +}; + +describe('CreativeCommissionDetail render-history project resolution (#4148)', () => { + beforeEach(() => { + vi.clearAllMocks(); + api.getCommission.mockResolvedValue(COMMISSION); + api.getCreativeDirectorProjectsByIds.mockResolvedValue([]); + }); + + it('fetches only the projects its runs reference, never the whole list', async () => { + api.getCreativeDirectorProjectsByIds.mockResolvedValue([ + { id: 'cd-1', name: 'P1' }, { id: 'cd-2', name: 'P2' }, + ]); + await renderPage(); + + await waitFor(() => expect(api.getCreativeDirectorProjectsByIds).toHaveBeenCalled()); + const [ids] = api.getCreativeDirectorProjectsByIds.mock.calls[0]; + expect([...ids].sort()).toEqual(['cd-1', 'cd-2']); + expect(api.listCreativeDirectorProjects).not.toHaveBeenCalled(); + + await waitFor(() => expect(screen.getAllByTestId('preview-cd-1')).toHaveLength(2)); + expect(screen.getAllByTestId('preview-cd-2')).toHaveLength(1); + }); + + it('skips the request entirely when no run references a project', async () => { + api.getCommission.mockResolvedValue({ + ...COMMISSION, + runs: [{ id: 'run-9', projectId: null, status: 'skipped', ranAt: '2026-05-04T10:00:00.000Z' }], + }); + await renderPage(); + + await screen.findByText('no render'); + expect(api.getCreativeDirectorProjectsByIds).not.toHaveBeenCalled(); + expect(api.listCreativeDirectorProjects).not.toHaveBeenCalled(); + }); + + it('degrades a run whose project the batch could not resolve to a status-only card', async () => { + api.getCreativeDirectorProjectsByIds.mockResolvedValue([{ id: 'cd-1', name: 'P1' }]); + await renderPage(); + + await waitFor(() => expect(screen.getAllByTestId('preview-cd-1')).toHaveLength(2)); + // cd-2 was requested but is gone (pruned project) — no preview, and the + // placeholder must read "unavailable" rather than staying on "loading…". + expect(screen.queryByTestId('preview-cd-2')).toBeNull(); + expect(screen.getByText('render unavailable')).toBeTruthy(); + }); +}); diff --git a/client/src/services/README.md b/client/src/services/README.md index be7b9b7c01..dcf89a101f 100644 --- a/client/src/services/README.md +++ b/client/src/services/README.md @@ -29,6 +29,7 @@ toasts on throw). **Custom catch ⇒ `silent: true`** — otherwise toasts fire |---|---| | `api.js` | Barrel — re-exports every `apiX.js`. | | `apiCore.js` | `request()` helper + stable PortOS-app id. Shared error / toast handling. | +| `apiBatch.js` | `fetchByIds(path, ids, options)` — batch-fetch records through a list route's `?ids=a,b,c` filter (dedupe, empty-list short-circuit, `{ items }` envelope unwrap). | | `socket.js` | Singleton Socket.IO client over relative path (Tailscale-friendly). | | `appUrls.js` | Compute candidate launch URLs for an app from page context. | diff --git a/client/src/services/api.js b/client/src/services/api.js index 989be41e98..dafc6d08f6 100644 --- a/client/src/services/api.js +++ b/client/src/services/api.js @@ -70,6 +70,7 @@ export * from './apiRounds.js'; export * from './apiSongbook.js'; export * from './apiPeerSync.js'; export * from './apiSyncReview.js'; +export * from './apiBatch.js'; export * from './apiCreativeDirector.js'; export * from './apiCreativeCommission.js'; export * from './apiGames.js'; diff --git a/client/src/services/apiBatch.js b/client/src/services/apiBatch.js new file mode 100644 index 0000000000..e771415475 --- /dev/null +++ b/client/src/services/apiBatch.js @@ -0,0 +1,39 @@ +import { request } from './apiCore.js'; + +/** + * The id list `fetchByIds` will actually request: trimmed, non-strings and + * blanks dropped, de-duplicated, in first-seen order. Exported so a caller that + * re-orders the response into its request order (apiCatalog) indexes against the + * exact same normalized list rather than its own raw input. + */ +export const normalizeIds = (ids) => [...new Set( + (Array.isArray(ids) ? ids : []) + .map((id) => (typeof id === 'string' ? id.trim() : '')) + .filter(Boolean), +)]; + +/** + * Batch-fetch records by id through a list route's `?ids=a,b,c` filter (#4148) — + * the client half of the server's shared `csvIdsParam` query param. + * + * Normalizes the ids (see above), then unwraps a paginated `{ items }` envelope + * to a plain array. Ids the server omits (missing or soft-deleted) are simply + * absent from the result, so callers index it by `id` rather than assuming + * positional parity with the request. + * + * The trim and the empty-list short-circuit are load-bearing, not + * micro-optimizations: a present-but-blank `?ids=` (which is what a + * whitespace-only id serializes to) reads as ABSENT server-side, so issuing the + * request anyway would return the whole unfiltered list — the exact over-fetch + * these batch helpers exist to remove. + * + * Lives in its own module rather than inside `apiCore.js` so the suites that + * mock `./apiCore.js` wholesale still intercept the `request` this makes. + */ +export async function fetchByIds(path, ids = [], options) { + const list = normalizeIds(ids); + if (list.length === 0) return []; + const params = new URLSearchParams({ ids: list.join(',') }); + const res = await request(`${path}?${params}`, options); + return Array.isArray(res) ? res : (Array.isArray(res?.items) ? res.items : []); +} diff --git a/client/src/services/apiCatalog.js b/client/src/services/apiCatalog.js index 334f7f2999..75fd6c3e8f 100644 --- a/client/src/services/apiCatalog.js +++ b/client/src/services/apiCatalog.js @@ -1,4 +1,5 @@ import { request } from './apiCore.js'; +import { fetchByIds, normalizeIds } from './apiBatch.js'; // Creative Ingredients Catalog API surface. Every helper takes an optional // `options` second arg so callers with their own `.catch` toast can pass @@ -84,17 +85,14 @@ export const listCatalogIngredients = ({ type, tag, q, refKind, refId, unlinked, // Batch fetch ingredients by id (max 50 server-side) — used by the Story // Builder remix handoff to hydrate the catalog ingredients the user selected. -// The `ids` filter rides the normal paged list endpoint, which returns the -// `{ items, nextOffset }` envelope ordered created_at DESC; this unwraps to a -// plain array AND re-orders it to the requested `ids` so chips + seed read in -// the user's selection order (mirroring the server's resolveCatalogIngredients). -// Empty/falsy ids are dropped before the request. +// `fetchByIds` handles the shared `?ids=` mechanics (dedupe, empty-list +// short-circuit, `{ items }` envelope unwrap); the extra step here is +// re-ordering to the requested `ids` — the paged list returns created_at DESC, +// and chips + seed must read in the user's selection order (mirroring the +// server's resolveCatalogIngredients). export const listCatalogIngredientsByIds = async (ids = [], options) => { - const list = (Array.isArray(ids) ? ids : []).filter(Boolean); - const params = new URLSearchParams(); - params.set('ids', list.join(',')); - const res = await request(`/catalog/ingredients?${params}`, options); - const items = Array.isArray(res) ? res : (Array.isArray(res?.items) ? res.items : []); + const list = normalizeIds(ids); + const items = await fetchByIds('/catalog/ingredients', list, options); const byId = new Map(items.map((ing) => [ing.id, ing])); return list.map((id) => byId.get(id)).filter(Boolean); }; diff --git a/client/src/services/apiCreativeDirector.js b/client/src/services/apiCreativeDirector.js index 5e664956d6..00e77d3e3a 100644 --- a/client/src/services/apiCreativeDirector.js +++ b/client/src/services/apiCreativeDirector.js @@ -1,4 +1,5 @@ import { request } from './apiCore.js'; +import { fetchByIds } from './apiBatch.js'; export const listCreativeDirectorProjects = (options = {}) => request('/creative-director', options); // Pass `{ slim: true }` to receive only the fields a polling consumer needs @@ -7,6 +8,13 @@ export const listCreativeDirectorProjects = (options = {}) => request('/creative // for 4s-poll surfaces like the Pipeline EpisodeVideoStage. export const getCreativeDirectorProject = (id, { slim = false } = {}) => request(`/creative-director/${encodeURIComponent(id)}${slim ? '?slim=1' : ''}`); +// Batch fetch projects by id (#4148) — the `ids` filter rides the normal list +// endpoint, so the response is the same full, non-slim project shape previews +// compute from. Used by surfaces that reference a known handful of projects +// (the Creative Commission detail page's render history) instead of pulling +// every project on the install. Server-side cap is 100 ids per batch. +export const getCreativeDirectorProjectsByIds = (ids = [], options = {}) => + fetchByIds('/creative-director', ids, options); export const createCreativeDirectorProject = (data, options = {}) => request('/creative-director', { method: 'POST', body: JSON.stringify(data), diff --git a/client/src/services/apiCreativeDirector.test.js b/client/src/services/apiCreativeDirector.test.js new file mode 100644 index 0000000000..3af8c891bd --- /dev/null +++ b/client/src/services/apiCreativeDirector.test.js @@ -0,0 +1,78 @@ +/** + * Creative Director API wrapper — batch-by-id fetch (#4148). + * + * The empty-list short-circuit is load-bearing, not a micro-optimization: a + * present-but-blank `?ids=` reads as ABSENT server-side, so issuing the request + * anyway would return every project on the install — exactly the over-fetch this + * helper exists to remove. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +vi.mock('./apiCore.js', async (importOriginal) => ({ + ...(await importOriginal()), + request: vi.fn(), +})); + +let request; +let getCreativeDirectorProjectsByIds; + +beforeEach(async () => { + vi.resetModules(); + ({ request } = await import('./apiCore.js')); + ({ getCreativeDirectorProjectsByIds } = await import('./apiCreativeDirector.js')); + request.mockReset(); +}); + +describe('getCreativeDirectorProjectsByIds', () => { + it('sends the ids as a CSV on the list route and returns the bare array', async () => { + request.mockResolvedValue([{ id: 'cd-1' }, { id: 'cd-2' }]); + const out = await getCreativeDirectorProjectsByIds(['cd-1', 'cd-2']); + expect(out.map((p) => p.id)).toEqual(['cd-1', 'cd-2']); + + const [path, options] = request.mock.calls[0]; + expect(path.startsWith('/creative-director?')).toBe(true); + expect(decodeURIComponent(path)).toContain('ids=cd-1,cd-2'); + expect(options).toEqual({}); + }); + + it('de-duplicates and drops falsy ids before building the query', async () => { + request.mockResolvedValue([]); + await getCreativeDirectorProjectsByIds(['cd-1', '', 'cd-1', null, 'cd-2', undefined]); + expect(decodeURIComponent(request.mock.calls[0][0])).toContain('ids=cd-1,cd-2'); + }); + + it('short-circuits an empty/all-falsy id list without issuing a request', async () => { + expect(await getCreativeDirectorProjectsByIds([])).toEqual([]); + expect(await getCreativeDirectorProjectsByIds([null, '', undefined])).toEqual([]); + expect(await getCreativeDirectorProjectsByIds()).toEqual([]); + expect(await getCreativeDirectorProjectsByIds('not-an-array')).toEqual([]); + expect(request).not.toHaveBeenCalled(); + }); + + // A whitespace-only id survives a bare `filter(Boolean)` and serializes to + // `?ids=%20`, which the server trims back to ABSENT — i.e. it would return + // every project on the install, the exact over-fetch this helper removes. + it('trims ids, and treats an all-whitespace list as empty (no request)', async () => { + expect(await getCreativeDirectorProjectsByIds([' ', '\t', 42])).toEqual([]); + expect(request).not.toHaveBeenCalled(); + + request.mockResolvedValue([]); + await getCreativeDirectorProjectsByIds([' cd-1 ', ' ', 'cd-2']); + expect(decodeURIComponent(request.mock.calls[0][0])).toContain('ids=cd-1,cd-2'); + }); + + it('unwraps a paginated { items } envelope and tolerates a junk response', async () => { + request.mockResolvedValue({ items: [{ id: 'cd-1' }], total: 1 }); + expect(await getCreativeDirectorProjectsByIds(['cd-1'])).toEqual([{ id: 'cd-1' }]); + + request.mockResolvedValue(null); + expect(await getCreativeDirectorProjectsByIds(['cd-1'])).toEqual([]); + }); + + it('forwards request options (e.g. { silent: true }) through to request()', async () => { + request.mockResolvedValue([]); + await getCreativeDirectorProjectsByIds(['cd-1'], { silent: true }); + expect(request.mock.calls[0][1]).toEqual({ silent: true }); + }); +}); diff --git a/server/lib/README.md b/server/lib/README.md index 46465093f5..5b451bfb3d 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -24,7 +24,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | Module | Purpose | |---|---| | `validation.js` | Catch-all Zod schemas (app/process/provider, social accounts, GitHub, backup/sharing, document/legacy-export) + the `validateRequest` middleware + shared helpers (`optionalBooleanMap`, `isSafeRecordId`, `parsePagination`). Re-exports the per-domain validation files below so existing deep imports keep working. | -| `sharedSchemas.js` | Cross-domain Zod fragments that `validation.js` and the per-domain `*Validation.js` files both need, kept in a leaf module so the domain files never import back through `validation.js` (its hoisted `export * from` lines would TDZ). `grokVideoDurationSchema` (grok clip-length union), `cloudModelIdString(message)` (cloud-CLI model-id charset/bounds), `recordRenderPinFields` (the `imageMode`/`imageModelId` per-record render pin pair), and `isSafeSubdirFilter(v)` (relative path with no wildcard, `..` segment, or leading `/`). | +| `sharedSchemas.js` | Cross-domain Zod fragments that `validation.js` and the per-domain `*Validation.js` files both need, kept in a leaf module so the domain files never import back through `validation.js` (its hoisted `export * from` lines would TDZ). `grokVideoDurationSchema` (grok clip-length union), `cloudModelIdString(message)` (cloud-CLI model-id charset/bounds), `recordRenderPinFields` (the `imageMode`/`imageModelId` per-record render pin pair), and `isSafeSubdirFilter(v)` (relative path with no wildcard, `..` segment, or leading `/`), and `csvIdsParam({ max, maxIdLength, truncate })` (the `?ids=a,b,c` batch-by-id query param: trims, drops empties, reads all-blank as absent, and either 400s or silently slices an over-cap batch). | | `agentOutputMarkers.js` | Status lines PortOS itself appends to an agent's output buffer: `SENTINEL_COMPLETION_MARKER` (the line `ingestDoneSentinel` writes just before the agent's `.agent-done` summary) plus `isAgentLifecycleLine(line)`/`stripLifecycleLines(lines)`, which match PortOS's telemetry on its actual message shapes rather than on a leading emoji (an agent's own summary may well start a line with `✅`). Pure — shared by the emitter (`services/agentTuiSpawning.js`) and by readers that must show only the agent's own words (notably the generated PR description). | | `agentSentinel.js` | The `.agent-done` completion sentinel: `DONE_SENTINEL_NAME`, `doneSentinelName(agentId)` → the per-instance filename `.agent-done-` (worktree-less agents share one workspace, so a shared name lets concurrent runs clobber — and finalize on — each other's signal), `doneSentinelPath(workspacePath, agentId)` → the single path every producer and consumer resolves, + pure `parseSentinelPayload(contents)` → `{ summary, payload }`. Back-compat — a plain-markdown sentinel yields `payload: null`; a JSON object yields its structured `payload` for a programmatic-I/O task type's `processTaskOutput` hook. `salvageSentinelPayload(contents)` (async) is the lenient second tier — runs `jsonExtract` over a fenced/prose-trailed/control-char-corrupted envelope so a less-capable model's near-valid output still surfaces its `payload` instead of being dropped. `extractSentinelPayloadFromTranscript(transcript, isPayload)` (async) is the third tier for programmatic-I/O types ONLY — scans the ANSI-stripped PTY transcript, newest balanced JSON block first, for a payload the model PRINTED instead of writing, and adopts it only if the owning hook's shape predicate accepts it (#3640). | | `agentValidation.js` | Social-bot agent schemas (personality, Moltbook/Moltworld accounts, automation schedules, agent tools + Moltworld payloads) and CoS Feature Agent definitions. | diff --git a/server/lib/catalogValidation.js b/server/lib/catalogValidation.js index 0a59c33da0..453a818536 100644 --- a/server/lib/catalogValidation.js +++ b/server/lib/catalogValidation.js @@ -19,6 +19,7 @@ import { USER_TYPE_FIELD_KINDS, isActiveType, } from './catalogTypes.js'; +import { csvIdsParam } from './sharedSchemas.js'; // Derived from the shared type registry (`catalogTypes.js`) — adding a SYSTEM // type there flows through to consumers automatically. Kept as a frozen @@ -298,16 +299,13 @@ export const catalogRevisionRestoreSchema = z.object({ }).strict(); export const catalogIngredientQuerySchema = z.object({ - // Batch-fetch by id. The wire form is a CSV string (`?ids=a,b,c`); we - // preprocess it into a trimmed, empties-dropped, ≤50 string[]. An empty/ - // all-blank CSV collapses to `undefined` so the field reads cleanly as - // absent (the route/service then falls through to the type/tag/q filters). - // When present, `ids` takes precedence over type/tag/q in `listIngredients`. - ids: z.preprocess((v) => { - if (typeof v !== 'string') return v; - const parts = v.split(',').map((s) => s.trim()).filter(Boolean).slice(0, 50); - return parts.length ? parts : undefined; - }, z.array(z.string().trim().min(1).max(64)).max(50).optional()), + // Batch-fetch by id — the shared CSV param (see sharedSchemas.csvIdsParam), + // trimmed and empties-dropped into a ≤50 string[]. An empty/all-blank CSV + // reads as absent (the route/service then falls through to the type/tag/q + // filters); when present, `ids` takes precedence over type/tag/q in + // `listIngredients`. `truncate` keeps this route's long-standing "silently + // slice at 50" contract rather than 400ing an over-cap batch. + ids: csvIdsParam({ max: 50, truncate: true }), type: ingredientTypeGate.optional(), tag: tag.optional(), q: z.string().trim().max(500).optional(), diff --git a/server/lib/creativeDirectorValidation.js b/server/lib/creativeDirectorValidation.js index 8d6884024e..b8492581a1 100644 --- a/server/lib/creativeDirectorValidation.js +++ b/server/lib/creativeDirectorValidation.js @@ -3,6 +3,7 @@ import { ASPECT_RATIOS, QUALITIES, PROJECT_STATUSES, SCENE_STATUSES, PLAN_STEP_S import { ARC_SHAPE_IDS, ARC_ROLES } from './storyArc.js'; import { BIBLE_LIMITS } from './storyBible.js'; import { emptyToUndefined } from './zodCompat.js'; +import { csvIdsParam } from './sharedSchemas.js'; // ============================================================================= // CREATIVE DIRECTOR + CREATE-SUITE IMPORTER SCHEMAS @@ -213,6 +214,26 @@ export const creativeDirectorProjectUpdateSchema = z.object({ modelOverrides: creativeDirectorModelOverridesSchema.optional(), }).strict(); +// Max ids one `GET /creative-director?ids=` batch may resolve (#4148). Sized at +// 2x the Creative Commission run cap (MAX_PERSISTED_RUNS = 50) — the batch's +// heaviest caller resolves one project per persisted run — so the real consumer +// never has to chunk while the request still stays bounded. +export const CREATIVE_DIRECTOR_IDS_BATCH_MAX = 100; + +// Query params for `GET /creative-director` (#4148). `ids` is the shared +// batch-by-id filter (see csvIdsParam) — an over-cap batch 400s rather than +// truncating, so the client can't mistake a sliced result for missing projects. +// An empty/all-blank value reads as absent and the route falls through to the +// full list. Unknown keys (limit/offset) strip out of the parsed result — the +// route reads those straight off `req.query` for paginateArray. +export const creativeDirectorProjectQuerySchema = z.object({ + // 120 matches the per-record peer-sync `recordId` bound (validation.js), not + // the helper's 64-char default: a locally-minted id is `cd-` (39), but a + // project that arrived from a peer may carry anything that contract accepts, + // and such a project must still be resolvable through this batch. + ids: csvIdsParam({ max: CREATIVE_DIRECTOR_IDS_BATCH_MAX, maxIdLength: 120 }), +}); + // One scene in the treatment, written by the agent on the treatment task. export const creativeDirectorSceneSchema = z.object({ sceneId: z.string().min(1).max(64), diff --git a/server/lib/sharedSchemas.js b/server/lib/sharedSchemas.js index 08b9e1eb94..ad5bef6df5 100644 --- a/server/lib/sharedSchemas.js +++ b/server/lib/sharedSchemas.js @@ -56,6 +56,30 @@ export const recordRenderPinFields = { ), }; +// Batch-by-id query param — `?ids=a,b,c` on a list route (#4148). Both wire +// forms normalize identically: a CSV string, and the repeated `?ids=a&ids=b` +// form Express hands over as an array (whose members may themselves be CSV). +// Each entry is trimmed and blanks are dropped, so one shape can't slip past the +// blank-removal or the cap the other enforces. An empty/all-blank value +// collapses to `undefined` so the field reads cleanly as absent +// (sentinel-not-empty) and the route falls through to its normal filters. +// +// `truncate` picks what an over-cap batch does. `false` (the default) rejects it +// with a 400 — the caller learns its request was too big instead of receiving a +// partial result it can't distinguish from "those ids don't exist". `true` +// silently slices at `max`, which the catalog's ingredient list has done since +// it shipped; kept as an option so this shared helper doesn't change that +// route's established contract. +export const csvIdsParam = ({ max, maxIdLength = 64, truncate = false } = {}) => z.preprocess((v) => { + if (typeof v !== 'string' && !Array.isArray(v)) return v; + const parts = (Array.isArray(v) ? v : [v]) + .flatMap((entry) => (typeof entry === 'string' ? entry.split(',') : [entry])) + .map((s) => (typeof s === 'string' ? s.trim() : s)) + .filter((s) => s !== ''); + const bounded = truncate ? parts.slice(0, max) : parts; + return bounded.length ? bounded : undefined; +}, z.array(z.string().trim().min(1).max(maxIdLength)).max(max).optional()); + // subdirFilter is interpolated into an rsync `--include=${subdirFilter}/***` // arg (rsync runs shell:false, so this is not shell injection — but `*` would // expand to `--include=*/***` and defeat the filter chain, and `../foo` would diff --git a/server/routes/creativeDirector.js b/server/routes/creativeDirector.js index 9b3489b298..d8830bfc23 100644 --- a/server/routes/creativeDirector.js +++ b/server/routes/creativeDirector.js @@ -20,11 +20,13 @@ import { creativeDirectorSceneUpdateSchema, creativeDirectorAutoCastSuggestSchema, creativeDirectorAutoCastApplySchema, + creativeDirectorProjectQuerySchema, isPaginationRequested, paginateArray, } from '../lib/validation.js'; import { listProjects, + getProjectsByIds, getProject, createProject, updateProject, @@ -45,8 +47,16 @@ const router = Router(); // Backward-compatible by default: returns the full projects array. When a client // passes `limit`/`offset`, the response becomes the bounded // `{ items, total, limit, offset }` envelope every paginated PortOS list shares. +// +// `?ids=a,b,c` (#4148) narrows to a known id set in one round trip — a caller +// that needs a handful of projects (the Creative Commission detail page +// resolving its runs' renders) no longer pays for every project on the install. +// Ids that don't resolve are simply absent, so the response is never padded with +// placeholders the client would have to filter. Mirrors the catalog's +// `GET /catalog/ingredients?ids=` batch filter. router.get('/', asyncHandler(async (req, res) => { - const projects = await listProjects(); + const { ids } = validateRequest(creativeDirectorProjectQuerySchema, req.query); + const projects = ids ? await getProjectsByIds(ids) : await listProjects(); if (!isPaginationRequested(req.query)) { return res.json(projects); } diff --git a/server/routes/creativeDirector.test.js b/server/routes/creativeDirector.test.js index 863c7435e0..d5243830ce 100644 --- a/server/routes/creativeDirector.test.js +++ b/server/routes/creativeDirector.test.js @@ -4,6 +4,7 @@ import { request } from '../lib/testHelper.js'; vi.mock('../services/creativeDirector/local.js', () => ({ listProjects: vi.fn(async () => [{ id: 'cd-1', name: 'A' }]), + getProjectsByIds: vi.fn(async () => []), getProject: vi.fn(), createProject: vi.fn(), updateProject: vi.fn(async (id, patch) => ({ id, ...patch })), @@ -58,6 +59,7 @@ import * as autoCast from '../services/creativeDirector/autoCast.js'; import * as hook from '../services/creativeDirector/completionHook.js'; import * as firstPass from '../services/creativeDirector/firstPassGen.js'; import * as firstPassMusicBed from '../services/creativeDirector/firstPassMusicGen.js'; +import { CREATIVE_DIRECTOR_IDS_BATCH_MAX } from '../lib/creativeDirectorValidation.js'; import creativeDirectorRoutes from './creativeDirector.js'; describe('creativeDirector routes', () => { @@ -89,6 +91,58 @@ describe('creativeDirector routes', () => { expect(r.body.limit).toBe(2); expect(r.body.offset).toBe(1); }); + + // #4148 — batch-by-id so a caller referencing a handful of projects doesn't + // pay for every project on the install. + it('with ?ids= resolves only the named projects and never lists them all', async () => { + cdService.getProjectsByIds.mockResolvedValueOnce([{ id: 'cd-2', name: 'B' }, { id: 'cd-9', name: 'I' }]); + const r = await request(app).get('/api/creative-director?ids=cd-2,cd-9'); + expect(r.status).toBe(200); + expect(r.body).toEqual([{ id: 'cd-2', name: 'B' }, { id: 'cd-9', name: 'I' }]); + expect(cdService.getProjectsByIds).toHaveBeenCalledWith(['cd-2', 'cd-9']); + expect(cdService.listProjects).not.toHaveBeenCalled(); + }); + + it('trims and drops blank ids, and falls back to the full list when all are blank', async () => { + cdService.getProjectsByIds.mockResolvedValueOnce([{ id: 'cd-2', name: 'B' }]); + await request(app).get('/api/creative-director?ids=%20cd-2%20,,'); + expect(cdService.getProjectsByIds).toHaveBeenCalledWith(['cd-2']); + + const r = await request(app).get('/api/creative-director?ids=%20,,'); + expect(r.status).toBe(200); + expect(r.body).toEqual([{ id: 'cd-1', name: 'A' }]); + expect(cdService.listProjects).toHaveBeenCalled(); + }); + + // Express hands `?ids=a&ids=b` over as an ARRAY — it must normalize through + // the same trim / blank-drop / cap path as the CSV form. + it('normalizes the repeated ?ids= array form identically to the CSV form', async () => { + cdService.getProjectsByIds.mockResolvedValueOnce([{ id: 'cd-2', name: 'B' }]); + await request(app).get('/api/creative-director?ids=%20cd-2%20&ids=&ids=cd-9'); + expect(cdService.getProjectsByIds).toHaveBeenCalledWith(['cd-2', 'cd-9']); + + const many = Array.from({ length: CREATIVE_DIRECTOR_IDS_BATCH_MAX + 1 }, (_, i) => `ids=cd-${i}`).join('&'); + const over = await request(app).get(`/api/creative-director?${many}`); + expect(over.status).toBe(400); + }); + + // A project that arrived from a peer may carry any id the per-record sync + // contract accepts (recordId, max 120) — the batch must still resolve it. + it('accepts a peer-length (120-char) project id', async () => { + const longId = `cd-${'a'.repeat(117)}`; + cdService.getProjectsByIds.mockResolvedValueOnce([{ id: longId }]); + const r = await request(app).get(`/api/creative-director?ids=${longId}`); + expect(r.status).toBe(200); + expect(cdService.getProjectsByIds).toHaveBeenCalledWith([longId]); + }); + + it('rejects an over-cap ids batch instead of silently truncating it', async () => { + const ids = Array.from({ length: CREATIVE_DIRECTOR_IDS_BATCH_MAX + 1 }, (_, i) => `cd-${i}`).join(','); + const r = await request(app).get(`/api/creative-director?ids=${ids}`); + expect(r.status).toBe(400); + expect(cdService.getProjectsByIds).not.toHaveBeenCalled(); + expect(cdService.listProjects).not.toHaveBeenCalled(); + }); }); describe('GET /:id', () => { diff --git a/server/services/creativeDirector/local.js b/server/services/creativeDirector/local.js index 7906f00803..dca7042799 100644 --- a/server/services/creativeDirector/local.js +++ b/server/services/creativeDirector/local.js @@ -75,6 +75,11 @@ export async function getProject(id, options = {}) { return (await selectBackend()).getProject(id, options); } +/** Batch fetch by id (#4148) — see the backends for the per-store implementation. */ +export async function getProjectsByIds(ids, options = {}) { + return (await selectBackend()).getProjectsByIds(ids, options); +} + /** Live project ids (or all when includeDeleted) — used by tombstone GC sweeps. */ export async function listProjectIds(options = {}) { return (await selectBackend()).listProjectIds(options); diff --git a/server/services/creativeDirector/local.test.js b/server/services/creativeDirector/local.test.js index de6706e6ae..4f124aae1d 100644 --- a/server/services/creativeDirector/local.test.js +++ b/server/services/creativeDirector/local.test.js @@ -32,7 +32,7 @@ vi.mock('./firstPassGen.js', () => ({ enqueueFirstPassSceneFrames: vi.fn(async () => ({ mode: 'local', enqueued: [], skipped: [] })), })); -const { setTreatment, recordRun, updateRun, trimRuns } = await import('./local.js'); +const { setTreatment, recordRun, updateRun, trimRuns, getProjectsByIds } = await import('./local.js'); import * as firstPassGen from './firstPassGen.js'; const VALID_TREATMENT = { @@ -226,3 +226,35 @@ describe('runs[] cap enforced at saveAll chokepoint', () => { expect(patched.status).toBe('completed'); }); }); + +// #4148 — batch-by-id read (file backend, through the dispatcher). The Creative +// Commission detail page resolves only the projects its runs reference instead +// of listing every project on the install. +describe('getProjectsByIds (#4148)', () => { + const STORED = [ + { id: 'cd-1', name: 'One' }, + { id: 'cd-2', name: 'Two' }, + { id: 'cd-3', name: 'Three', deleted: true }, + ]; + + it('returns only the requested live projects, ignoring unknown ids', async () => { + mockReadJSONFile.mockResolvedValue(STORED); + const found = await getProjectsByIds(['cd-2', 'cd-nope', 'cd-1']); + expect(found.map((p) => p.id)).toEqual(['cd-1', 'cd-2']); + }); + + it('omits tombstoned projects unless includeDeleted is set', async () => { + mockReadJSONFile.mockResolvedValue(STORED); + expect(await getProjectsByIds(['cd-3'])).toEqual([]); + const withDeleted = await getProjectsByIds(['cd-3'], { includeDeleted: true }); + expect(withDeleted.map((p) => p.id)).toEqual(['cd-3']); + }); + + it('short-circuits an empty/blank id list without reading the store', async () => { + mockReadJSONFile.mockResolvedValue(STORED); + expect(await getProjectsByIds([])).toEqual([]); + expect(await getProjectsByIds([null, undefined, ''])).toEqual([]); + expect(await getProjectsByIds()).toEqual([]); + expect(mockReadJSONFile).not.toHaveBeenCalled(); + }); +}); diff --git a/server/services/creativeDirector/projectsDB.js b/server/services/creativeDirector/projectsDB.js index 21ce611d25..35f0fff4dc 100644 --- a/server/services/creativeDirector/projectsDB.js +++ b/server/services/creativeDirector/projectsDB.js @@ -113,6 +113,21 @@ export async function getProject(id, { includeDeleted = false } = {}) { return includeDeleted || !project.deleted ? project : null; } +/** + * Batch fetch by id (#4148) — one round trip for a known id set, so a caller + * that only needs a handful of projects (the Creative Commission detail page + * resolving its runs' renders) doesn't pull the whole table. Unknown ids are + * simply absent from the result; order matches listProjects (created_at ASC). + */ +export async function getProjectsByIds(ids, { includeDeleted = false } = {}) { + const wanted = [...new Set((ids || []).filter(Boolean))]; + if (wanted.length === 0) return []; + const result = includeDeleted + ? await query(`SELECT id, data FROM creative_director_projects WHERE id = ANY($1) ORDER BY created_at ASC`, [wanted]) + : await query(`SELECT id, data FROM creative_director_projects WHERE id = ANY($1) AND deleted = FALSE ORDER BY created_at ASC`, [wanted]); + return result.rows.map(rowToProject); +} + /** Live project ids (or all when includeDeleted) — used by tombstone GC sweeps. */ export async function listProjectIds({ includeDeleted = false } = {}) { const result = includeDeleted diff --git a/server/services/creativeDirector/projectsDB.test.js b/server/services/creativeDirector/projectsDB.test.js index 19432469df..4350fe37ac 100644 --- a/server/services/creativeDirector/projectsDB.test.js +++ b/server/services/creativeDirector/projectsDB.test.js @@ -98,6 +98,24 @@ describe.skipIf(!dbReady)('projectsDB round-trip', () => { expect(all.find((p) => p.name === 'Legacy row')?.id).toBe(id); }); + // #4148 — the batch-by-id read the Creative Commission detail page uses so it + // doesn't list every project on the install. + it('batch-fetches by id, skipping unknown ids and tombstoned projects', async () => { + const a = await db.createProject(CREATE_INPUT); + const b = await db.createProject(CREATE_INPUT); + const gone = await db.createProject(CREATE_INPUT); + created.push(a.id, b.id, gone.id); + await db.deleteProject(gone.id); + + const batch = await db.getProjectsByIds([a.id, 'cd-does-not-exist', gone.id, b.id]); + expect(batch.map((p) => p.id).sort()).toEqual([a.id, b.id].sort()); + + expect(await db.getProjectsByIds([])).toEqual([]); + // includeDeleted opts the tombstone back in (the sync/GC read shape). + const withDeleted = await db.getProjectsByIds([gone.id], { includeDeleted: true }); + expect(withDeleted.map((p) => p.id)).toEqual([gone.id]); + }); + it('applies a treatment and patches a scene', async () => { const p = await db.createProject(CREATE_INPUT); created.push(p.id); diff --git a/server/services/creativeDirector/projectsFile.js b/server/services/creativeDirector/projectsFile.js index a5f22498cc..9813db7942 100644 --- a/server/services/creativeDirector/projectsFile.js +++ b/server/services/creativeDirector/projectsFile.js @@ -63,6 +63,18 @@ export async function getProject(id, { includeDeleted = false } = {}) { return includeDeleted || !found.deleted ? found : null; } +/** + * Batch fetch by id (#4148) — mirrors the PG backend's single-round-trip lookup + * so a caller with a known id set doesn't have to list every project. Unknown + * ids are simply absent from the result; order follows the stored append order. + */ +export async function getProjectsByIds(ids, { includeDeleted = false } = {}) { + const wanted = new Set((ids || []).filter(Boolean)); + if (wanted.size === 0) return []; + const all = await loadAll(); + return all.filter((p) => wanted.has(p.id) && (includeDeleted || !p.deleted)); +} + /** Live project ids (or all when includeDeleted) — used by tombstone GC sweeps. */ export async function listProjectIds({ includeDeleted = false } = {}) { const all = await loadAll();