fix(video-importer): harden packaging and upload diagnostics#9229
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR enforces environment variable validation at startup, migrates GraphQL authentication from optional JWT to required Firebase tokens, adds public URL verification to R2 uploads, makes Slack integration mandatory with misconfiguration alerts, and provides test fixture infrastructure and documentation. ChangesVideo Importer Environment Validation and Upload Verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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 |
Contributor
…les-in-video-importer'
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
apps/video-importer/src/importers/video.spec.ts (2)
108-146: ⚡ Quick winAdd coverage for the behaviors actually changed in this PR.
The success test verifies counts but never asserts that the new
publicUrlargument is forwarded touploadToR2, and there is no test for the new failure branch whereuploadToR2rejects and the recorded failure message includesformatR2AssetDiagnostic(r2Asset)output ("R2 upload/public verification: …"). Those two behaviors are the core changes invideo.tsfor this PR and should be pinned by tests.As per coding guidelines: "Add or update co-located
*.spec.tstests for parsing, validation, retry/status behavior, and import branching touched by a change".Suggested additions
assert.equal(uploadToR2Mock.mock.calls.length, 1) + const uploadArgs = uploadToR2Mock.mock.calls[0].arguments[0] + assert.equal(uploadArgs.publicUrl, 'https://cdn.example.com/video.mp4') + assert.equal(uploadArgs.uploadUrl, 'https://upload.example.com/video.mp4') assert.equal(requestMock.mock.calls.length, 1) assert.equal(markFileAsCompletedMock.mock.calls.length, 1) }) + + it('should record an upload/public-verification failure with diagnostic info when uploadToR2 rejects', async () => { + validateVideoAndEditionMock.mock.mockImplementation(async () => ({ + editionId: 'edition-id' + })) + getVideoMetadataMock.mock.mockImplementation(async () => ({ + durationMs: 10000, + duration: 10, + width: 1920, + height: 1080 + })) + createR2AssetMock.mock.mockImplementation(async () => ({ + id: 'asset-1', + uploadUrl: 'https://upload.example.com/video.mp4', + publicUrl: 'https://cdn.example.com/video.mp4' + })) + uploadToR2Mock.mock.mockImplementation(async () => { + throw new Error('Public URL verification failed: HEAD 403 Forbidden') + }) + mock.method(console, 'error', () => {}) + const summary = createProcessingSummary(1) + + await processVideoFile( + '0_JesusVisionJohn---base---3934---1.mp4', + '/tmp/0_JesusVisionJohn---base---3934---1.mp4', + 1024, + summary + ) + + assert.equal(summary.failed, 1) + assert.equal(formatR2AssetDiagnosticMock.mock.calls.length, 1) + assert.equal(requestMock.mock.calls.length, 0) + assert.equal(markFileAsCompletedMock.mock.calls.length, 0) + const failure = summary.failedFiles?.[0] + assert.ok( + failure?.reason?.startsWith('R2 upload/public verification:'), + 'failure reason should be prefixed with R2 upload/public verification' + ) + assert.ok( + failure?.reason?.includes('assetId=test-asset'), + 'failure reason should include diagnostic info from formatR2AssetDiagnostic' + ) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/video-importer/src/importers/video.spec.ts` around lines 108 - 146, The success test needs to assert that the created R2 asset's publicUrl is forwarded to uploadToR2 and you must add a new failure test that simulates uploadToR2 rejecting and verifies the recorded failure message includes formatR2AssetDiagnostic(r2Asset) output; update the existing spec using the existing helpers (createProcessingSummary and processVideoFile) by changing the success case to inspect uploadToR2Mock.mock.calls[0] to ensure the second/appropriate argument equals the publicUrl from createR2AssetMock, and add a new it(...) that mocks createR2AssetMock to return a known uploadUrl/publicUrl, makes uploadToR2Mock.mockImplementation(async () => { throw new Error("upload failed") }), runs processVideoFile, and then asserts summary.failed === 1 and that summary.failedFiles or failure messages contain the formatted diagnostic string produced by formatR2AssetDiagnostic(r2Asset).
56-58: 💤 Low valueAvoid
anyin test helper; type the module-exports shape.
getModuleExportsreturnsany, which propagates loosely-typedVIDEO_FILENAME_REGEX/processVideoFileassignments below. For ES module imports undernode:test, the dynamicimport()already yields the namespace object; themodule.exports/defaultfallbacks here are unlikely to be needed, and the function can simply return the typed module.As per coding guidelines: "Define a type if possible."
Proposed fix
-function getModuleExports<T>(module: T): any { - return (module as any)['module.exports'] ?? (module as any).default ?? module -} +function getModuleExports<T extends object>(module: T): T { + return module +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/video-importer/src/importers/video.spec.ts` around lines 56 - 58, getModuleExports currently returns any and uses CommonJS fallbacks; change it to return a properly typed module so VIDEO_FILENAME_REGEX and processVideoFile are strongly typed. Replace the body with a simple identity that returns the generic type: change the signature to getModuleExports<T>(module: T): T and return module (remove the module.exports/default fallbacks), or define a specific interface (e.g. { VIDEO_FILENAME_REGEX: RegExp; processVideoFile: (...args:any[]) => any }) and use getModuleExports<ThatInterface>(...) so callers (VIDEO_FILENAME_REGEX, processVideoFile) get correct types.apps/video-importer/src/video-importer.ts (1)
215-237: ⚡ Quick winDrop the post-run Slack config branches after preflight.
Once
checkStartupEnvironment()has let the run continue, missing or partial Slack credentials should already have terminated the process. Keeping these checks here leaves a second source of truth that can drift from the preflight rules; this can simplify to a single!options.dryRun && options.slackgate.Possible simplification
- const slackTokenConfigured = - typeof process.env.SLACK_BOT_TOKEN === 'string' && - process.env.SLACK_BOT_TOKEN.trim().length > 0 - const slackChannelConfigured = - typeof process.env.SLACK_CHANNEL_ID === 'string' && - process.env.SLACK_CHANNEL_ID.trim().length > 0 - - if ( - !options.dryRun && - options.slack && - slackTokenConfigured !== slackChannelConfigured - ) { - console.warn( - '[video-importer] Slack is partially configured: set both SLACK_BOT_TOKEN and SLACK_CHANNEL_ID to enable notifications.' - ) - } - - if ( - !options.dryRun && - options.slack && - slackTokenConfigured && - slackChannelConfigured - ) { + if (!options.dryRun && options.slack) { try { const { postVideoImporterSlackSummary } = await import( /* webpackChunkName: "video-importer-slack" */ './services/slack' ) await postVideoImporterSlackSummary({ folderPath, summary })As per coding guidelines, keep failure paths explicit: log the failing stage, increment
summary.failed, and return before later side effects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/video-importer/src/video-importer.ts` around lines 215 - 237, The Slack pre/post-run checks duplicate logic already enforced by checkStartupEnvironment(); remove the partial-configuration branches around the slackTokenConfigured/slackChannelConfigured booleans and replace the two conditional blocks with a single guard if (!options.dryRun && options.slack) that assumes preflight validated credentials; if you must handle a failure here instead, make the failure path explicit by logging the failing stage (e.g., using processLogger or console.warn), incrementing summary.failed, and returning immediately so later side-effects are not executed; reference the existing symbols options, checkStartupEnvironment, summary.failed, slackTokenConfigured and slackChannelConfigured to locate and simplify the logic.apps/video-importer/src/startupPreflight.ts (1)
5-21: ⚡ Quick winUse a single shared env schema to prevent drift.
requiredEnvSchemaduplicates the contract inenv.ts, which can silently diverge and cause inconsistent startup/runtime validation. Consider exporting a shared schema fragment and reusing it in both places.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/video-importer/src/startupPreflight.ts` around lines 5 - 21, requiredEnvSchema duplicates the same contract in env.ts; export the common schema from env.ts (e.g., export const baseEnvSchema or sharedRequiredEnvSchema) and import it here instead of redefining everything, then use z.object({...shared fields...}) or z.intersection/shared .extend to compose any local additions so both startupPreflight.ts's requiredEnvSchema and the runtime env validation share the exact same schema symbol (reference requiredEnvSchema -> sharedRequiredEnvSchema and the env.ts export names in your edits).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/video-importer/src/services/r2.ts`:
- Around line 182-196: The verification currently accepts a 200 response and
then calls rangeResponse.arrayBuffer(), which can download the entire object if
the CDN ignores the Range header; change the logic in the verification block to
either reject any non-206 status (i.e., throw unless rangeResponse.status ===
206) or, if you must accept 200, avoid arrayBuffer() and instead read only the
first byte via the streaming API (use rangeResponse.body.getReader().read() or
response.arrayBuffer slice techniques) and validate bytes.byteLength > 0 before
closing the reader; update the code around the Range request and the use of
rangeResponse.arrayBuffer() accordingly to prevent full-body buffering (refer to
the rangeResponse variable and the Range request with
R2_PUBLIC_URL_VERIFY_TIMEOUT_MS).
- Around line 168-173: The strict string equality check between
headResponse.headers.get('content-length') and String(expectedContentLength) is
fragile; change the check to parse the header when present (e.g., const cl =
headResponse.headers.get('content-length'); if (cl != null) { const parsed =
Number(cl); if (Number.isFinite(parsed) && parsed !== expectedContentLength)
throw new Error(...); } ) and skip/avoid throwing when the header is missing or
unparsable (optionally log a warning) — keep the numeric comparison against
expectedContentLength and retain the overall verifyFileUpload flow.
In `@apps/video-importer/src/services/slack.ts`:
- Around line 444-470: The Slack alert text currently blames the shipped .env
file; update the message produced in the postSlackMessage call (the block that
uses hostname, errorLines and extraLine) so it describes an invalid runtime
configuration instead (e.g., "The importer could not start on `hostname` due to
invalid runtime configuration/overrides") and change the remediation text in the
context element (currently referencing the shipped .env) to a runtime-agnostic,
actionable instruction (e.g., verify environment variables, secrets and shell
overrides or reconfigure the runtime) while keeping checkStartupEnvironment() as
the validation source and preserving existing variables (hostname, errorLines,
extraLine).
In `@apps/video-importer/test-videos/.gitignore`:
- Around line 2-7: Add ignore patterns for `.failed` fixture artifacts: update
the .gitignore used for test fixtures to include a generic *.failed plus the
specific extension variants (*.mp4.failed, *.srt.failed, *.aac.failed) so the
importer-created .failed status files cannot be accidentally committed; ensure
these patterns appear alongside the existing *.mp4, *.mp4.completed, *.srt,
*.srt.completed, *.aac, and *.aac.completed entries.
In `@apps/video-importer/test-videos/README.md`:
- Line 14: Update the README sentence that currently states "It also recreates
the intentionally invalid MP4 and subtitle fixture" to accurately reflect that
only the MP4 fixture is intentionally invalid; change the phrase to something
like "It also recreates the intentionally invalid MP4 fixture and the valid
subtitle fixture" so readers are not misled — search for the phrase
"intentionally invalid MP4 and subtitle fixture" in README.md to locate and edit
the line.
---
Nitpick comments:
In `@apps/video-importer/src/importers/video.spec.ts`:
- Around line 108-146: The success test needs to assert that the created R2
asset's publicUrl is forwarded to uploadToR2 and you must add a new failure test
that simulates uploadToR2 rejecting and verifies the recorded failure message
includes formatR2AssetDiagnostic(r2Asset) output; update the existing spec using
the existing helpers (createProcessingSummary and processVideoFile) by changing
the success case to inspect uploadToR2Mock.mock.calls[0] to ensure the
second/appropriate argument equals the publicUrl from createR2AssetMock, and add
a new it(...) that mocks createR2AssetMock to return a known
uploadUrl/publicUrl, makes uploadToR2Mock.mockImplementation(async () => { throw
new Error("upload failed") }), runs processVideoFile, and then asserts
summary.failed === 1 and that summary.failedFiles or failure messages contain
the formatted diagnostic string produced by formatR2AssetDiagnostic(r2Asset).
- Around line 56-58: getModuleExports currently returns any and uses CommonJS
fallbacks; change it to return a properly typed module so VIDEO_FILENAME_REGEX
and processVideoFile are strongly typed. Replace the body with a simple identity
that returns the generic type: change the signature to
getModuleExports<T>(module: T): T and return module (remove the
module.exports/default fallbacks), or define a specific interface (e.g. {
VIDEO_FILENAME_REGEX: RegExp; processVideoFile: (...args:any[]) => any }) and
use getModuleExports<ThatInterface>(...) so callers (VIDEO_FILENAME_REGEX,
processVideoFile) get correct types.
In `@apps/video-importer/src/startupPreflight.ts`:
- Around line 5-21: requiredEnvSchema duplicates the same contract in env.ts;
export the common schema from env.ts (e.g., export const baseEnvSchema or
sharedRequiredEnvSchema) and import it here instead of redefining everything,
then use z.object({...shared fields...}) or z.intersection/shared .extend to
compose any local additions so both startupPreflight.ts's requiredEnvSchema and
the runtime env validation share the exact same schema symbol (reference
requiredEnvSchema -> sharedRequiredEnvSchema and the env.ts export names in your
edits).
In `@apps/video-importer/src/video-importer.ts`:
- Around line 215-237: The Slack pre/post-run checks duplicate logic already
enforced by checkStartupEnvironment(); remove the partial-configuration branches
around the slackTokenConfigured/slackChannelConfigured booleans and replace the
two conditional blocks with a single guard if (!options.dryRun && options.slack)
that assumes preflight validated credentials; if you must handle a failure here
instead, make the failure path explicit by logging the failing stage (e.g.,
using processLogger or console.warn), incrementing summary.failed, and returning
immediately so later side-effects are not executed; reference the existing
symbols options, checkStartupEnvironment, summary.failed, slackTokenConfigured
and slackChannelConfigured to locate and simplify the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4459711d-0ebe-433b-a16a-56ac05e98ecb
📒 Files selected for processing (17)
apps/video-importer/AGENTS.mdapps/video-importer/README.mdapps/video-importer/src/env.tsapps/video-importer/src/gql/graphqlClient.tsapps/video-importer/src/importers/audiopreview.tsapps/video-importer/src/importers/subtitle.tsapps/video-importer/src/importers/video.spec.tsapps/video-importer/src/importers/video.tsapps/video-importer/src/services/r2.tsapps/video-importer/src/services/slack.tsapps/video-importer/src/startupPreflight.tsapps/video-importer/src/types.tsapps/video-importer/src/utils/startupPreflight.spec.tsapps/video-importer/src/video-importer.tsapps/video-importer/test-videos/.gitignoreapps/video-importer/test-videos/README.mdapps/video-importer/test-videos/reset-fixtures.sh
Kneesal
approved these changes
May 21, 2026
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.
Summary
Hardens the video importer packaging and runtime diagnostics on top of the burned-in subtitle branch.
What Changed
Verification
pnpm dlx nx run video-importer:testpnpm dlx nx run video-importer:type-checkpnpm dlx nx run video-importer:lintpnpm dlx nx run video-importer:package3 successful / 3 failedSummary by CodeRabbit
New Features
Documentation
Tests