Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .changelog/next/changed-issue-4148.md
Original file line number Diff line number Diff line change
@@ -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
23 changes: 11 additions & 12 deletions client/src/pages/CreativeCommissionDetail.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -89,28 +89,27 @@ 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(',');
}, [commission]);

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); });
Expand Down
107 changes: 107 additions & 0 deletions client/src/pages/CreativeCommissionDetail.test.jsx
Original file line number Diff line number Diff line change
@@ -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 }) => <div data-testid={`preview-${project.id}`} />,
}));

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(<MemoryRouter><CreativeCommissionDetail /></MemoryRouter>);
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();
});
});
1 change: 1 addition & 0 deletions client/src/services/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. |

Expand Down
1 change: 1 addition & 0 deletions client/src/services/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
39 changes: 39 additions & 0 deletions client/src/services/apiBatch.js
Original file line number Diff line number Diff line change
@@ -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 : []);
}
18 changes: 8 additions & 10 deletions client/src/services/apiCatalog.js
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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);
};
Expand Down
8 changes: 8 additions & 0 deletions client/src/services/apiCreativeDirector.js
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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),
Expand Down
78 changes: 78 additions & 0 deletions client/src/services/apiCreativeDirector.test.js
Original file line number Diff line number Diff line change
@@ -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 });
});
});
Loading