Skip to content

perf: resolve a commission's render projects with a by-id batch (#4148) - #4254

Merged
atomantic merged 4 commits into
mainfrom
claim/issue-4148
Aug 15, 2026
Merged

perf: resolve a commission's render projects with a by-id batch (#4148)#4254
atomantic merged 4 commits into
mainfrom
claim/issue-4148

Conversation

@atomantic

Copy link
Copy Markdown
Owner

Summary

client/src/pages/CreativeCommissionDetail.jsx fetched every Creative Director project (non-slim) on load just to index the ones its runs[].projectId referenced — cost scaled with the install's total project count rather than with the commission.

This adds a by-id batch resolver and points the page at it:

  • GET /creative-director?ids=a,b,c — a batch filter on the existing list route, mirroring the catalog's GET /catalog/ingredients?ids= precedent (no new path, so nothing can shadow /:id). Validated by a new creativeDirectorProjectQuerySchema in server/lib/creativeDirectorValidation.js.
  • getProjectsByIds(ids, { includeDeleted }) on both project backends (projectsDB.js uses WHERE id = ANY($1); projectsFile.js filters the loaded array) plus the local.js dispatcher — one round trip, tombstones excluded by default, unknown ids simply absent.
  • getCreativeDirectorProjectsByIds() in client/src/services/apiCreativeDirector.js — de-dupes/drops blank ids and short-circuits an empty list with no request.
  • The page now indexes the response by id (positional parity was never safe: a pruned project is absent), and passes { silent: true } since it owns a .catch that degrades to status-only cards (client CLAUDE.md: custom catch ⇒ silent).

Decisions made while implementing

  • Batch cap = 100 ids (CREATIVE_DIRECTOR_IDS_BATCH_MAX), 2x the Creative Commission MAX_PERSISTED_RUNS = 50, so the real consumer never has to chunk.
  • Over-cap requests 400 rather than silently truncating. The catalog schema slices at its cap; that would hand back a partial result a caller can't distinguish from "those ids don't exist", so this one rejects instead.

Test plan

  • cd server && NODE_ENV=test npm test — 1389 files / 29065 tests pass. The only 4 failures (routes/health.test.js, services/updateExecutor.test.js) reproduce identically on a clean origin/main checkout and are environmental (an ambient PORTOS_FORCE_CLEAN_WORKSPACES env var + gh/forge reachability), not from this change.
  • cd client && NODE_ENV=test npm test — 644 files / 7842 tests pass.
  • New coverage:
    • server/routes/creativeDirector.test.js?ids= dispatches to getProjectsByIds and never calls listProjects; blank/whitespace ids trim and an all-blank CSV falls back to the full list; an over-cap batch 400s without touching either service.
    • server/services/creativeDirector/local.test.js — file-backend batch: only requested live projects, tombstones excluded unless includeDeleted, empty/blank list short-circuits without reading the store.
    • server/services/creativeDirector/projectsDB.test.js — PG round-trip batch case (skips without a test DB; runs under npm run test:db).
    • client/src/pages/CreativeCommissionDetail.test.jsx (new) — only the referenced ids go out (de-duplicated), the whole-list route is never called, no request at all when no run references a project, and an unresolved id degrades to the "render unavailable" card. Verified 2 of the 3 cases fail against the pre-fix page.

Closes #4148

Add a `?ids=a,b,c` batch filter to GET /creative-director (backed by a
getProjectsByIds read on both the PG and file project backends) and switch
the Creative Commission detail page to it, so resolving a commission's
render history costs one bounded round trip instead of fetching every
Creative Director project on the install.
Review follow-ups: extract csvIdsParam into lib/sharedSchemas.js (catalog keeps
its silent-truncate contract via the truncate flag), move the client's by-ids
fetch into services/apiBatch.js so the catalog and Creative Director helpers
share one implementation, and cover the wrapper's dedupe / empty-list
short-circuit / envelope unwrap with a service test.
@atomantic

Copy link
Copy Markdown
Owner Author

Review round 1 — claude (headless, high effort)

No correctness bugs found. Four smaller findings, all applied in 9fe309d except one consciously declined:

  1. DRY — the ?ids= CSV schema was a copy of catalogIngredientQuerySchema. Applied: extracted csvIdsParam({ max, maxIdLength, truncate }) into server/lib/sharedSchemas.js (the leaf module that exists precisely so domain validation files can share fragments without TDZ-ing through validation.js). Both schemas now call it; catalog passes truncate: true to keep its long-standing silently-slice-at-50 contract, CD keeps the 400.
  2. DRY — the client batch helper was a copy of listCatalogIngredientsByIds. Applied as fetchByIds(path, ids, options) in a new client/src/services/apiBatch.js rather than in apiCore.js. Deliberate: apiCatalog.test.js mocks ./apiCore.js wholesale, and a helper defined inside apiCore.js would close over the real request and escape that mock. A separate module keeps the interception working for every existing suite.
  3. Unreachable res?.items branch in the CD helper — resolved by (2): the shared helper legitimately needs the envelope unwrap for catalog's paginated list.
  4. Test gap on the client wrapper. Applied: new client/src/services/apiCreativeDirector.test.js covers the CSV query, dedupe/falsy-drop, the empty-list short-circuit, envelope unwrap, junk-response tolerance, and options passthrough.

Declined (design note, not a finding): a present-but-all-blank ?ids= falls through to the full list rather than returning []. Kept — ?ids= blank is indistinguishable from absent, matches the catalog precedent, and is documented + tested. The client can't hit it: fetchByIds short-circuits an empty list with no request at all, which is exactly why that short-circuit is load-bearing.

…ds (#4148)

Codex review follow-ups:
- csvIdsParam now normalizes the repeated ?ids=a&ids=b array form through the
  same trim / blank-drop / cap path as the CSV form, instead of passing arrays
  through unnormalized.
- The Creative Director batch accepts 120-char ids (the per-record peer-sync
  recordId bound) rather than the helper's 64-char default, so a project that
  arrived from a peer is still resolvable.
- fetchByIds trims ids and drops non-strings, so a whitespace-only id can no
  longer serialize to a blank ?ids= that the server reads as absent and answers
  with the full unfiltered list. normalizeIds is shared with apiCatalog's
  request-order re-sort so both index the same normalized list.
@atomantic

Copy link
Copy Markdown
Owner Author

Review round 2 — codex (headless, read-only sandbox)

Three findings, all real, all fixed in 37b628c:

  1. server/lib/sharedSchemas.js — the repeated ?ids=a&ids=b form bypassed normalization. Express hands that over as an array, and the preprocess returned arrays untouched — so blanks weren't dropped and the truncate path never applied. csvIdsParam now flattens/trims/blank-drops both wire forms through one path. Regression test: routes/creativeDirector.test.js asserts the array form produces the same service call as the CSV form, and that an over-cap array 400s.
  2. server/lib/creativeDirectorValidation.js — the 64-char default id bound was too tight. A locally minted id is cd-<uuid> (39 chars), but a project that arrived from a peer may carry anything the per-record sync recordId contract accepts (server/lib/validation.js, max 120) — that project would have 400'd out of its own batch. Now maxIdLength: 120, with a test for a 120-char id. Catalog keeps its existing 64.
  3. client/src/services/apiBatch.js — whitespace-only ids survived filter(Boolean). They serialize to ?ids=%20, which the server trims back to absent and answers with the full unfiltered project list — precisely the over-fetch this PR removes. fetchByIds now trims and drops non-strings via a shared normalizeIds, which apiCatalog's request-order re-sort also uses so both index the same normalized list. Regression test asserts an all-whitespace list issues no request.

Test status after the fixes: server 1389 files / 29065 tests and client 645 files / 7842 tests pass. The only red files (routes/health.test.js, services/updateExecutor.test.js) fail identically on a clean origin/main checkout — environmental (an ambient PORTOS_FORCE_CLEAN_WORKSPACES var, plus gh/forge reachability), untouched by this PR.

@atomantic

Copy link
Copy Markdown
Owner Author

Reviewer status

  • claude — 1 round (its configured cap). No correctness bugs; 4 smaller findings, 3 applied and 1 declined with rationale (see the round-1 comment above).
  • codex — 1 substantive round: 3 findings, all real, all fixed in 37b628c with regression tests. The confirmation re-review could not run: the codex CLI is on PATH and was invoked against the updated diff, but the provider returned You've hit your usage limit … try again at Aug 20th. That is a quota block, not a missing reviewer.

In place of the blocked confirmation pass, the three fixes are each pinned by a test that fails without them: the array-form normalization and the 120-char peer id in server/routes/creativeDirector.test.js, and the whitespace-id no-request case in client/src/services/apiCreativeDirector.test.js. Full server + client suites are at parity with clean main.

@atomantic
atomantic merged commit af249b8 into main Aug 15, 2026
7 checks passed
@atomantic
atomantic deleted the claim/issue-4148 branch August 15, 2026 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resolve a commission's render projects without fetching every CD project

1 participant