feat: add content-creation API endpoints and video persistence#259
Merged
sweetmantech merged 24 commits intotestfrom Mar 12, 2026
Merged
feat: add content-creation API endpoints and video persistence#259sweetmantech merged 24 commits intotestfrom
sweetmantech merged 24 commits intotestfrom
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a content-creation subsystem: four Next.js API routes plus validators, handlers for estimate/templates/validate/create, artist repo readiness checks, Trigger.dev task triggering, video persistence to Supabase/storage, and supporting utilities/constants. (34 words) Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as "API Route"
participant Validator as "Validator"
participant Auth as "Auth"
participant Readiness as "Readiness Service"
participant Trigger as "Trigger.dev"
participant TaskSvc as "Task Service"
participant TaskHandler as "Task Run Handler"
participant Persist as "Persist Video"
participant Storage as "Supabase Storage"
Client->>API: POST /api/content/create
API->>Validator: validateCreateContentBody(request)
Validator->>Auth: validateAuthContext
Auth-->>Validator: auth context
Validator-->>API: validated payload
API->>Readiness: getArtistContentReadiness(accountId, artistSlug)
Readiness-->>API: readiness {ready, missing, warnings}
alt ready
API->>Trigger: triggerCreateContent(payload)
Trigger->>TaskSvc: tasks.trigger(CREATE_CONTENT_TASK_ID, payload)
TaskSvc-->>Trigger: run handle / runId
Trigger-->>API: handle
API-->>Client: 202 Accepted {runId}
else not ready
API-->>Client: 400 Bad Request {missing}
end
Client->>TaskHandler: GET /api/tasks/{runId}
TaskHandler->>TaskSvc: fetch run
TaskSvc-->>TaskHandler: run {status, output}
alt run has video output
TaskHandler->>Persist: persistCreateContentRunVideo(run)
Persist->>Storage: download/upload video & create file record
Storage-->>Persist: file metadata + signed URL
Persist-->>TaskHandler: hydrated run with video metadata
end
TaskHandler-->>Client: 200 OK {run}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/tasks/getTaskRunHandler.ts (1)
27-47:⚠️ Potential issue | 🟠 MajorPrevent hydration side-effects from breaking run retrieval.
At Line 27 and Line 43, a persistence failure currently turns a successful run fetch into a 500. Polling should still return the run payload when hydration fails.
💡 Suggested fail-open fix
- const hydratedRuns = await Promise.all(runs.map(run => persistCreateContentRunVideo(run))); + const hydratedRuns = await Promise.all( + runs.map(async (run) => { + try { + return await persistCreateContentRunVideo(run); + } catch (error) { + console.warn(`Failed to persist content video for run ${run.id}:`, error); + return run; + } + }), + ); @@ - const hydratedRun = await persistCreateContentRunVideo(result); + let hydratedRun = result; + try { + hydratedRun = await persistCreateContentRunVideo(result); + } catch (error) { + console.warn(`Failed to persist content video for run ${result.id}:`, error); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tasks/getTaskRunHandler.ts` around lines 27 - 47, The hydration step using persistCreateContentRunVideo must not convert a successful fetch into a 500 on failure; change the batch and single-run paths to fail-open: for the runs array use Promise.allSettled (or individually try/catch each persistCreateContentRunVideo) and, when a persistence/hydration promise rejects, log the error and return the original run object in place of the hydrated one, and for the single-run path wrap the await persistCreateContentRunVideo(result) in a try/catch that logs the error and falls back to using result (instead of throwing), so retrieveTaskRun(validatedQuery.runId) responses are returned even if persistCreateContentRunVideo fails.
🧹 Nitpick comments (7)
lib/content/contentTemplates.ts (1)
27-33: Optional: Consider using a Set for O(1) lookup.The current implementation uses
.some()which is O(n). While the array is small, a Set would provide O(1) lookup and is a cleaner pattern for membership tests.♻️ Optional refactor using Set
+const SUPPORTED_TEMPLATE_NAMES = new Set( + CONTENT_TEMPLATES.map((t) => t.name) +); + /** * * `@param` template */ export function isSupportedContentTemplate(template: string): boolean { - return CONTENT_TEMPLATES.some(item => item.name === template); + return SUPPORTED_TEMPLATE_NAMES.has(template); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/contentTemplates.ts` around lines 27 - 33, Replace the linear lookup in isSupportedContentTemplate by precomputing a Set of template names and testing membership against it: create a Set (e.g., CONTENT_TEMPLATE_NAME_SET or CONTENT_TEMPLATE_NAMES) derived from CONTENT_TEMPLATES.map(t => t.name) and change isSupportedContentTemplate(template: string) to return thatSet.has(template) for O(1) membership checks; ensure the Set is initialized once at module load so existing callers of isSupportedContentTemplate and the CONTENT_TEMPLATES constant remain unaffected.lib/content/validateGetContentValidateQuery.ts (1)
7-16: Consider exporting the schema and using inferred types.Per coding guidelines, validation files should "export both the schema and inferred TypeScript type." Currently, the schema is defined inline and not exported, and
ValidatedGetContentValidateQueryis manually typed rather than inferred.♻️ Suggested refactor to align with conventions
-const getContentValidateQuerySchema = z.object({ +export const getContentValidateQuerySchema = z.object({ artist_slug: z .string({ message: "artist_slug is required" }) .min(1, "artist_slug cannot be empty"), }); -export type ValidatedGetContentValidateQuery = { - accountId: string; - artistSlug: string; -}; +export type GetContentValidateQueryInput = z.infer<typeof getContentValidateQuerySchema>; + +export type ValidatedGetContentValidateQuery = { + accountId: string; + artistSlug: string; +};Note: The return type
ValidatedGetContentValidateQueryincludesaccountIdfrom auth context, so a separate manual type for the full validated result is still appropriate. However, exporting the schema enables reuse and testing.As per coding guidelines: "Create validate functions in
validate<EndpointName>Body.tsorvalidate<EndpointName>Query.tsfiles that export both the schema and inferred TypeScript type"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/validateGetContentValidateQuery.ts` around lines 7 - 16, Export the existing Zod schema (getContentValidateQuerySchema) and add an inferred TypeScript type using z.infer to replace the manual shape for the query payload; e.g., export the schema and export a derived type like GetContentValidateQuery = z.infer<typeof getContentValidateQuerySchema>, while keeping the existing ValidatedGetContentValidateQuery (which includes accountId) as a separate manual type for the full validated result.lib/content/createContentHandler.ts (1)
14-70: Split this handler to reduce responsibility and keep it under the size guideline.This function currently mixes validation flow, readiness checks, trigger orchestration, and error shaping in one block; extracting small helpers would improve maintainability.
As per coding guidelines, domain functions should follow single responsibility and "Keep functions under 50 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/createContentHandler.ts` around lines 14 - 70, The createContentHandler function is doing validation, readiness checking, orchestration, and error shaping in one block; refactor by extracting three small helpers: validateCreateContentRequest(request) that calls validateCreateContentBody and returns the validated payload or a NextResponse; ensureReadiness(payload) that wraps getArtistContentReadiness and returns readiness or a NextResponse when not ready; and triggerContentCreation(payload) that calls triggerCreateContent and returns the handle. Replace the inlined branches in createContentHandler with calls to these helpers and a minimal try/catch that only converts thrown errors to the JSON error response (keeping createContentHandler under 50 lines), keeping references to validateCreateContentBody, getArtistContentReadiness, triggerCreateContent, and getCorsHeaders unchanged.lib/content/getContentEstimateHandler.ts (1)
6-7: Move pricing constants/profiles to a shared module to avoid drift.Keeping rates inline in this handler makes future pricing changes error-prone across endpoints/services.
As per coding guidelines, shared logic should be extracted into reusable utilities (DRY).
Also applies to: 33-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/getContentEstimateHandler.ts` around lines 6 - 7, The file defines pricing constants (e.g., BASE_IMAGE_TO_VIDEO_COST and BASE_AUDIO_TO_VIDEO_COST and the other pricing constants around lines 33-36) inline in getContentEstimateHandler; extract these into a shared module (e.g., a new exported pricing/profiles module) and replace the inline definitions with imports. Create a single source-of-truth module that exports named constants (or a pricing profile object) and update getContentEstimateHandler (and any other files referencing those values) to import the shared symbols so future pricing changes only require edits in that module.lib/content/validateGetContentEstimateQuery.ts (1)
7-13: Export the schema alongside the inferred type.
ValidatedGetContentEstimateQueryis exported, butgetContentEstimateQuerySchemais not. Export both to satisfy validate-module conventions and improve reuse in tests/handlers.Small update
-const getContentEstimateQuerySchema = z.object({ +export const getContentEstimateQuerySchema = z.object({As per coding guidelines:
lib/**/validate*.tsshould “export both the schema and inferred TypeScript type.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/validateGetContentEstimateQuery.ts` around lines 7 - 13, The file currently only exports the inferred type ValidatedGetContentEstimateQuery but not the schema itself; export getContentEstimateQuerySchema alongside the type so callers/tests can reuse the Zod schema. Locate the const getContentEstimateQuerySchema and make it exported (export const getContentEstimateQuerySchema) while keeping the existing export type ValidatedGetContentEstimateQuery unchanged.lib/content/getArtistContentReadiness.ts (1)
45-131: Refactor readiness checks into declarative rules.This function is long and repetitive (multiple near-identical
if (!hasFile) issues.push(...)blocks). A small rules array + helper would reduce duplication and keep logic easier to evolve.As per coding guidelines:
lib/**/*.tsasks for SRP, “Keep functions under 50 lines,” and “DRY: Consolidate similar logic into shared utilities.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/getArtistContentReadiness.ts` around lines 45 - 131, getArtistContentReadiness contains repetitive if(!hasFile...) blocks building the issues array; refactor by declaring a small rules array of objects (e.g., {path: "context/images/face-guide.png", severity: "required", fix: "..."}), iterate the rules to push to issues using the existing hasFile helper for file checks, and handle the songs/*.mp3 check as a separate rule that uses hasAnyMp3; retain the same output shape and the existing variables (getArtistContentReadiness, hasFile, hasAnyMp3, issues) but replace the repeated conditionals with a concise loop/helper that constructs issues and then computes requiredMissing and warnings as before.lib/content/persistCreateContentRunVideo.ts (1)
47-133: Split this workflow into smaller helpers.Lines 47-133 combine eligibility checks, persistence, and output shaping in one function. Extracting helpers (e.g.,
shouldPersistRun,persistOrReuseVideoFile,toRunVideoOutput) will reduce complexity and improve maintainability.As per coding guidelines:
lib/**/*.tsrequires SRP and to “Keep functions under 50 lines” plus “DRY: Consolidate similar logic into shared utilities.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/persistCreateContentRunVideo.ts` around lines 47 - 133, The function persistCreateContentRunVideo is doing too much—combine eligibility checks, file persistence/reuse, and output shaping—so split it into small helpers: implement shouldPersistRun(run) to encapsulate the early-return checks (taskIdentifier vs CREATE_CONTENT_TASK_ID, isCompleted, and required output fields), implement persistOrReuseVideoFile(output, run.id) to handle selectFileByStorageKey + createSignedFileUrlByKey branch, fetching the video (fetch + response.ok check), uploadFileByKey, createFileRecord and return a file record (or existing file) and implement toRunVideoOutput(run, fileRecord) to shape the run.output.video payload (fileId, storageKey, fileName, mimeType, sizeBytes, signedUrl); then refactor persistCreateContentRunVideo to call these helpers in sequence and simply return the augmented run. Keep helper names as shown and reuse existing unique functions (selectFileByStorageKey, createSignedFileUrlByKey, uploadFileByKey, createFileRecord, CREATE_CONTENT_TASK_ID) to avoid duplicating logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/content/createContentHandler.ts`:
- Around line 61-67: The handler currently returns error.message to clients
which can leak internal details; update the error path in createContentHandler
(the block using NextResponse.json and getCorsHeaders) to log the full error
internally (e.g., console.error or your logger with the caught error) and return
a generic JSON error payload like { status: "error", error: "Internal server
error" } with status 500 and CORS headers; do not include error.message or stack
in the response.
In `@lib/content/getArtistContentReadiness.ts`:
- Around line 52-56: The current code in getArtistContentReadiness calling
selectAccountSnapshots masks operational query failures as a "No GitHub
repository" data error; update selectAccountSnapshots to surface DB/query
failures by throwing an error (instead of returning an empty array on failure),
and change getArtistContentReadiness to only throw "No GitHub repository found
for this account" when snapshots is an empty array (no configured snapshot)
while allowing actual query errors from selectAccountSnapshots to propagate (or
be caught and rethrown as an operational error) so that selectAccountSnapshots
and the caller distinguish config-missing vs query-failure cases.
In `@lib/content/getContentValidateHandler.ts`:
- Around line 34-40: The handler currently returns raw exception text via
error.message; change it to return a sanitized error response (e.g., { status:
"error", error: "Failed to validate content readiness" }) instead of the raw
message, and ensure the original error is logged internally (use the existing
logger or console.error) for diagnostics; update the code around
NextResponse.json and getCorsHeaders to send the generic 500 response while
logging the original `error` object (not exposed to the response).
In `@lib/content/persistCreateContentRunVideo.ts`:
- Around line 64-113: The code currently does a read-before-write using
selectFileByStorageKey then createFileRecord which can race under concurrent
requests; modify persistCreateContentRunVideo to handle conflicts by wrapping
createFileRecord in a try/catch and on insert failure (unique constraint
conflict for storageKey) re-run selectFileByStorageKey to fetch the existingFile
and return the run using that record (use its
id/storage_key/file_name/mime_type/size_bytes and createSignedFileUrlByKey), or
alternatively replace the insert with an atomic upsert if your DB client
supports it; reference selectFileByStorageKey, createFileRecord,
uploadFileByKey, storageKey and createdFile when implementing the recovery path.
In `@lib/content/validateCreateContentBody.ts`:
- Around line 16-23: The schema uses .default(DEFAULT_CONTENT_TEMPLATE) and
.default(false) before .optional(), which means explicit undefined won't be
replaced by the defaults; update the Zod definitions for template and lipsync to
either swap the chaining to .optional().default(...) so defaults apply when the
key is missing but still allow explicit values, or remove the .default(...) from
the schema for template and lipsync and rely on the existing defaulting logic in
the function body (DEFAULT_CONTENT_TEMPLATE and the code that sets lipsync) to
set defaults; adjust the schema entries for template, lipsync (and keep
account_id unchanged) accordingly.
---
Outside diff comments:
In `@lib/tasks/getTaskRunHandler.ts`:
- Around line 27-47: The hydration step using persistCreateContentRunVideo must
not convert a successful fetch into a 500 on failure; change the batch and
single-run paths to fail-open: for the runs array use Promise.allSettled (or
individually try/catch each persistCreateContentRunVideo) and, when a
persistence/hydration promise rejects, log the error and return the original run
object in place of the hydrated one, and for the single-run path wrap the await
persistCreateContentRunVideo(result) in a try/catch that logs the error and
falls back to using result (instead of throwing), so
retrieveTaskRun(validatedQuery.runId) responses are returned even if
persistCreateContentRunVideo fails.
---
Nitpick comments:
In `@lib/content/contentTemplates.ts`:
- Around line 27-33: Replace the linear lookup in isSupportedContentTemplate by
precomputing a Set of template names and testing membership against it: create a
Set (e.g., CONTENT_TEMPLATE_NAME_SET or CONTENT_TEMPLATE_NAMES) derived from
CONTENT_TEMPLATES.map(t => t.name) and change
isSupportedContentTemplate(template: string) to return thatSet.has(template) for
O(1) membership checks; ensure the Set is initialized once at module load so
existing callers of isSupportedContentTemplate and the CONTENT_TEMPLATES
constant remain unaffected.
In `@lib/content/createContentHandler.ts`:
- Around line 14-70: The createContentHandler function is doing validation,
readiness checking, orchestration, and error shaping in one block; refactor by
extracting three small helpers: validateCreateContentRequest(request) that calls
validateCreateContentBody and returns the validated payload or a NextResponse;
ensureReadiness(payload) that wraps getArtistContentReadiness and returns
readiness or a NextResponse when not ready; and triggerContentCreation(payload)
that calls triggerCreateContent and returns the handle. Replace the inlined
branches in createContentHandler with calls to these helpers and a minimal
try/catch that only converts thrown errors to the JSON error response (keeping
createContentHandler under 50 lines), keeping references to
validateCreateContentBody, getArtistContentReadiness, triggerCreateContent, and
getCorsHeaders unchanged.
In `@lib/content/getArtistContentReadiness.ts`:
- Around line 45-131: getArtistContentReadiness contains repetitive
if(!hasFile...) blocks building the issues array; refactor by declaring a small
rules array of objects (e.g., {path: "context/images/face-guide.png", severity:
"required", fix: "..."}), iterate the rules to push to issues using the existing
hasFile helper for file checks, and handle the songs/*.mp3 check as a separate
rule that uses hasAnyMp3; retain the same output shape and the existing
variables (getArtistContentReadiness, hasFile, hasAnyMp3, issues) but replace
the repeated conditionals with a concise loop/helper that constructs issues and
then computes requiredMissing and warnings as before.
In `@lib/content/getContentEstimateHandler.ts`:
- Around line 6-7: The file defines pricing constants (e.g.,
BASE_IMAGE_TO_VIDEO_COST and BASE_AUDIO_TO_VIDEO_COST and the other pricing
constants around lines 33-36) inline in getContentEstimateHandler; extract these
into a shared module (e.g., a new exported pricing/profiles module) and replace
the inline definitions with imports. Create a single source-of-truth module that
exports named constants (or a pricing profile object) and update
getContentEstimateHandler (and any other files referencing those values) to
import the shared symbols so future pricing changes only require edits in that
module.
In `@lib/content/persistCreateContentRunVideo.ts`:
- Around line 47-133: The function persistCreateContentRunVideo is doing too
much—combine eligibility checks, file persistence/reuse, and output shaping—so
split it into small helpers: implement shouldPersistRun(run) to encapsulate the
early-return checks (taskIdentifier vs CREATE_CONTENT_TASK_ID, isCompleted, and
required output fields), implement persistOrReuseVideoFile(output, run.id) to
handle selectFileByStorageKey + createSignedFileUrlByKey branch, fetching the
video (fetch + response.ok check), uploadFileByKey, createFileRecord and return
a file record (or existing file) and implement toRunVideoOutput(run, fileRecord)
to shape the run.output.video payload (fileId, storageKey, fileName, mimeType,
sizeBytes, signedUrl); then refactor persistCreateContentRunVideo to call these
helpers in sequence and simply return the augmented run. Keep helper names as
shown and reuse existing unique functions (selectFileByStorageKey,
createSignedFileUrlByKey, uploadFileByKey, createFileRecord,
CREATE_CONTENT_TASK_ID) to avoid duplicating logic.
In `@lib/content/validateGetContentEstimateQuery.ts`:
- Around line 7-13: The file currently only exports the inferred type
ValidatedGetContentEstimateQuery but not the schema itself; export
getContentEstimateQuerySchema alongside the type so callers/tests can reuse the
Zod schema. Locate the const getContentEstimateQuerySchema and make it exported
(export const getContentEstimateQuerySchema) while keeping the existing export
type ValidatedGetContentEstimateQuery unchanged.
In `@lib/content/validateGetContentValidateQuery.ts`:
- Around line 7-16: Export the existing Zod schema
(getContentValidateQuerySchema) and add an inferred TypeScript type using
z.infer to replace the manual shape for the query payload; e.g., export the
schema and export a derived type like GetContentValidateQuery = z.infer<typeof
getContentValidateQuerySchema>, while keeping the existing
ValidatedGetContentValidateQuery (which includes accountId) as a separate manual
type for the full validated result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a759052-531c-4fa4-a18d-1244940fac5e
⛔ Files ignored due to path filters (9)
lib/content/__tests__/createContentHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/content/__tests__/getArtistContentReadiness.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/content/__tests__/getContentEstimateHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/content/__tests__/getContentTemplatesHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/content/__tests__/getContentValidateHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/content/__tests__/persistCreateContentRunVideo.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/content/__tests__/validateCreateContentBody.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/tasks/__tests__/getTaskRunHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/trigger/__tests__/triggerCreateContent.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (19)
app/api/content/create/route.tsapp/api/content/estimate/route.tsapp/api/content/templates/route.tsapp/api/content/validate/route.tslib/const.tslib/content/contentTemplates.tslib/content/createContentHandler.tslib/content/getArtistContentReadiness.tslib/content/getContentEstimateHandler.tslib/content/getContentTemplatesHandler.tslib/content/getContentValidateHandler.tslib/content/persistCreateContentRunVideo.tslib/content/validateCreateContentBody.tslib/content/validateGetContentEstimateQuery.tslib/content/validateGetContentValidateQuery.tslib/supabase/files/selectFileByStorageKey.tslib/supabase/storage/createSignedFileUrlByKey.tslib/tasks/getTaskRunHandler.tslib/trigger/triggerCreateContent.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/content/getArtistContentReadiness.ts (1)
43-47:⚠️ Potential issue | 🟠 MajorDistinguish DB/query failures from “missing GitHub repo.”
Line 43 currently depends on
selectAccountSnapshots, which can return[]on query failure; Line 45-46 then misreports that as missing configuration. Please let operational errors propagate (or wrap as operational errors) and reserve this message for true no-repo cases only.As per coding guidelines,
lib/**/*.tsdomain functions must provide proper error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/getArtistContentReadiness.ts` around lines 43 - 47, Wrap the selectAccountSnapshots(accountId) call in a try/catch so genuine query/DB errors propagate (or are rethrown/wrapped as operational errors) instead of being reported as missing config; after the call, if snapshots.length === 0 throw an operational error like "Failed to load account snapshots for account <id>" to signal a query issue, and only when snapshots exist but snapshots[0]?.github_repo is falsy throw the existing "No GitHub repository found for this account" message; reference selectAccountSnapshots and the github_repo check in getArtistContentReadiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/content/getArtistContentReadiness.ts`:
- Around line 36-124: Split getArtistContentReadiness into small helpers:
extract selectAccountSnapshots + github_repo lookup into a function
fetchAccountRepo(accountId) that returns githubRepo; extract getRepoFileTree
call into fetchRepoTree(githubRepo); extract path utilities into
getArtistRootPrefix and hasFile/hasAnyMp3 helpers (e.g.,
buildHasFileChecker(blobPaths, artistRootPrefix)); encode the file checks as a
rule list (array of {path, severity, fix, matcher?: (blobPaths,
prefix)=>boolean}) and evaluate that list to produce issues; keep
getArtistContentReadiness as an orchestration function that calls
fetchAccountRepo, fetchRepoTree, buildHasFileChecker, runs the rule list to
gather issues, and returns the final shape (artist_slug, ready, missing,
warnings). Ensure helper names above match existing identifiers
(getArtistRootPrefix, getRepoFileTree, selectAccountSnapshots) so refs stay
consistent.
In `@lib/content/validateCreateContentBody.ts`:
- Around line 38-48: The response key "missing_fields" is misleading because
validation failures aren't always about missing fields; update the error payload
in the NextResponse.json call to use a neutral name like "field" or
"invalid_fields" (e.g., set "field": firstError.path or "invalid_fields":
firstError.path) and keep the existing "error": firstError.message and status
info; modify the code around result, firstError, and the NextResponse.json
invocation in validateCreateContentBody.ts to reflect the new key.
---
Duplicate comments:
In `@lib/content/getArtistContentReadiness.ts`:
- Around line 43-47: Wrap the selectAccountSnapshots(accountId) call in a
try/catch so genuine query/DB errors propagate (or are rethrown/wrapped as
operational errors) instead of being reported as missing config; after the call,
if snapshots.length === 0 throw an operational error like "Failed to load
account snapshots for account <id>" to signal a query issue, and only when
snapshots exist but snapshots[0]?.github_repo is falsy throw the existing "No
GitHub repository found for this account" message; reference
selectAccountSnapshots and the github_repo check in getArtistContentReadiness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea09bab6-8769-42df-af07-0eaeb4285fd2
📒 Files selected for processing (6)
lib/content/contentTemplates.tslib/content/getArtistContentReadiness.tslib/content/validateCreateContentBody.tslib/content/validateGetContentEstimateQuery.tslib/content/validateGetContentValidateQuery.tslib/tasks/getTaskRunHandler.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/content/contentTemplates.ts
- lib/content/validateGetContentValidateQuery.ts
- lib/content/validateGetContentEstimateQuery.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/content/createContentHandler.ts (1)
58-64:⚠️ Potential issue | 🟠 MajorDo not return raw exception messages in 500 responses.
At Line 59,
error.messageis sent to clients, which can expose internal details. Log the caught error internally and return a generic message in the response body.🔒 Proposed fix
} catch (error) { - const message = error instanceof Error ? error.message : "Failed to trigger content creation"; + console.error("Failed to trigger content creation", error); return NextResponse.json( { status: "error", - error: message, + error: "Failed to trigger content creation", }, { status: 500, headers: getCorsHeaders() }, ); }As per coding guidelines,
lib/**/*.tsdomain functions must ensure proper error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/createContentHandler.ts` around lines 58 - 64, The catch block in createContentHandler.ts currently returns raw error.message to the client; instead, log the full error internally (using the existing logger, e.g., processLogger.error(error) or console.error(error) if no logger is available) and return a generic 500 response body (e.g., status: "error", error: "Internal server error" or "Failed to create content") from the catch in createContentHandler to avoid leaking internal details; keep the internal log intact for debugging but never include error.message in the NextResponse returned to clients.lib/content/getArtistContentReadiness.ts (1)
43-46:⚠️ Potential issue | 🟠 MajorDistinguish snapshot query failures from “repo not configured.”
At Lines 43-46, an empty result is treated as missing repo, but
selectAccountSnapshotscan also return[]on DB/query failure. That masks operational issues as a data/config issue. Let query failures propagate (or be wrapped as operational errors) and reserve this message for truly missing snapshots/repo config.As per coding guidelines,
lib/**/*.tsdomain functions must ensure proper error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/getArtistContentReadiness.ts` around lines 43 - 46, The current code treats an empty snapshots array from selectAccountSnapshots(accountId) as "No GitHub repository found," which masks DB/query failures; update the logic so that if snapshots is an empty array you throw or propagate an operational/query error (e.g., "Snapshot query returned no rows for accountId X" or rethrow the DB error) while only throwing "No GitHub repository found for this account" when snapshots.length > 0 but snapshots[0].github_repo is null/undefined; reference selectAccountSnapshots, the snapshots variable, and the existing "No GitHub repository found for this account" message when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/content/createContentHandler.ts`:
- Around line 58-64: The catch block in createContentHandler.ts currently
returns raw error.message to the client; instead, log the full error internally
(using the existing logger, e.g., processLogger.error(error) or
console.error(error) if no logger is available) and return a generic 500
response body (e.g., status: "error", error: "Internal server error" or "Failed
to create content") from the catch in createContentHandler to avoid leaking
internal details; keep the internal log intact for debugging but never include
error.message in the NextResponse returned to clients.
In `@lib/content/getArtistContentReadiness.ts`:
- Around line 43-46: The current code treats an empty snapshots array from
selectAccountSnapshots(accountId) as "No GitHub repository found," which masks
DB/query failures; update the logic so that if snapshots is an empty array you
throw or propagate an operational/query error (e.g., "Snapshot query returned no
rows for accountId X" or rethrow the DB error) while only throwing "No GitHub
repository found for this account" when snapshots.length > 0 but
snapshots[0].github_repo is null/undefined; reference selectAccountSnapshots,
the snapshots variable, and the existing "No GitHub repository found for this
account" message when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c846073-4bc0-4d38-ae1f-2eef1547d7d1
⛔ Files ignored due to path filters (1)
lib/content/__tests__/getArtistContentReadiness.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (2)
lib/content/createContentHandler.tslib/content/getArtistContentReadiness.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/content/validateCreateContentBody.ts (1)
44-54:⚠️ Potential issue | 🟡 MinorError response key
missing_fieldsis misleading for validation errors.The
missing_fieldskey suggests fields are absent, but Zod validation errors can also indicate type mismatches, constraint violations, or other issues. This could confuse API consumers debugging their requests.🛠️ Suggested improvement
if (!result.success) { const firstError = result.error.issues[0]; return NextResponse.json( { status: "error", - missing_fields: firstError.path, + field: firstError.path, error: firstError.message, }, { status: 400, headers: getCorsHeaders() }, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/validateCreateContentBody.ts` around lines 44 - 54, The response uses a misleading key "missing_fields" for all Zod validation failures; update the error response in the validation failure branch (where result, firstError, NextResponse.json and getCorsHeaders are used) to return a more accurate payload such as "validation_errors" (or "errors") and include either the full result.error.issues array or map each issue to { path, message, code } so clients see type/constraint info as well as the field path; keep the status and 400 response unchanged.lib/content/createContentHandler.ts (1)
61-69:⚠️ Potential issue | 🟠 MajorDo not expose internal exception messages in 500 responses.
The catch block currently forwards
error.messageto clients, which can leak internal implementation details, stack traces, or sensitive information. This is a security concern and was flagged in a previous review.🔒 Suggested safe error handling
} catch (error) { - const message = error instanceof Error ? error.message : "Failed to trigger content creation"; + console.error("Failed to trigger content creation", error); return NextResponse.json( { status: "error", - error: message, + error: "Failed to trigger content creation", }, { status: 500, headers: getCorsHeaders() }, ); }As per coding guidelines, API handlers must provide proper error handling without exposing internals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/createContentHandler.ts` around lines 61 - 69, The catch block in createContentHandler.ts currently returns error.message to clients via NextResponse.json which leaks internals; change the response to return a generic error (e.g., { status: "error", error: "Internal server error" }) and keep the 500 + getCorsHeaders() behavior, and concurrently log the original error server-side using your logger or console.error(error) so the exception details are not exposed to the response; update the catch that references the error variable and the NextResponse.json call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/content/createContentHandler.ts`:
- Around line 61-69: The catch block in createContentHandler.ts currently
returns error.message to clients via NextResponse.json which leaks internals;
change the response to return a generic error (e.g., { status: "error", error:
"Internal server error" }) and keep the 500 + getCorsHeaders() behavior, and
concurrently log the original error server-side using your logger or
console.error(error) so the exception details are not exposed to the response;
update the catch that references the error variable and the NextResponse.json
call accordingly.
In `@lib/content/validateCreateContentBody.ts`:
- Around line 44-54: The response uses a misleading key "missing_fields" for all
Zod validation failures; update the error response in the validation failure
branch (where result, firstError, NextResponse.json and getCorsHeaders are used)
to return a more accurate payload such as "validation_errors" (or "errors") and
include either the full result.error.issues array or map each issue to { path,
message, code } so clients see type/constraint info as well as the field path;
keep the status and 400 response unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc29be99-6617-4e0d-a2a5-acd7d3e83276
⛔ Files ignored due to path filters (2)
lib/content/__tests__/createContentHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/content/__tests__/validateCreateContentBody.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (4)
lib/content/createContentHandler.tslib/content/getArtistContentReadiness.tslib/content/validateCreateContentBody.tslib/trigger/triggerCreateContent.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/content/getArtistContentReadiness.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/content/createContentHandler.ts (1)
79-85:⚠️ Potential issue | 🟠 MajorDo not expose internal exception messages in 500 responses.
Line 80/Line 84 forwards internal error text to clients. Return a generic message and log the original error server-side.
🔒 Suggested safe error handling
- } catch (error) { - const message = error instanceof Error ? error.message : "Failed to trigger content creation"; + } catch (error) { + console.error("Failed to trigger content creation", error); return NextResponse.json( { status: "error", - error: message, + error: "Failed to trigger content creation", }, { status: 500, headers: getCorsHeaders() }, ); }As per coding guidelines, "For domain functions, ensure: ... Proper error handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/createContentHandler.ts` around lines 79 - 85, The catch block in createContentHandler.ts currently exposes internal exception text to clients; instead, log the original error on the server (e.g., using console.error or your app logger) from inside the catch, and change the NextResponse.json payload returned by the createContentHandler catch to a generic 500-level error message (e.g., "Internal server error") without forwarding error.message; ensure you still return an appropriate HTTP 500 response via NextResponse with the status field set to "error" and do not include the original error string in the JSON.
🧹 Nitpick comments (1)
lib/content/createContentHandler.ts (1)
12-89: Refactor handler into smaller units to meet SRP and size constraints.
createContentHandlercurrently mixes validation, readiness checks, payload construction, trigger orchestration, and HTTP response mapping in one function, and exceeds the 50-line guideline.As per coding guidelines, "Apply Single Responsibility Principle (SRP): one exported function per file; each file should do one thing well" and "For domain functions, ensure: ... Keep functions under 50 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/createContentHandler.ts` around lines 12 - 89, The createContentHandler function is doing too many things and exceeds the size guideline—extract the readiness check, payload construction, batch vs single trigger orchestration, and HTTP response mapping into small helpers so the exported createContentHandler stays under ~50 lines and follows SRP. Specifically: keep createContentHandler as the top-level flow that calls validateCreateContentBody and then uses a new checkArtistReadiness(validated.accountId, validated.artistSlug) to return early on not-ready, call buildCreateContentPayload(validated, readiness.githubRepo) to construct payload, delegate parallel invocation to triggerBatchCreateContent(payload, validated.batch) or triggerSingleCreateContent(payload) depending on validated.batch, and use mapCreateContentResponse(result, validated) to build the NextResponse; move try/catch around only the orchestration call so error handling remains centralized. Ensure helper names (checkArtistReadiness, buildCreateContentPayload, triggerBatchCreateContent, triggerSingleCreateContent, mapCreateContentResponse) are used in the diff to locate and implement the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/content/createContentHandler.ts`:
- Around line 48-52: The batch path uses Promise.all with triggerCreateContent
which rejects the whole request on the first failed trigger; change this to
Promise.allSettled for validated.batch, then iterate the settled results to
collect successful runIds (from fulfilled results) and capture per-item errors
(from rejected results) so the handler returns/records partial successes rather
than throwing; update the code around the handles variable and any response
construction to use the settled results (reference: validated.batch,
triggerCreateContent, handles).
In `@lib/github/getOrgRepoUrls.ts`:
- Around line 17-21: The call to getRepoGitModules in getOrgRepoUrls.ts
hardcodes branch "main"; either document this constraint in the function JSDoc
(e.g., state getOrgRepoUrls assumes sandbox repos use "main" as default) or
refactor getOrgRepoUrls to accept a branch parameter or resolve the repo default
branch dynamically and pass it to getRepoGitModules(); locate getOrgRepoUrls and
update its signature to accept an optional branch (or look up
repoInfo.branch/default branch via the same pattern used in
expandSubmoduleEntries.ts) and replace the hardcoded "main" with that variable
before calling getRepoGitModules.
---
Duplicate comments:
In `@lib/content/createContentHandler.ts`:
- Around line 79-85: The catch block in createContentHandler.ts currently
exposes internal exception text to clients; instead, log the original error on
the server (e.g., using console.error or your app logger) from inside the catch,
and change the NextResponse.json payload returned by the createContentHandler
catch to a generic 500-level error message (e.g., "Internal server error")
without forwarding error.message; ensure you still return an appropriate HTTP
500 response via NextResponse with the status field set to "error" and do not
include the original error string in the JSON.
---
Nitpick comments:
In `@lib/content/createContentHandler.ts`:
- Around line 12-89: The createContentHandler function is doing too many things
and exceeds the size guideline—extract the readiness check, payload
construction, batch vs single trigger orchestration, and HTTP response mapping
into small helpers so the exported createContentHandler stays under ~50 lines
and follows SRP. Specifically: keep createContentHandler as the top-level flow
that calls validateCreateContentBody and then uses a new
checkArtistReadiness(validated.accountId, validated.artistSlug) to return early
on not-ready, call buildCreateContentPayload(validated, readiness.githubRepo) to
construct payload, delegate parallel invocation to
triggerBatchCreateContent(payload, validated.batch) or
triggerSingleCreateContent(payload) depending on validated.batch, and use
mapCreateContentResponse(result, validated) to build the NextResponse; move
try/catch around only the orchestration call so error handling remains
centralized. Ensure helper names (checkArtistReadiness,
buildCreateContentPayload, triggerBatchCreateContent,
triggerSingleCreateContent, mapCreateContentResponse) are used in the diff to
locate and implement the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 716262bb-a055-4a07-b727-a39227c4fe2d
⛔ Files ignored due to path filters (2)
lib/content/__tests__/createContentHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/content/__tests__/validateCreateContentBody.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (4)
lib/content/createContentHandler.tslib/content/getArtistContentReadiness.tslib/content/validateCreateContentBody.tslib/github/getOrgRepoUrls.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/content/validateCreateContentBody.ts
- lib/content/getArtistContentReadiness.ts
sweetmantech
approved these changes
Mar 5, 2026
- fix: z.coerce.boolean() treated 'false' as true in estimate query params - fix: remove account_id from create content body (derive from auth only) - fix: missing field now returns only required issues, not all issues - fix: make video hydration best-effort so GET /tasks/runs doesn't crash - refactor: derive DEFAULT_CONTENT_TEMPLATE from CONTENT_TEMPLATES array (DRY) - refactor: export getContentValidateQuerySchema for reuse
- config/content-creation/config.json is now 'recommended' not 'required' - artists only need face-guide.png + at least one .mp3 to be ready - pipeline will use default model/resolution settings if no config exists - add test for config-optional readiness check
- extend TriggerCreateContentPayload with githubRepo field - getArtistContentReadiness now returns githubRepo - createContentHandler passes githubRepo when triggering task
- add caption_length (short/medium/long) to request schema, defaults to short - pass through to task payload - restore proper validation flow in handler - update tests
- getArtistContentReadiness checks main repo first, falls back to org repos - reads .gitmodules to discover org submodule URLs - gatsby-grace now validates as ready=true
- add batch param (1-30, default 1)
- batch mode returns { runIds: [...] } instead of { runId }
- each task runs independently with its own random selections
- move inline getOrgRepoUrls from getArtistContentReadiness into lib/github/ - reuse existing getRepoGitModules + parseGitModules instead of duplicating - getArtistContentReadiness now imports the shared utility
- rename misleading 'missing_fields' to 'field' in validation error response - don't expose internal error messages in 500 responses (security) - log errors server-side, return generic message to clients
- distinguish 'no repo configured' vs 'query failed' in readiness check - sanitize 500 error in validate handler (don't expose internals) - make persistence idempotent (re-select on insert race condition) - fix Zod .optional().default() ordering for correct behavior - batch mode uses Promise.allSettled (partial failures return successful runIds) - getOrgRepoUrls accepts configurable branch param
Per reviewer feedback: single mode and batch mode now both return runIds array. Removes the runId/runIds split. Clients always handle one shape.
- getArtistRootPrefix → lib/content/getArtistRootPrefix.ts - getArtistFileTree → lib/content/getArtistFileTree.ts - isCompletedRun + TriggerRunLike → lib/content/isCompletedRun.ts - booleanFromString → lib/content/booleanFromString.ts Co-Authored-By: Claude Opus 4.6 <[email protected]>
- POST /api/content/create accepts artist_account_id OR artist_slug - GET /api/content/validate accepts artist_account_id OR artist_slug - resolveArtistSlug looks up artist name from accounts table, converts to slug - matches docs contract (either identifier works) - 404 if artist_account_id not found, 400 if neither provided
Per reviewer feedback: one ID system, not two. - artist_account_id is the only accepted identifier - API resolves account_id → name → slug internally - removes artist_slug from create and validate schemas - matches how the rest of the Recoup platform works
raw.githubusercontent.com returns 404 for private repos. Switched to api.github.com/repos/.../contents/ which works with Bearer token.
Validation can't reliably see files in org submodule repos (private repo GitHub API limitations). The task has its own submodule-aware file discovery. Validation now warns but doesn't block — task fails with clear error if files are actually missing.
sidneyswift
force-pushed
the
feat/content-creation-pipeline
branch
from
7071131 to
7dd354e
Compare
- Add artistAccountId to validated types and pass through handlers - Replace artist_slug with artist_account_id in API responses - artistSlug remains as internal impl detail for file system paths - Update all content tests to match Co-Authored-By: Claude Opus 4.6 <[email protected]>
…er/whatsapp - Move dynamic import to static top-level import in createContentHandler - Add proper mock in test instead of relying on dynamic import workaround - Install missing @chat-adapter/whatsapp package (fixes pre-existing test failure) All 1460 tests passing. Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Return ready=false instead of throwing when repo tree is null - Add repo/branch context to GitHub API error logs Co-Authored-By: Claude Opus 4.6 <[email protected]>
Prevents leaking the internal GitHub repository URL in the public API response by destructuring it out before serializing. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Decouples run status checks from video storage uploads. Video persistence will be handled separately in a future iteration. Co-Authored-By: Claude Opus 4.6 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
POST /api/content/create— triggers background content pipeline, returns runIdGET /api/content/templates— lists available content templatesGET /api/content/validate— checks artist readiness for content creationGET /api/content/estimate— returns cost estimatesArchitecture boundary
uploadFileByKey+createFileRecordpatternGET /api/tasks/runs(no new status endpoint)Related PRs
Verification
Summary by CodeRabbit
New Features
Bug Fixes