docs: make SRP/DRY review expectations explicit in AGENTS#258
Open
sidneyswift wants to merge 2 commits intotestfrom
Open
docs: make SRP/DRY review expectations explicit in AGENTS#258sidneyswift wants to merge 2 commits intotestfrom
sidneyswift wants to merge 2 commits intotestfrom
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive content creation API feature with four new endpoints and supporting infrastructure. It includes request validation layers, delegated handler functions, Trigger.dev integration for background job processing, and Supabase storage integration for persisting video outputs. The feature supports template management, cost estimation, artist readiness validation, and content creation workflows. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as API Route:<br/>create
participant Validator as Validator:<br/>validateCreateContentBody
participant Auth as Auth Context
participant Readiness as getArtistContentReadiness
participant GitHub as GitHub API
participant Trigger as Trigger.dev
participant BgJob as Background Job:<br/>CreateContent
Client->>API: POST /api/content/create<br/>(artist_slug, template, lipsync)
API->>Validator: validateCreateContentBody(request)
Validator->>Auth: validateAuthContext()
Auth-->>Validator: accountId (or NextResponse)
Validator-->>API: ValidatedCreateContentBody | NextResponse
alt Validation Failed
API-->>Client: 400 Bad Request (CORS headers)
else Validation Passed
API->>Readiness: getArtistContentReadiness(accountId, artistSlug)
Readiness->>GitHub: Fetch repo file tree
GitHub-->>Readiness: File paths
Readiness-->>API: ArtistContentReadiness<br/>(ready, missing, warnings)
alt Artist Not Ready
API-->>Client: 400 Bad Request<br/>(readiness details, CORS headers)
else Artist Ready
API->>Trigger: triggerCreateContent(payload)
Trigger-->>BgJob: Queue task
Trigger-->>API: { runId, ... }
API-->>Client: 202 Accepted<br/>(runId, status, CORS headers)
BgJob->>BgJob: Process video creation
end
end
sequenceDiagram
actor Client
participant API as API Route:<br/>validate
participant Validator as validateGetContentValidateQuery
participant Auth as Auth Context
participant Readiness as getArtistContentReadiness
participant GitHub as GitHub API
Client->>API: GET /api/content/validate<br/>?artist_slug=...
API->>Validator: validateGetContentValidateQuery(request)
Validator->>Auth: validateAuthContext()
Auth-->>Validator: accountId (or NextResponse)
alt Auth Failed
Validator-->>API: NextResponse
API-->>Client: 401/403 Unauthorized (CORS headers)
else Auth Passed
Validator-->>API: { accountId, artistSlug }
API->>Readiness: getArtistContentReadiness(accountId, artistSlug)
Readiness->>GitHub: Fetch repo file tree
GitHub-->>Readiness: File paths
Readiness-->>API: ArtistContentReadiness
API-->>Client: 200 Success<br/>{ status, ready, missing, warnings }<br/>(CORS headers)
end
sequenceDiagram
participant BgJob as Background Job:<br/>Completed
participant Hydrator as persistCreateContentRunVideo
participant Fetch as Fetch Video
participant Storage as Supabase Storage
participant Database as Supabase Database
participant FileQuery as selectFileByStorageKey
participant SignedURL as createSignedFileUrlByKey
BgJob->>Hydrator: [Completed Run]
alt Task is CREATE_CONTENT
Hydrator->>FileQuery: selectFileByStorageKey(storageKey)
alt File Exists
FileQuery-->>Hydrator: FileRecord
Hydrator->>SignedURL: createSignedFileUrlByKey(storageKey)
SignedURL-->>Hydrator: signed URL
Hydrator-->>BgJob: [Run with video metadata]
else File Not Found
FileQuery-->>Hydrator: null
Hydrator->>Fetch: Fetch videoSourceUrl
Fetch-->>Hydrator: blob, mimeType
Hydrator->>Storage: Upload blob with storageKey
Storage-->>Hydrator: { path: ... }
Hydrator->>Database: Create file record<br/>(ownerAccountId, artistAccountId, storageKey, etc.)
Database-->>Hydrator: FileRecord (with fileId)
Hydrator->>SignedURL: createSignedFileUrlByKey(storageKey)
SignedURL-->>Hydrator: signed URL
Hydrator-->>BgJob: [Run with video metadata & signed URL]
end
else Other Task Type
Hydrator-->>BgJob: [Unchanged Run]
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes The changes span 17+ new modules with heterogeneous complexity. While API routes and most handlers follow consistent patterns (low effort), two modules—getArtistContentReadiness (131 lines, GitHub API integration with file tree inspection and validation logic) and persistCreateContentRunVideo (133 lines, multi-step video fetch/upload/storage/database workflow)—contain dense, intricate logic requiring careful architectural review. Understanding interactions across validation chains, handler delegation, Supabase integration, and Trigger.dev job triggering adds cognitive load. The varied nature of edits across domain-specific concerns (auth, file operations, job triggering, GitHub API) necessitates separate reasoning per module family. 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)
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: 8
🧹 Nitpick comments (6)
lib/content/validateGetContentEstimateQuery.ts (1)
7-13: Export the schema constant for consistency with validator conventions.
ValidatedGetContentEstimateQueryis exported, but the schema itself is not. Exporting both keeps validation contracts reusable and consistent.As per coding guidelines,
lib/**/validate*.ts: “... 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, Export the existing Zod schema constant so it’s available alongside the inferred type: add an export for getContentEstimateQuerySchema (the z.object(...) currently declared) so both getContentEstimateQuerySchema and ValidatedGetContentEstimateQuery are exported; update the declaration of getContentEstimateQuerySchema to be exported (export const getContentEstimateQuerySchema = ...) to follow the validator convention and keep the schema and type reusable.lib/content/contentTemplates.ts (1)
7-25: Remove duplicated default template literal to avoid drift.
DEFAULT_CONTENT_TEMPLATErepeats"artist-caption-bedroom"fromCONTENT_TEMPLATES. Derive it from the source list so updates stay consistent.As per coding guidelines,
**/*.{ts,tsx}: “Extract shared logic into reusable utilities following Don't Repeat Yourself (DRY) principle”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/contentTemplates.ts` around lines 7 - 25, DEFAULT_CONTENT_TEMPLATE duplicates the literal "artist-caption-bedroom"; change it to be derived from the source array to avoid drift by referencing CONTENT_TEMPLATES so updates stay consistent. Replace the hardcoded DEFAULT_CONTENT_TEMPLATE value with a computed value that reads CONTENT_TEMPLATES (for example using CONTENT_TEMPLATES[0].name or a small helper like getDefaultTemplateName()) after CONTENT_TEMPLATES is declared, ensuring the symbol DEFAULT_CONTENT_TEMPLATE is exported and CONTENT_TEMPLATES remains the single source of truth.lib/content/validateGetContentValidateQuery.ts (1)
7-16: Export the Zod schema and inferred query type from this validator module.This file currently keeps the schema private and uses a manual output type. Please export a schema type (
z.infer) for the validated query contract to match repository validator conventions.As per coding guidelines,
lib/**/validate*.ts: “Create validate functions ... 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, The schema is currently private and the output type is hand-written; export the Zod schema and derive the TS type from it: export the existing getContentValidateQuerySchema and replace the manual ValidatedGetContentValidateQuery definition with export type ValidatedGetContentValidateQuery = z.infer<typeof getContentValidateQuerySchema>, ensuring the module exports both the schema and the inferred type so callers can import the validator contract directly.lib/trigger/triggerCreateContent.ts (1)
4-9: Extract this payload shape into a shared type to prevent drift.
TriggerCreateContentPayloadoverlaps with content run output fields inlib/content/persistCreateContentRunVideo.ts(account/template/lipsync identifiers). A shared contract type would reduce divergence risk.As per coding guidelines,
**/*.{ts,tsx}: “Extract shared logic into reusable utilities following Don't Repeat Yourself (DRY) principle”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/trigger/triggerCreateContent.ts` around lines 4 - 9, Extract the overlapping shape into a single exported shared type (e.g., ContentCreatePayload) instead of duplicating it: create the new shared type matching the fields in TriggerCreateContentPayload and the content run output used in persistCreateContentRunVideo.ts, replace TriggerCreateContentPayload with the new shared type, and update all imports/usages (including where persistCreateContentRunVideo references account/template/lipsync) to import the shared type so both modules reference the same contract and prevent divergence.lib/content/getArtistContentReadiness.ts (1)
45-120: Refactor repeated file checks into a declarative rule set.This function is doing too much in one block and repeats near-identical issue construction six times. Extracting checks into a config list + small helper will improve readability and keep the function below the 50-line target.
As per coding guidelines:
lib/**/*.ts: For domain functions, ensure ... Keep functions under 50 linesandDRY: 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 - 120, The getArtistContentReadiness function repeats nearly identical file-check logic; refactor by extracting a declarative rules array (each item: path, severity, fix, and a check predicate) and a small helper (e.g., applyRule or checkRule) that runs the predicate using existing helpers hasFile/hasAnyMp3 and pushes the issue to issues when false; replace the repeated if-blocks with a single loop over rules to construct issues, keeping getArtistContentReadiness concise and under 50 lines while reusing the hasFile and hasAnyMp3 symbols.lib/content/createContentHandler.ts (1)
47-49: Add explicit return type totriggerCreateContentfor type safety.The function lacks a return type annotation and relies on the Trigger.dev SDK contract rather than compile-time guarantees. While the code works (accessing
handle.idsucceeds), explicit typing strengthens the type system and aligns with TypeScript best practices.Consider adding the return type from
@trigger.dev/sdk:export async function triggerCreateContent(payload: TriggerCreateContentPayload): Promise<TaskRunHandle> { const handle = await tasks.trigger(CREATE_CONTENT_TASK_ID, payload); return handle; }Note: Similar trigger functions (
triggerSetupSandbox,triggerRunSandboxCommand) also lack explicit return types and would benefit from the same treatment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/content/createContentHandler.ts` around lines 47 - 49, Add explicit return type annotations to the trigger helper functions to enforce compile-time safety: update triggerCreateContent to declare its return type as Promise<TaskRunHandle> and annotate its payload parameter with TriggerCreateContentPayload imported from `@trigger.dev/sdk`; do the same for triggerSetupSandbox and triggerRunSandboxCommand (use the appropriate payload types for each) so each function signature explicitly returns Promise<TaskRunHandle> and prevents relying solely on the Trigger.dev runtime contract.
🤖 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 122-129: The return currently sets missing to the full issues
array causing duplication; change the returned object in
getArtistContentReadiness (the block that builds artist_slug, ready, missing,
warnings) to set missing: requiredMissing instead of missing: issues so that
missing contains only items from requiredMissing while warnings remains the
recommended filter.
In `@lib/content/persistCreateContentRunVideo.ts`:
- Around line 64-113: Add a DB uniqueness guard on (owner_account_id,
storage_key) and make the create path resilient to races: ensure the files table
has a unique constraint/index on (owner_account_id, storage_key), then update
the createFileRecord flow (or persistCreateContentRunVideo where
selectFileByStorageKey, uploadFileByKey and createFileRecord are used) to handle
conflicts by catching the unique-violation error (or using an upsert/INSERT ...
ON CONFLICT DO NOTHING/DO UPDATE) and then re-selecting the existing row via
selectFileByStorageKey to return its id/signedUrl; this keeps upload+create
idempotent and prevents duplicate records when concurrent requests race.
- Around line 90-93: The code calls fetch(output.videoSourceUrl) without
validation — add explicit URL validation before fetching: in the
persistCreateContentRunVideo flow (the block using output.videoSourceUrl / the
const response = await fetch(...) call) construct a URL object from
output.videoSourceUrl, verify url.protocol === 'https:', and verify url.hostname
is in an allowlist (e.g., ALLOWED_VIDEO_HOSTS or a config-driven set); reject or
throw if validation fails. Also guard against hostnames that resolve to
localhost/private IPs (or use net.isIP + CIDR checks) and ensure only the
validated url.href is passed to fetch (no unvalidated redirects allowed). Ensure
the new allowlist constant and validation logic are used immediately before the
existing fetch call.
- Around line 47-133: persistCreateContentRunVideo is doing too much; split into
small helpers: (1) extract eligibility checks into a helper like
isEligibleForPersist(run) that returns the validated CreateContentOutput (moves
taskIdentifier/isCompleted and required fields checks), (2) extract
existing-file lookup into findExistingFile(ownerAccountId, storageKey) which
calls selectFileByStorageKey and createSignedFileUrlByKey and returns the mapped
video object if found, (3) extract download+upload+create-record flow into
downloadUploadCreateVideo(output, storageKey, fileName) which fetches
output.videoSourceUrl, validates response, reads blob, calls uploadFileByKey and
createFileRecord, then calls createSignedFileUrlByKey and returns the mapped
created video object, and (4) extract mapping into buildRunWithVideo(run,
output, videoObj) to return the updated run; then refactor
persistCreateContentRunVideo to orchestrate: call isEligibleForPersist, compute
fileName/storageKey, call findExistingFile and if not found call
downloadUploadCreateVideo, then return buildRunWithVideo. Reference functions:
persistCreateContentRunVideo, selectFileByStorageKey, createSignedFileUrlByKey,
uploadFileByKey, createFileRecord.
In `@lib/content/validateCreateContentBody.ts`:
- Line 22: Remove acceptance of account_id from the request schema and
auth-override logic: delete the account_id field from the Zod schema (the
property named account_id) and remove the code path that reads/uses
request-provided account_id in the validateCreateContentBody flow (the auth
override logic around account_id). Ensure account ID is obtained only from the
authentication context (the existing auth/account extraction used elsewhere in
validateCreateContentBody) and not from the request body or tool schema.
In `@lib/content/validateGetContentEstimateQuery.ts`:
- Around line 8-10: The schema fields lipsync and compare currently use
z.coerce.boolean(), which treats any non-empty string (e.g. "false") as true;
update these fields in validateGetContentEstimateQuery to explicitly parse
string query values by accepting only "true"/"false" and transforming to boolean
(e.g., replace z.coerce.boolean() for lipsync and compare with
z.enum(["true","false"]).transform(v => v === "true") or equivalent
z.string().pipe/transform logic) while keeping batch as the numeric coercion;
ensure the transformed fields default to false if absent.
In `@lib/supabase/files/selectFileByStorageKey.ts`:
- Around line 2-17: The function selectFileByStorageKey currently returns
Promise<FileRecord | null>; change its return type to use the DB table type
(Promise<Tables<"files"> | null>) to avoid drift with write-helper types—update
the function signature and any related imports/usages (remove or stop using
FileRecord in this file) so the function returns the Tables<"files"> shape
directly and callers are updated to expect that DB table type.
In `@lib/tasks/getTaskRunHandler.ts`:
- Line 27: The current use of Promise.all and direct await on
persistCreateContentRunVideo makes the whole GET fail if any hydration errors
occur; change hydration to be best-effort by catching per-run errors and falling
back to the original run payload: replace the Promise.all(runs.map(run =>
persistCreateContentRunVideo(run))) with a resilient pattern (either
Promise.allSettled over runs.map(...) and map settled results to either the
fulfilled value or the original run, or map runs to async functions that try {
return await persistCreateContentRunVideo(run) } catch (err) { return run }),
and similarly wrap the single await persistCreateContentRunVideo call in a
try/catch that returns the original run on error; keep function names
persistCreateContentRunVideo and variable hydratedRuns to locate the changes.
---
Nitpick comments:
In `@lib/content/contentTemplates.ts`:
- Around line 7-25: DEFAULT_CONTENT_TEMPLATE duplicates the literal
"artist-caption-bedroom"; change it to be derived from the source array to avoid
drift by referencing CONTENT_TEMPLATES so updates stay consistent. Replace the
hardcoded DEFAULT_CONTENT_TEMPLATE value with a computed value that reads
CONTENT_TEMPLATES (for example using CONTENT_TEMPLATES[0].name or a small helper
like getDefaultTemplateName()) after CONTENT_TEMPLATES is declared, ensuring the
symbol DEFAULT_CONTENT_TEMPLATE is exported and CONTENT_TEMPLATES remains the
single source of truth.
In `@lib/content/createContentHandler.ts`:
- Around line 47-49: Add explicit return type annotations to the trigger helper
functions to enforce compile-time safety: update triggerCreateContent to declare
its return type as Promise<TaskRunHandle> and annotate its payload parameter
with TriggerCreateContentPayload imported from `@trigger.dev/sdk`; do the same for
triggerSetupSandbox and triggerRunSandboxCommand (use the appropriate payload
types for each) so each function signature explicitly returns
Promise<TaskRunHandle> and prevents relying solely on the Trigger.dev runtime
contract.
In `@lib/content/getArtistContentReadiness.ts`:
- Around line 45-120: The getArtistContentReadiness function repeats nearly
identical file-check logic; refactor by extracting a declarative rules array
(each item: path, severity, fix, and a check predicate) and a small helper
(e.g., applyRule or checkRule) that runs the predicate using existing helpers
hasFile/hasAnyMp3 and pushes the issue to issues when false; replace the
repeated if-blocks with a single loop over rules to construct issues, keeping
getArtistContentReadiness concise and under 50 lines while reusing the hasFile
and hasAnyMp3 symbols.
In `@lib/content/validateGetContentEstimateQuery.ts`:
- Around line 7-13: Export the existing Zod schema constant so it’s available
alongside the inferred type: add an export for getContentEstimateQuerySchema
(the z.object(...) currently declared) so both getContentEstimateQuerySchema and
ValidatedGetContentEstimateQuery are exported; update the declaration of
getContentEstimateQuerySchema to be exported (export const
getContentEstimateQuerySchema = ...) to follow the validator convention and keep
the schema and type reusable.
In `@lib/content/validateGetContentValidateQuery.ts`:
- Around line 7-16: The schema is currently private and the output type is
hand-written; export the Zod schema and derive the TS type from it: export the
existing getContentValidateQuerySchema and replace the manual
ValidatedGetContentValidateQuery definition with export type
ValidatedGetContentValidateQuery = z.infer<typeof
getContentValidateQuerySchema>, ensuring the module exports both the schema and
the inferred type so callers can import the validator contract directly.
In `@lib/trigger/triggerCreateContent.ts`:
- Around line 4-9: Extract the overlapping shape into a single exported shared
type (e.g., ContentCreatePayload) instead of duplicating it: create the new
shared type matching the fields in TriggerCreateContentPayload and the content
run output used in persistCreateContentRunVideo.ts, replace
TriggerCreateContentPayload with the new shared type, and update all
imports/usages (including where persistCreateContentRunVideo references
account/template/lipsync) to import the shared type so both modules reference
the same contract and prevent divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 09dae1ab-89c9-44e7-9746-8d9b3123337c
⛔ Files ignored due to path filters (10)
AGENTS.mdis excluded by none and included by nonelib/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
Comment on lines
+122
to
+129
| const requiredMissing = issues.filter(item => item.severity === "required"); | ||
| const warnings = issues.filter(item => item.severity === "recommended"); | ||
|
|
||
| return { | ||
| artist_slug: artistSlug, | ||
| ready: requiredMissing.length === 0, | ||
| missing: issues, | ||
| warnings, |
There was a problem hiding this comment.
missing should return only required issues.
Line 128 currently returns all issues (required + recommended) in missing, while warnings already returns recommended items. Return requiredMissing in missing to avoid duplicated/misleading output.
✅ Suggested fix
return {
artist_slug: artistSlug,
ready: requiredMissing.length === 0,
- missing: issues,
+ missing: requiredMissing,
warnings,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const requiredMissing = issues.filter(item => item.severity === "required"); | |
| const warnings = issues.filter(item => item.severity === "recommended"); | |
| return { | |
| artist_slug: artistSlug, | |
| ready: requiredMissing.length === 0, | |
| missing: issues, | |
| warnings, | |
| const requiredMissing = issues.filter(item => item.severity === "required"); | |
| const warnings = issues.filter(item => item.severity === "recommended"); | |
| return { | |
| artist_slug: artistSlug, | |
| ready: requiredMissing.length === 0, | |
| missing: requiredMissing, | |
| warnings, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/content/getArtistContentReadiness.ts` around lines 122 - 129, The return
currently sets missing to the full issues array causing duplication; change the
returned object in getArtistContentReadiness (the block that builds artist_slug,
ready, missing, warnings) to set missing: requiredMissing instead of missing:
issues so that missing contains only items from requiredMissing while warnings
remains the recommended filter.
Comment on lines
+47
to
+133
| export async function persistCreateContentRunVideo<T extends TriggerRunLike>(run: T): Promise<T> { | ||
| if (run.taskIdentifier !== CREATE_CONTENT_TASK_ID || !isCompleted(run)) { | ||
| return run; | ||
| } | ||
|
|
||
| const output = (run.output ?? {}) as CreateContentOutput; | ||
| if (!output.accountId || !output.artistSlug || !output.videoSourceUrl) { | ||
| return run; | ||
| } | ||
|
|
||
| if (output.video?.storageKey) { | ||
| return run; | ||
| } | ||
|
|
||
| const fileName = `${output.artistSlug}-${run.id}.mp4`; | ||
| const storageKey = `content/${output.accountId}/${output.artistSlug}/${fileName}`; | ||
|
|
||
| const existingFile = await selectFileByStorageKey({ | ||
| ownerAccountId: output.accountId, | ||
| storageKey, | ||
| }); | ||
|
|
||
| if (existingFile) { | ||
| const signedUrl = await createSignedFileUrlByKey({ | ||
| key: existingFile.storage_key, | ||
| }); | ||
|
|
||
| return { | ||
| ...run, | ||
| output: { | ||
| ...output, | ||
| video: { | ||
| fileId: existingFile.id, | ||
| storageKey: existingFile.storage_key, | ||
| fileName: existingFile.file_name, | ||
| mimeType: existingFile.mime_type, | ||
| sizeBytes: existingFile.size_bytes, | ||
| signedUrl, | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| const response = await fetch(output.videoSourceUrl); | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to download rendered video: ${response.status} ${response.statusText}`); | ||
| } | ||
|
|
||
| const videoBlob = await response.blob(); | ||
| const mimeType = response.headers.get("content-type") || "video/mp4"; | ||
|
|
||
| await uploadFileByKey(storageKey, videoBlob, { | ||
| contentType: mimeType, | ||
| upsert: true, | ||
| }); | ||
|
|
||
| const createdFile = await createFileRecord({ | ||
| ownerAccountId: output.accountId, | ||
| // Phase 1: artist account mapping is not wired yet, so we scope to owner account. | ||
| artistAccountId: output.accountId, | ||
| storageKey, | ||
| fileName, | ||
| mimeType, | ||
| sizeBytes: videoBlob.size, | ||
| description: `Content pipeline output for ${output.artistSlug}`, | ||
| tags: ["content", "video", output.template ?? "unknown-template"], | ||
| }); | ||
|
|
||
| const signedUrl = await createSignedFileUrlByKey({ | ||
| key: createdFile.storage_key, | ||
| }); | ||
|
|
||
| return { | ||
| ...run, | ||
| output: { | ||
| ...output, | ||
| video: { | ||
| fileId: createdFile.id, | ||
| storageKey: createdFile.storage_key, | ||
| fileName: createdFile.file_name, | ||
| mimeType: createdFile.mime_type, | ||
| sizeBytes: createdFile.size_bytes, | ||
| signedUrl, | ||
| }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Split this into smaller helpers; current function exceeds SRP and size limits.
This method combines task filtering, remote I/O, storage, DB persistence, and output mapping. Extracting helpers will improve testability and reduce maintenance risk.
As per coding guidelines, lib/**/*.ts: “Single responsibility per function” 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/persistCreateContentRunVideo.ts` around lines 47 - 133,
persistCreateContentRunVideo is doing too much; split into small helpers: (1)
extract eligibility checks into a helper like isEligibleForPersist(run) that
returns the validated CreateContentOutput (moves taskIdentifier/isCompleted and
required fields checks), (2) extract existing-file lookup into
findExistingFile(ownerAccountId, storageKey) which calls selectFileByStorageKey
and createSignedFileUrlByKey and returns the mapped video object if found, (3)
extract download+upload+create-record flow into
downloadUploadCreateVideo(output, storageKey, fileName) which fetches
output.videoSourceUrl, validates response, reads blob, calls uploadFileByKey and
createFileRecord, then calls createSignedFileUrlByKey and returns the mapped
created video object, and (4) extract mapping into buildRunWithVideo(run,
output, videoObj) to return the updated run; then refactor
persistCreateContentRunVideo to orchestrate: call isEligibleForPersist, compute
fileName/storageKey, call findExistingFile and if not found call
downloadUploadCreateVideo, then return buildRunWithVideo. Reference functions:
persistCreateContentRunVideo, selectFileByStorageKey, createSignedFileUrlByKey,
uploadFileByKey, createFileRecord.
Comment on lines
+64
to
+113
| const existingFile = await selectFileByStorageKey({ | ||
| ownerAccountId: output.accountId, | ||
| storageKey, | ||
| }); | ||
|
|
||
| if (existingFile) { | ||
| const signedUrl = await createSignedFileUrlByKey({ | ||
| key: existingFile.storage_key, | ||
| }); | ||
|
|
||
| return { | ||
| ...run, | ||
| output: { | ||
| ...output, | ||
| video: { | ||
| fileId: existingFile.id, | ||
| storageKey: existingFile.storage_key, | ||
| fileName: existingFile.file_name, | ||
| mimeType: existingFile.mime_type, | ||
| sizeBytes: existingFile.size_bytes, | ||
| signedUrl, | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| const response = await fetch(output.videoSourceUrl); | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to download rendered video: ${response.status} ${response.statusText}`); | ||
| } | ||
|
|
||
| const videoBlob = await response.blob(); | ||
| const mimeType = response.headers.get("content-type") || "video/mp4"; | ||
|
|
||
| await uploadFileByKey(storageKey, videoBlob, { | ||
| contentType: mimeType, | ||
| upsert: true, | ||
| }); | ||
|
|
||
| const createdFile = await createFileRecord({ | ||
| ownerAccountId: output.accountId, | ||
| // Phase 1: artist account mapping is not wired yet, so we scope to owner account. | ||
| artistAccountId: output.accountId, | ||
| storageKey, | ||
| fileName, | ||
| mimeType, | ||
| sizeBytes: videoBlob.size, | ||
| description: `Content pipeline output for ${output.artistSlug}`, | ||
| tags: ["content", "video", output.template ?? "unknown-template"], | ||
| }); |
There was a problem hiding this comment.
The dedupe path is race-prone and can create duplicate file records.
The selectFileByStorageKey check followed by createFileRecord is non-atomic. Concurrent calls can both miss then insert. Add a DB uniqueness guard on (owner_account_id, storage_key) and handle conflict by selecting the existing row.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/content/persistCreateContentRunVideo.ts` around lines 64 - 113, Add a DB
uniqueness guard on (owner_account_id, storage_key) and make the create path
resilient to races: ensure the files table has a unique constraint/index on
(owner_account_id, storage_key), then update the createFileRecord flow (or
persistCreateContentRunVideo where selectFileByStorageKey, uploadFileByKey and
createFileRecord are used) to handle conflicts by catching the unique-violation
error (or using an upsert/INSERT ... ON CONFLICT DO NOTHING/DO UPDATE) and then
re-selecting the existing row via selectFileByStorageKey to return its
id/signedUrl; this keeps upload+create idempotent and prevents duplicate records
when concurrent requests race.
Comment on lines
+90
to
+93
| const response = await fetch(output.videoSourceUrl); | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to download rendered video: ${response.status} ${response.statusText}`); | ||
| } |
There was a problem hiding this comment.
Validate and constrain videoSourceUrl before fetching (SSRF risk).
fetch(output.videoSourceUrl) accepts untrusted URLs with no protocol/host allowlist checks. This can be abused to reach internal resources. Please enforce HTTPS + allowed hostnames before download.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/content/persistCreateContentRunVideo.ts` around lines 90 - 93, The code
calls fetch(output.videoSourceUrl) without validation — add explicit URL
validation before fetching: in the persistCreateContentRunVideo flow (the block
using output.videoSourceUrl / the const response = await fetch(...) call)
construct a URL object from output.videoSourceUrl, verify url.protocol ===
'https:', and verify url.hostname is in an allowlist (e.g., ALLOWED_VIDEO_HOSTS
or a config-driven set); reject or throw if validation fails. Also guard against
hostnames that resolve to localhost/private IPs (or use net.isIP + CIDR checks)
and ensure only the validated url.href is passed to fetch (no unvalidated
redirects allowed). Ensure the new allowlist constant and validation logic are
used immediately before the existing fetch call.
| .default(DEFAULT_CONTENT_TEMPLATE) | ||
| .optional(), | ||
| lipsync: z.boolean().default(false).optional(), | ||
| account_id: z.string().uuid("account_id must be a valid UUID").optional(), |
There was a problem hiding this comment.
Remove account_id from the request contract and auth override flow.
Line 22 and Lines 55–57 allow request-body account_id overrides. This breaks the endpoint contract and should be derived solely from authentication context.
🔒 Suggested contract-safe change
export const createContentBodySchema = z.object({
@@
- account_id: z.string().uuid("account_id must be a valid UUID").optional(),
});
@@
- const authResult = await validateAuthContext(request, {
- accountId: result.data.account_id,
- });
+ const authResult = await validateAuthContext(request);As per coding guidelines: Never use account_id in request bodies or tool schemas; always derive the account ID from authentication.
Also applies to: 55-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/content/validateCreateContentBody.ts` at line 22, Remove acceptance of
account_id from the request schema and auth-override logic: delete the
account_id field from the Zod schema (the property named account_id) and remove
the code path that reads/uses request-provided account_id in the
validateCreateContentBody flow (the auth override logic around account_id).
Ensure account ID is obtained only from the authentication context (the existing
auth/account extraction used elsewhere in validateCreateContentBody) and not
from the request body or tool schema.
Comment on lines
+8
to
+10
| lipsync: z.coerce.boolean().default(false), | ||
| batch: z.coerce.number().int().min(1).max(100).default(1), | ||
| compare: z.coerce.boolean().default(false), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Zod v4 z.coerce.boolean() string input behavior documentation
💡 Result:
In Zod v4, z.coerce.boolean() coerces by calling JavaScript’s built-in Boolean(input). (zod.dev)
That means for string inputs:
""→false- any non-empty string (including
"false","0","no") →true(developer.mozilla.org)
If you want "true"/"false" (and similar) parsed logically to booleans, Zod v4 provides z.stringbool() (with configurable truthy/falsy sets). (zod.dev)
Citations:
- 1: https://zod.dev/api
- 2: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean?utm_source=openai
- 3: https://zod.dev/api
z.coerce.boolean() will mis-handle "false" query values.
Query parameters arrive as strings. JavaScript's Boolean() function (which z.coerce.boolean() uses) treats any non-empty string—including "false", "0", "no"—as true. This means ?lipsync=false or ?compare=false will be parsed as true, causing unexpected behavior.
For query strings, explicitly parse string values first: use Zod's z.enum(["true", "false"]).transform(v => v === "true") or z.string().pipe(z.coerce.boolean()) with custom logic, rather than relying on direct coercion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/content/validateGetContentEstimateQuery.ts` around lines 8 - 10, The
schema fields lipsync and compare currently use z.coerce.boolean(), which treats
any non-empty string (e.g. "false") as true; update these fields in
validateGetContentEstimateQuery to explicitly parse string query values by
accepting only "true"/"false" and transforming to boolean (e.g., replace
z.coerce.boolean() for lipsync and compare with
z.enum(["true","false"]).transform(v => v === "true") or equivalent
z.string().pipe/transform logic) while keeping batch as the numeric coercion;
ensure the transformed fields default to false if absent.
Comment on lines
+2
to
+17
| import type { FileRecord } from "@/lib/supabase/files/createFileRecord"; | ||
|
|
||
| /** | ||
| * Select a file record by storage key for an owner account. | ||
| * | ||
| * @param root0 | ||
| * @param root0.ownerAccountId | ||
| * @param root0.storageKey | ||
| */ | ||
| export async function selectFileByStorageKey({ | ||
| ownerAccountId, | ||
| storageKey, | ||
| }: { | ||
| ownerAccountId: string; | ||
| storageKey: string; | ||
| }): Promise<FileRecord | null> { |
There was a problem hiding this comment.
Use Tables<"files"> for the return type instead of FileRecord.
This query reads DB rows directly; returning FileRecord (from write helper types) risks schema drift and nullability mismatches over time.
As per coding guidelines, lib/supabase/**/*.ts: “Return typed results using Tables<"table_name">”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/supabase/files/selectFileByStorageKey.ts` around lines 2 - 17, The
function selectFileByStorageKey currently returns Promise<FileRecord | null>;
change its return type to use the DB table type (Promise<Tables<"files"> |
null>) to avoid drift with write-helper types—update the function signature and
any related imports/usages (remove or stop using FileRecord in this file) so the
function returns the Tables<"files"> shape directly and callers are updated to
expect that DB table type.
| try { | ||
| if (validatedQuery.mode === "list") { | ||
| const runs = await listTaskRuns(validatedQuery.accountId, validatedQuery.limit); | ||
| const hydratedRuns = await Promise.all(runs.map(run => persistCreateContentRunVideo(run))); |
There was a problem hiding this comment.
Hydration failures currently fail the whole GET response.
Line 27 (Promise.all) and Line 43 (await persistCreateContentRunVideo) make response success dependent on hydration success. A single hydration error will return 500 even when run retrieval succeeded. Make hydration best-effort and fall back to the original run payload.
💡 Suggested resilient fallback
- 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 {
+ return run;
+ }
+ }),
+ );
@@
- const hydratedRun = await persistCreateContentRunVideo(result);
+ let hydratedRun = result;
+ try {
+ hydratedRun = await persistCreateContentRunVideo(result);
+ } catch {
+ // Best-effort hydration; return the retrieved run when hydration fails.
+ }Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/tasks/getTaskRunHandler.ts` at line 27, The current use of Promise.all
and direct await on persistCreateContentRunVideo makes the whole GET fail if any
hydration errors occur; change hydration to be best-effort by catching per-run
errors and falling back to the original run payload: replace the
Promise.all(runs.map(run => persistCreateContentRunVideo(run))) with a resilient
pattern (either Promise.allSettled over runs.map(...) and map settled results to
either the fulfilled value or the original run, or map runs to async functions
that try { return await persistCreateContentRunVideo(run) } catch (err) { return
run }), and similarly wrap the single await persistCreateContentRunVideo call in
a try/catch that returns the original run on error; keep function names
persistCreateContentRunVideo and variable hydratedRuns to locate the changes.
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
1 participant
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 this updates
AGENTS.mdresolveAccountId()Why
Recent review feedback requested stricter SRP/DRY compliance (utility/file splitting and schema reuse). This update makes expectations unambiguous for future agent-authored changes.
Scope
Summary by CodeRabbit