Unify-api #27
Unify-api #27
Conversation
WalkthroughThe changes update multiple modules to standardize file processing across audio, image, and video types. The Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as Server
participant PA as processAudioHandler
participant PAud as processAudio (Core)
C->>S: POST /audio/process with audio file & options
S->>PA: Route request to processAudioHandler
PA->>PAud: Validate options and invoke processAudio
PAud-->>PA: Yield processed audio chunks
PA->>S: Stream processed results or write output file
S-->>C: Return multipart/mixed response with ProcessedFile(s)
sequenceDiagram
participant App as Application
participant C as @m4k/client
participant S as M4K Server
App->>C: Call processAudio/processImage/processVideo with file and options
C->>S: Send processing request (via HTTP or Bun)
S-->>C: Return streaming response with ProcessedFile(s)
C-->>App: Provide processed output and progress updates
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (23)
packages/core/src/videos/video-processor.ts (1)
56-58: Consider handling potential filesystem errors.The
mkdiroperation could fail due to permission issues or other I/O errors. Since this is using a top-level await, an unhandled rejection could occur.-await mkdir(tmpDir, { recursive: true }); +try { + await mkdir(tmpDir, { recursive: true }); +} catch (error) { + console.error(`Failed to create temporary directory ${tmpDir}:`, error); + // Consider fallback or graceful handling +}packages/client/src/index.ts (1)
1-1: Remove unnecessary renaming in export.The export statement contains a useless rename that should be simplified.
-export { ProcessedFile as ProcessedFile } from "@m4k/common"; +export { ProcessedFile } from "@m4k/common";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
test.ts (2)
3-7: Consider verifying.namebefore using non-null assertion.
When callingprocessVideo(Bun.file("fixtures/video.mp4").name!, ...), you assume.nameis never null or undefined. To prevent potential runtime errors, consider a safer fallback approach:- for await (const value of processVideo(Bun.file("fixtures/video.mp4").name!, { + const fileRef = Bun.file("fixtures/video.mp4"); + for await (const value of processVideo(fileRef.name ?? "video.mp4", { format: "mp4", videoCodec: "libx265", output: "test.mp4", })!) {
11-26: Add error handling for file reading.
If the file at"fixtures/image.jpeg"doesn’t exist or cannot be read, the code could fail without a clear error path. A simple try/catch or a check before callingprocessImagecan improve resilience.packages/server/src/audio/process-audio-handler.spec.ts (3)
24-28: Rename variable to reflect audio processing context.
Currently,videoInforeferences audio metadata. Consider renaming it toaudioInfoor a more generic name likemediaInfoto avoid confusion.- const videoInfo = collected.find((x) => "type" in x)!; + const audioInfo = collected.find((x) => "type" in x)!;
30-60: Test name references “videos” instead of “audio.”
The test “should process multiple videos in sequence” might be copying from a video scenario. Since it tests audio with “.ogg” output, rename for clarity and consistency:- it("should process multiple videos in sequence", async () => { + it("should process multiple audio files in sequence", async () => {
157-157: Remove the unnecessarycontinuestatement.
The linter flags this as superfluous. You can safely drop it without changing the logic:- continue; // keepalive + // keepalive🧰 Tools
🪛 Biome (1.9.4)
[error] 157-157: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
packages/core/src/images/image-processor.ts (1)
17-91: Consider handling stream errors inside the queue task.
WrappingexhaustAsyncIterableToWritablecalls in try/catch helps ensure errors from reading or writing streams (e.g., file not found, insufficient permissions) are surfaced. This will improve the reliability of the image processing queue.+ try { ... ); // ... existing code ... + } catch (err) { + yield { error: (err as Error).message }; + } }, signal);packages/core/src/audio/audio-processor.spec.ts (1)
58-58: Remove trailing space in test descriptionThere's an extra space at the end of the test description.
- it("should enqueue processing multiple audio files ", async () => { + it("should enqueue processing multiple audio files", async () => {packages/server/src/util/response.ts (2)
1-1: Update import to use@m4k/commonnamespaceFor consistency with other files in the codebase, consider updating the import to use the
@m4k/commonnamespace instead of justm4k.-import { ProcessedFile } from "m4k"; +import { ProcessedFile } from "@m4k/common";
18-19: Avoid using{}as a typeThe type
{}is discouraged as it actually means "any non-nullable value" rather than "empty object" as commonly assumed. Consider using a more specific type orunknownfor better type safety.- iterable: AsyncIterable<{} | ProcessedFile | Blob> + iterable: AsyncIterable<unknown | ProcessedFile | Blob>🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/server/README.md (1)
19-25: Add language specifiers to fenced code blocksAdd language specifiers to the fenced code blocks to improve syntax highlighting and readability in the documentation.
-``` +```http URL: /audio/process?<Options|options=OptionsJSON[]> Method: POST Headers: - [X-Options: <OptionsJSON[]>] Body: Binary file```diff -``` +```http Status: 200 OK Headers: - Content-Type: multipart/mixed; boundary=<boundary> Body: multipart/mixed body with status updates and the processed files (if no output option was provided)Also applies to: 29-34 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 19-19: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>packages/core/src/audio/audio-processor.ts (3)</summary><blockquote> `6-12`: **Fix JSDoc comment** The JSDoc comment incorrectly states this function processes a video, but it should refer to audio processing. ```diff /** - * Process a video. + * Process an audio file. * @param input - The input video. Can be a file path, a stream or a blob. * @param opts - The options for the video processing. * @param signal - An optional abort signal. - * @returns An iterable of the processed videos. + * @returns An iterable of the processed audio. */
35-35: Consider using a more appropriate default format for audioThe default format is set to "mp4", which is primarily a video container format. Consider using a more appropriate default audio format like "mp3" or "aac".
- format: opts.format ?? "mp4", + format: opts.format ?? "mp3",
42-45: Update JSDoc commentThe JSDoc comment incorrectly refers to video processing for the audio queue.
/** - * The queue for video processing. + * The queue for audio processing. */packages/server/src/audio/process-audio-handler.ts (2)
9-21: Clarify the usage of theoptionsfield in the schema.
Currently,optionsis defined as an optional string field. If you expect more structured data, consider requiring JSON or specifying constraints for this field. This will improve readability and reduce parsing ambiguity.
40-59: Enhance streaming flow with robust cleanup.
While streaming or writing output files, consider using a try/finally block or similar approach to ensure partial or interrupted operations are handled cleanly. This prevents orphaned temp files if the request is abruptly closed.packages/client/README.md (2)
8-25: Include an advanced example for audio processing options.
Showcasing usage of advanced fields likebitrate,codec, orfilterswould help users configure audio processing more effectively.
27-44: Add clarity on the structure ofoptsfor image processing.
It may be beneficial to provide a short example demonstrating a minimaloptsobject, illustrating how to specify fields likeformatorfilters.packages/server/src/videos/process-video-handler.ts (1)
31-31: Consider consolidatingoutputandnameinto the schema.
Currently,optsArrextendsVideoOptionswithoutputandname. Including them in the validation schema (as optional fields) could make it clearer and reduce ad-hoc property extensions.packages/core/src/util/ffmpeg-processor.ts (1)
10-16: Doc comment mismatch.
The JSDoc references@param opts, but the actual signature hasbuildArgsplus{ signal, queue, tmpDir }. Consider aligning the doc comment with the new parameter names to avoid confusion./** - * @param opts - The options for video processing. + * @param buildArgs - Function returning input/output ffmpeg args and format. + * @param param1 - Destructured object containing signal, queue, and tmpDir. */packages/client/src/client.ts (2)
75-79: Consider renamingoptimizeFetchtoprocessFetchfor consistency.Since all other functions have been renamed from "optimize" to "process", it would be more consistent to rename this internal function as well.
-async function* optimizeFetch<T>( +async function* processFetch<T>( url: string, input: AsyncIterable<Uint8Array> | Blob, opts: any )And update the function calls in the processing functions:
-yield* optimizeFetch<Progress | QueuePosition | ProcessingError>( +yield* processFetch<Progress | QueuePosition | ProcessingError>(
88-90: Update error message to reflect the new naming.The error message still references "optimize" instead of "process".
- `Failed to optimize: [${res.statusText}] ${await res.text()}` + `Failed to process: [${res.statusText}] ${await res.text()}`
🛑 Comments failed to post (2)
packages/server/src/videos/process-video-handler.ts (1)
47-60: 🛠️ Refactor suggestion
Add a more robust error handling path for output file writing.
If an error occurs while writing the output file or removing the temporary file, it may leave partial files or lock resources. Protect these operations within a try/finally block or add error handling logic to ensure proper cleanup.packages/client/src/client.spec.ts (1)
60-82:
⚠️ Potential issuePipeline failure due to size mismatch.
Your test expects 1055836 bytes, but the pipeline reports 1055796. Container metadata or slight encoding differences can cause minor deviations.Consider a more tolerant check:
- expect(files).toEqual([ - { name: "video.mp4", type: "video/mp4", size: 1055836 }, - ]); + expect(files).toHaveLength(1); + expect(files[0].name).toBe("video.mp4"); + expect(files[0].type).toBe("video/mp4"); + // Allow a slight variation in file size + expect(Math.abs(files[0].size - 1055836)).toBeLessThanOrEqual(512);📝 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.describe("processVideo()", () => { it("should query the server to process a video", async () => { const video = Bun.file("../../fixtures/video.mp4"); const files: { name: string; type: string; size: number }[] = []; for await (const x of processVideo("http://localhost:3000", video, [ { format: "mp4", name: "video.mp4" }, ])) { if (x instanceof ProcessedFile) { const chunks = await Array.fromAsync(x.stream); files.push({ name: x.name, type: x.type, size: chunks.reduce((sum, chunk) => sum + chunk.length, 0), }); } } expect(files).toHaveLength(1); expect(files[0].name).toBe("video.mp4"); expect(files[0].type).toBe("video/mp4"); // Allow a slight variation in file size expect(Math.abs(files[0].size - 1055836)).toBeLessThanOrEqual(512); }); });🧰 Tools
🪛 GitHub Actions: Test and Build
[error] 78-78: Test failed: expect(received).toEqual(expected) - Expected size to be 1055836 but received 1055796.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
packages/client/src/index.ts (1)
1-1: Remove redundant type alias.The current export statement includes a useless rename. This can be simplified.
-export { ProcessedFile as ProcessedFile } from "@m4k/common"; +export { ProcessedFile } from "@m4k/common";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
packages/server/src/audio/process-audio-handler.spec.ts (5)
30-30: Fix typo in test description.The test description refers to processing "multiple videos" but should refer to "multiple audio files" to match the actual test content.
- it("should process multiple videos in sequence", async () => { + it("should process multiple audio files in sequence", async () => {
53-54: Variable naming inconsistency.The variable is named
video1but it's referring to an audio file. This should be renamed for clarity.- const video1 = coll1.find((x) => "type" in x)!; - expect(video1.filename).toMatch(/\.ogg$/); - expect(video1.size).toBeGreaterThan(1000); + const audio1 = coll1.find((x) => "type" in x)!; + expect(audio1.filename).toMatch(/\.ogg$/); + expect(audio1.size).toBeGreaterThan(1000);
57-59: Variable naming inconsistency.Similar to the previous comment, the variable is named
video2but refers to an audio file.- const video2 = coll2.find((x) => "type" in x)!; - expect(video2.filename).toMatch(/\.ogg$/); - expect(video2.size).toBeGreaterThan(1000); + const audio2 = coll2.find((x) => "type" in x)!; + expect(audio2.filename).toMatch(/\.ogg$/); + expect(audio2.size).toBeGreaterThan(1000);
157-157: Remove unnecessary continue statement.The continue statement is unnecessary as it's the last statement in the conditional block.
} else if (part.type === "text/plain") { - continue; // keepalive + // keepalive } else {🧰 Tools
🪛 Biome (1.9.4)
[error] 157-157: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
24-24: Variable naming inconsistency.The variable is named
videoInfobut it's referring to audio data. This should be renamed for clarity.- const videoInfo = collected.find((x) => "type" in x)!; - expect(videoInfo.filename).toEqual(expect.stringContaining(".ogg")); - expect(videoInfo.type).toEqual("audio/ogg"); - expect(videoInfo.size).toBeGreaterThan(1000); + const audioInfo = collected.find((x) => "type" in x)!; + expect(audioInfo.filename).toEqual(expect.stringContaining(".ogg")); + expect(audioInfo.type).toEqual("audio/ogg"); + expect(audioInfo.size).toBeGreaterThan(1000);packages/server/README.md (2)
19-25: Add language specifier to fenced code block.It's best practice to specify the language for fenced code blocks to enable proper syntax highlighting.
-``` +```http URL: /audio/process?<Options|options=OptionsJSON[]> Method: POST Headers: - [X-Options: <OptionsJSON[]>] Body: Binary file<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 19-19: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `29-34`: **Add language specifier to fenced code block.** It's best practice to specify the language for fenced code blocks to enable proper syntax highlighting. ```diff -``` +```http Status: 200 OK Headers: - Content-Type: multipart/mixed; boundary=<boundary> Body: multipart/mixed body with status updates and the processed files (if no output option was provided)<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 29-29: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>packages/core/src/util/ffmpeg-processor.ts (1)</summary><blockquote> `35-35`: **Await on inputPath could cause issues with string inputs** When `inputPath` is a string, `await inputPath` will still work (it will just return the string), but it's unnecessary and could be confusing. Consider checking the type before awaiting. ```diff - ["-y", ...inArgs, "-i", await inputPath, ...outArgs, outputPath], + ["-y", ...inArgs, "-i", typeof inputPath === "string" ? inputPath : await inputPath, ...outArgs, outputPath],packages/client/src/client.ts (1)
75-79: Helper function parameter types updatedThe
optimizeFetchfunction has been updated to accept AsyncIterable or Blob as input, aligning with the processing functions. Consider renaming this function toprocessFetchto maintain naming consistency.-async function* optimizeFetch<T>( +async function* processFetch<T>(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
fixtures/audio.mp3is excluded by!**/*.mp3
📒 Files selected for processing (19)
.gitignore(1 hunks)README.md(1 hunks)packages/client/README.md(2 hunks)packages/client/src/client.spec.ts(2 hunks)packages/client/src/client.ts(5 hunks)packages/client/src/index.ts(1 hunks)packages/common/src/index.ts(4 hunks)packages/core/README.md(1 hunks)packages/core/src/audio/audio-processor.spec.ts(1 hunks)packages/core/src/audio/audio-processor.ts(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/util/ffmpeg-processor.ts(3 hunks)packages/core/src/videos/video-processor.spec.ts(5 hunks)packages/core/src/videos/video-processor.ts(1 hunks)packages/server/README.md(1 hunks)packages/server/src/audio/process-audio-handler.spec.ts(1 hunks)packages/server/src/audio/process-audio-handler.ts(1 hunks)packages/server/src/server.ts(2 hunks)test-client.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- .gitignore
- packages/core/src/audio/audio-processor.spec.ts
- packages/core/src/videos/video-processor.ts
- packages/server/src/audio/process-audio-handler.ts
- packages/common/src/index.ts
- packages/core/src/videos/video-processor.spec.ts
- packages/core/src/audio/audio-processor.ts
- packages/server/src/server.ts
🧰 Additional context used
🧬 Code Definitions (3)
packages/server/src/audio/process-audio-handler.spec.ts (2)
packages/server/src/audio/process-audio-handler.ts (1)
processAudioHandler(25-60)packages/common/src/index.ts (3)
QueuePosition(5-7)Progress(1-3)ProcessingError(9-11)
test-client.ts (5)
packages/client/src/client.ts (3)
processAudio(25-35)processVideo(63-73)processImage(44-54)packages/core/src/audio/audio-processor.ts (1)
processAudio(13-40)packages/common/src/index.ts (1)
ProcessedFile(127-133)packages/core/src/videos/video-processor.ts (1)
processVideo(12-46)packages/core/src/images/image-processor.ts (1)
processImage(17-91)
packages/client/src/client.ts (1)
packages/common/src/index.ts (4)
AudioOptions(104-125)ImageOptions(13-65)VideoOptions(67-102)ProcessedFile(127-133)
🪛 Biome (1.9.4)
packages/server/src/audio/process-audio-handler.spec.ts
[error] 157-157: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
packages/client/src/index.ts
[error] 1-1: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
🪛 markdownlint-cli2 (0.17.2)
packages/server/README.md
19-19: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
29-29: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (41)
packages/core/src/index.ts (1)
2-5: Well-structured exports with consistent naming.The changes in exports demonstrate a well-organized approach, providing a unified API surface with consistent function naming patterns. The transition from "optimizer" to "processor" terminology and the addition of audio processing capabilities align well with the PR objectives.
packages/client/src/index.ts (1)
2-2: API surface is consistent with architectural changes.The export pattern properly exposes the newly added audio processing functionality and maintains the consistent naming pattern across media types.
packages/server/src/audio/process-audio-handler.spec.ts (1)
8-29: Comprehensive test coverage for audio processing.The test suite thoroughly validates the basic functionality of audio processing with proper status code checking and result validation.
packages/server/README.md (1)
15-35: Well-documented new audio processing endpoint.The documentation for the new audio processing endpoint follows the same structure as the existing endpoints, providing clear information about request and response formats.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
19-19: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
29-29: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
packages/core/README.md (4)
6-7: Well-defined API unification statement.The clear statement about API unification for audio, images, and videos sets the stage for the documentation that follows and aligns with the PR objective of unifying the API.
10-21: Good addition of audio processing documentation.The new section for audio processing includes all necessary information - import statements, input type explanation, and usage pattern. This is consistent with the PR objective of adding a separate API for processing audio.
22-23: Appropriate terminology update for image processing.The section has been appropriately renamed from "Converting images" to "Processing images" and the code example has been updated to use the new
processImagefunction andProcessedFiletype, maintaining consistency across the API.Also applies to: 25-32
34-44: Consistent documentation for video processing.The video processing section follows the same pattern as the audio and image sections, creating a consistent experience for developers using the API.
README.md (4)
9-15: Clear server deployment instructions with updated Docker image.The section has been renamed to "Running the server" and includes the updated Docker image version, providing clear instructions for deploying the server as a container.
17-29: Helpful local installation instructions.Added instructions for installing and running the server locally provide flexibility for users who prefer not to use Docker. The note about Bun runtime requirement is important information for users.
31-37: Good module import example for server embedding.The added example shows how to import and embed the server in a custom project, giving developers additional flexibility for integration.
39-59: Comprehensive client usage example.The new section provides a clear example of how to use the client package to connect to the server and process a video file. The example demonstrates proper handling of both processed files and status updates.
packages/client/README.md (3)
8-25: Well-documented audio processing functionality.The new section for audio processing includes a complete example with proper import statements, input type explanation, and comprehensive output handling including progress reporting and error handling.
27-44: Consistent image processing documentation.The image processing section maintains the same structure as the audio section, ensuring consistency across the API documentation. The example includes all necessary error and progress handling patterns.
46-63: Complete video processing documentation.The video processing section completes the documentation set, providing developers with examples for all three media types. The structure is consistent with the other sections, making the API intuitive to use.
test-client.ts (4)
1-6: Updated imports reflect new API structure.The imports have been updated to use the new
ProcessedFiletype and processing functions, aligning with the unified API approach.
8-24: Comprehensive audio processing example.The new audio processing example demonstrates how to use the
processAudiofunction with appropriate options and how to properly handle the returnedProcessedFile. This implementation matches the documentation and core implementation seen in the relevant code snippets.
26-30: Consistent video processing implementation.The video processing example follows the same pattern as the audio processing, using the unified API approach. The options now include an output path parameter which aligns with the API changes.
Also applies to: 31-31
44-48: Standardized image processing implementation.The image processing implementation maintains consistency with the audio and video examples, using the same pattern for function calls and output handling, which reinforces the unified API approach.
Also applies to: 49-49
packages/core/src/util/ffmpeg-processor.ts (9)
1-1: Import changes reflect the new naming conventionThe import has been updated from
ConvertedFiletoProcessedFile, which aligns with the broader renaming effort across the codebase to standardize terminology.
4-8: New imports support the enhanced input handlingThese additional imports enable the new functionality for handling various input types (file paths, streams, blobs). The
createWriteStreamandrmfunctions support temporary file operations, whileTasqueprovides queue management.
10-25: Function signature rework improves API flexibilityThe function has been renamed from
optimizeVideotoprocessFfmpegand its signature has been enhanced to accept multiple input types and use a callback for argument building. This makes the function more versatile and reusable for different media types.
77-77: Updated to use ProcessedFile instead of ConvertedFileThis change is part of the broader refactoring to standardize on "process" terminology throughout the codebase.
80-82: Improved cleanup handling for temporary filesThe code now checks if the input path is a temporary file before attempting to remove it, which prevents accidentally deleting the original file when a file path is provided directly.
104-116: New helper function for temporary file creationThe
createTmpfunction efficiently handles conversion of both stream and blob inputs to a temporary file. The implementation correctly handles the different input types and returns a promise that resolves to the file path.
112-112: Stream extraction handles both Blob and AsyncIterable inputsThe ternary expression correctly handles different input types by checking if the input is a Blob (which has a
streammethod) or directly using the AsyncIterable.
118-120: Secure random ID generationThe
randomIdfunction uses cryptographically secure random values, which is appropriate for generating unique file identifiers. The implementation uses Base64URL encoding which avoids characters that could cause issues in file paths.
26-31:❓ Verification inconclusive
Dynamic input and output path handling
The implementation now creates a unique ID for each processing job and handles different input types appropriately. This makes the function more versatile but could potentially introduce race conditions if multiple processes use the same temporary directory.
🏁 Script executed:
#!/bin/bash # Verify if there's any potential race condition handling in the tmpDir creation or cleanup rg -A 5 $'tmpDir.*mkdir'Length of output: 25
Attention: Revisit Concurrency Controls for
tmpDirThe dynamic handling using a unique ID for each processing job is a solid improvement. However, our verification did not uncover any explicit mechanism (such as atomic directory creation via
mkdiror file locking) to manage concurrent access to the shared temporary directory (tmpDir). This raises a potential risk of race conditions when multiple processes operate in parallel using the same directory.Action Items:
- Confirm that the shared usage of
tmpDiris either by design or that additional safeguards (e.g., locking mechanisms or unique subdirectory creation per job) are implemented elsewhere.- Verify that the helper function (e.g.,
createTmp) safely creates and cleans up temporary files in concurrent environments.Please review these aspects to ensure robust concurrency handling.
packages/client/src/client.spec.ts (6)
1-1: Updated import reflects naming convention changeThe import has been updated from
ConvertedFiletoProcessedFile, aligning with the broader renaming effort across the codebase.
4-4: Updated imports include new processing functionsThe import statement now includes
processAudio,processImage, andprocessVideo, reflecting the renamed and expanded API.
6-8: Mock fetch implementation updatedThe mock fetch implementation has been updated with improved syntax, using arrow functions and proper typecasting for the server options.
10-32: Added comprehensive tests for audio processingA new test case for
processAudio()verifies that the function correctly processes audio files and returns instances ofProcessedFilewith the expected properties.
34-43: Updated image processing testsThe test for image processing has been updated to use
processImageinstead ofoptimizeImageandProcessedFileinstead ofConvertedFile. The functionality remains consistent.
60-82: Added comprehensive tests for video processingA new test case for
processVideo()verifies that the function correctly processes video files and returns instances ofProcessedFilewith the expected properties. Note that the size check usesexpect.any(Number)since video sizes may vary across ffmpeg versions or platforms.packages/client/src/client.ts (7)
1-9: Updated imports reflect API changesThe imports have been updated to use
ProcessedFileinstead ofConvertedFileand include the new option types (AudioOptions,ImageOptions,VideoOptions). This aligns with the broader renaming effort across the codebase.
18-35: New audio processing function addedA new
processAudiofunction has been added with comprehensive JSDoc comments. The function accepts a host URL, input (as AsyncIterable or Blob), and options for audio processing. The implementation delegates to theoptimizeFetchhelper function.
37-54: Image processing function renamed and updatedThe
optimizeImagefunction has been renamed toprocessImagewith updated JSDoc comments and parameter types. The function now accepts AsyncIterable or Blob as input, aligning with the other processing functions.
56-73: Video processing function renamed and updatedThe
optimizeVideofunction has been renamed toprocessVideowith updated JSDoc comments and parameter types. The function now accepts AsyncIterable or Blob as input, aligning with the other processing functions.
80-86: Updated request creationThe request creation has been updated to use the new input handling function. The implementation correctly uses
inputToStreamto handle different input types.
96-103: Updated to use ProcessedFile instead of ConvertedFileThis change is part of the broader refactoring to standardize on "process" terminology throughout the codebase. The implementation now yields instances of
ProcessedFileinstead ofConvertedFile.
107-111: Input handling helper function updatedThe
inputToStreamfunction has been updated to handle AsyncIterable or Blob inputs. The function correctly checks if the input is a ReadableStream or if it doesn't implement the AsyncIterable protocol.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/server/src/videos/process-video-handler.ts (1)
30-42: 🛠️ Refactor suggestionAdd null check for request body
The handler passes
request.body!directly toprocessVideowithout checking if it's null or undefined. This could lead to runtime errors if the request doesn't have a body.- const iterable = processVideo(request.body!, opts, { + if (!request.body) return error(400, "Request body is required"); + const iterable = processVideo(request.body, opts, { signal: request.signal, });
🧹 Nitpick comments (9)
packages/core/src/util/streams.ts (1)
3-13: Add error handling to ensure proper stream cleanupThe function correctly handles backpressure and ensures proper stream termination, but lacks error handling. If an exception occurs during iteration, the writable stream might not be properly closed.
Consider adding a try/finally block to ensure the writable stream is properly closed even in case of errors:
export async function exhaustAsyncIterableToWritable( it: AsyncIterable<Uint8Array>, writable: Writable ) { - for await (const chunk of it) { - if (!writable.write(chunk)) - await new Promise((resolve) => writable.once("drain", resolve)); - } - writable.end(); - await new Promise((resolve) => writable.on("finish", resolve)); + try { + for await (const chunk of it) { + if (!writable.write(chunk)) + await new Promise((resolve) => writable.once("drain", resolve)); + } + writable.end(); + await new Promise((resolve) => writable.on("finish", resolve)); + } catch (error) { + writable.end(); + throw error; + } }packages/server/src/util/response.ts (2)
18-20: Improve type safety in the parameter definitionThe
{}type is flagged by static analysis as it provides minimal type safety.Consider using a more specific type or
unknowninstead of{}:export async function queueAndStream( - iterable: AsyncIterable<{} | ProcessedFile | Blob> + iterable: AsyncIterable<unknown | ProcessedFile | Blob> ) {🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
30-43: Add null checks for optional propertiesThe code uses non-null assertion operators (
!) withx.nameandx.stream, but these properties could be undefined as thestreamparameter is optional in theProcessedFileconstructor.Replace non-null assertions with proper null checks:
if (x instanceof ProcessedFile) { clear(); + if (!x.name || !x.stream) { + multipart.part({ payload: { error: "Missing required properties in ProcessedFile" } }); + continue; + } multipart.part({ contentType: x.type, - filename: basename(x.name!), + filename: basename(x.name), }); - for await (const chunk of x.stream!) { + for await (const chunk of x.stream) { multipart.write(chunk); } } else {packages/server/src/audio/process-audio-handler.ts (2)
9-21: Schema definition includes unused fieldThe
optionsfield is included in the schema definition but isn't used anywhere in the handler implementation. This could be confusing to consumers of the API.- options: T.Optional(T.String()),
40-58: Consistent error handling for ProcessedFile operationsThe file handling operations (writing to output, reading streams) aren't wrapped in try/catch blocks. Consider adding proper error handling to ensure robustness.
if (opts.output) { - await Bun.write(Bun.file(opts.output), Bun.file(value.name)); - await rm(value.name, { force: true }); + try { + await Bun.write(Bun.file(opts.output), Bun.file(value.name)); + await rm(value.name, { force: true }); + } catch (err) { + return error(500, `Failed to write to output: ${(err as Error).message}`); + }test-client.ts (2)
1-24: Extract common file handling logicAll three processing examples use identical logic for handling the processed file streams. Consider extracting this into a reusable function to reduce code duplication.
+async function handleProcessedFile(value: ProcessedFile) { + const file = Bun.file(value.name); + const writer = file.writer(); + for await (const chunk of value.stream) { + writer.write(chunk); + await writer.flush(); + } + await writer.end(); +} for await (const value of processAudio( "http://localhost:3000", Bun.file("fixtures/audio.mp3"), { format: "ogg", codec: "libvorbis", output: "output/audio.ogg" } )) { if (value instanceof ProcessedFile) { - const file = Bun.file(value.name); - const writer = file.writer(); - for await (const chunk of value.stream) { - writer.write(chunk); - await writer.flush(); - } - await writer.end(); + await handleProcessedFile(value); } else { console.log(value); } }
22-23: Improve non-file result handlingThe else block only logs the value without differentiating between different types (progress, queue position, error). Consider adding more specific handling for each type.
- } else { - console.log(value); + } else if ("progress" in value) { + console.log(`Progress: ${value.progress}%`); + } else if ("position" in value) { + console.log(`Queue position: ${value.position}`); + } else if ("error" in value) { + console.error(`Error: ${value.error}`); + } else { + console.log("Unknown value type:", value); }packages/server/src/videos/process-video-handler.ts (2)
49-52: Add error handling for file operationsThe file operations (writing to output, removing temporary files) aren't wrapped in try/catch blocks. Consider adding proper error handling to ensure robustness.
if (opts.output) { - await Bun.write(Bun.file(opts.output), Bun.file(value.name)); - await rm(value.name, { force: true }); + try { + await Bun.write(Bun.file(opts.output), Bun.file(value.name)); + await rm(value.name, { force: true }); + } catch (err) { + console.error(`Failed to write to output: ${(err as Error).message}`); + // Consider returning an error or handling this more gracefully + }
38-39: Consider supporting multiple optionsThe comment indicates that only one option is currently supported. Consider supporting multiple options to align with the array parsing approach, or document this limitation better.
Would you like me to provide an implementation that supports processing multiple options in sequence?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockfixtures/audio.mp3is excluded by!**/*.mp3
📒 Files selected for processing (30)
.gitignore(1 hunks).husky/pre-commit(1 hunks)README.md(1 hunks)docker-compose.yaml(1 hunks)package.json(1 hunks)packages/client/README.md(2 hunks)packages/client/src/client.spec.ts(2 hunks)packages/client/src/client.ts(5 hunks)packages/client/src/index.ts(1 hunks)packages/common/src/index.ts(4 hunks)packages/core/README.md(1 hunks)packages/core/src/audio/audio-processor.spec.ts(1 hunks)packages/core/src/audio/audio-processor.ts(1 hunks)packages/core/src/images/image-optimizer.ts(0 hunks)packages/core/src/images/image-processor.ts(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/util/ffmpeg-processor.ts(3 hunks)packages/core/src/util/streams.ts(1 hunks)packages/core/src/videos/video-processor.spec.ts(5 hunks)packages/core/src/videos/video-processor.ts(1 hunks)packages/server/README.md(2 hunks)packages/server/src/audio/process-audio-handler.spec.ts(1 hunks)packages/server/src/audio/process-audio-handler.ts(1 hunks)packages/server/src/images/process-image-handler.ts(6 hunks)packages/server/src/server.ts(2 hunks)packages/server/src/util/file.ts(0 hunks)packages/server/src/util/response.ts(3 hunks)packages/server/src/videos/process-video-handler.ts(3 hunks)test-client.ts(2 hunks)test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/server/src/util/file.ts
- packages/core/src/images/image-optimizer.ts
✅ Files skipped from review due to trivial changes (1)
- .husky/pre-commit
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/server/src/server.ts
- docker-compose.yaml
- packages/server/src/audio/process-audio-handler.spec.ts
- .gitignore
- test.ts
- packages/core/src/videos/video-processor.ts
- packages/core/src/videos/video-processor.spec.ts
- packages/core/src/images/image-processor.ts
- packages/core/src/audio/audio-processor.spec.ts
- packages/client/src/index.ts
- packages/core/src/index.ts
- packages/server/README.md
- packages/common/src/index.ts
- packages/core/src/audio/audio-processor.ts
🧰 Additional context used
🧬 Code Definitions (7)
packages/server/src/audio/process-audio-handler.ts (2)
packages/common/src/index.ts (2)
AudioOptions(104-125)ProcessedFile(127-133)packages/core/src/audio/audio-processor.ts (1)
processAudio(13-40)
test-client.ts (5)
packages/client/src/client.ts (3)
processAudio(25-35)processVideo(63-73)processImage(44-54)packages/core/src/audio/audio-processor.ts (1)
processAudio(13-40)packages/common/src/index.ts (1)
ProcessedFile(127-133)packages/core/src/videos/video-processor.ts (1)
processVideo(12-46)packages/core/src/images/image-processor.ts (1)
processImage(17-91)
packages/server/src/videos/process-video-handler.ts (3)
packages/common/src/index.ts (2)
VideoOptions(67-102)ProcessedFile(127-133)packages/client/src/client.ts (1)
processVideo(63-73)packages/core/src/videos/video-processor.ts (1)
processVideo(12-46)
packages/client/src/client.spec.ts (4)
packages/client/src/index.ts (5)
setFetch(2-2)processAudio(2-2)ProcessedFile(1-1)processImage(2-2)processVideo(2-2)packages/server/src/server.ts (1)
serveOpts(6-23)packages/core/src/index.ts (3)
processAudio(2-2)processImage(3-3)processVideo(5-5)packages/common/src/index.ts (1)
ProcessedFile(127-133)
packages/client/src/client.ts (1)
packages/common/src/index.ts (4)
AudioOptions(104-125)ImageOptions(13-65)VideoOptions(67-102)ProcessedFile(127-133)
packages/server/src/images/process-image-handler.ts (6)
packages/common/src/index.ts (2)
ImageOptions(13-65)ProcessedFile(127-133)packages/client/src/index.ts (2)
processImage(2-2)ProcessedFile(1-1)packages/core/src/index.ts (1)
processImage(3-3)packages/client/src/client.ts (1)
processImage(44-54)packages/core/src/images/image-processor.ts (1)
processImage(17-91)packages/server/src/util/response.ts (2)
error(5-7)queueAndStream(18-53)
packages/server/src/util/response.ts (1)
packages/common/src/index.ts (1)
ProcessedFile(127-133)
🪛 Biome (1.9.4)
packages/server/src/util/response.ts
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (39)
package.json (2)
8-8: New "check" Script Added: Verify Combined Build & Test Command
The addition of the"check": "bun run build -f && bun run test"script helps automate the build and test process. Please ensure that both commands work as expected in your environment, and consider verifying that the-fflag with the build command is required and functions correctly.
14-14: New "husky" Dev Dependency: Confirm Pre-commit Hook Integration
The introduction of"husky": "^9.1.7"is in line with the updated pre-commit configuration. Please confirm that documentation or setup instructions (such as runninghusky install) are provided so that developers correctly initialize Husky after cloning the repository.packages/core/README.md (4)
6-7: LGTM! Clear introduction to the unified API conceptThe explicit mention of a unified API for all media types sets proper expectations for users and improves documentation clarity.
10-20: LGTM! Well-structured audio processing documentationThe new audio processing section follows the same pattern as other media types, contributing to a consistent documentation experience. The examples clearly show how to use the new API.
22-32: LGTM! Updated image processing sectionThe section has been renamed from "optimizing" to "processing" for consistency with other media types and includes updated import statements and standardized input type description.
34-44: LGTM! Consistent video processing documentationThe video processing section follows the same pattern as other media types, maintaining documentation consistency across the codebase.
packages/server/src/util/response.ts (1)
1-1: LGTM! Updated import to reflect the new ProcessedFile typeImport statement correctly updated to use ProcessedFile, consistent with the codebase-wide renaming.
README.md (4)
9-15: LGTM! Clear server deployment instructionsThe updated Docker command references the correct image version and provides clear deployment instructions.
17-29: LGTM! Helpful local installation instructionsThe addition of local installation instructions makes the documentation more comprehensive, clearly noting the Bun runtime requirement.
31-37: LGTM! Useful server embedding instructionsThe example for embedding the server as a module is a valuable addition for developers who want to integrate the server into their own applications.
39-59: LGTM! Comprehensive client usage exampleThe client usage example clearly demonstrates how to:
- Connect to the server
- Process a video file
- Handle both processed file results and status updates
packages/server/src/audio/process-audio-handler.ts (1)
33-34: Consider supporting multiple optionsThe comment indicates that only one option is currently supported. Consider supporting multiple options to align with the array parsing approach, or document this limitation better.
Is this a temporary limitation or a design decision? If it's temporary, you might want to track it with a TODO comment.
packages/client/src/client.spec.ts (5)
1-4: Updated imports reflect the API changesThe imports have been correctly updated to use
ProcessedFilefrom the common package and to import the new functionsprocessAudio,processImage, andprocessVideo.
6-8: Good test setup with mock fetch implementationThe setup correctly configures the tests to route requests through the server's fetch handler directly, enabling proper integration testing without network calls.
10-32: New test suite for audio processing functionalityThe new test case for
processAudio()correctly verifies that:
- The function can process an audio file
- The server returns a properly formatted
ProcessedFileinstance- The processed file has the expected name, MIME type, and size
This aligns with the PR objective of adding dedicated audio processing capabilities.
34-58: Updated image processing test suiteThe test for
processImage()has been correctly updated to:
- Use the new function name (
processImageinstead ofoptimizeImage)- Check for
ProcessedFileinstances instead ofConvertedFile- Maintain the same verification logic for the processed output
This ensures backward compatibility while implementing the unified API approach.
60-82: New test suite for video processingThe test case for
processVideo()follows the same pattern as the audio and image tests, providing consistent coverage for all media types. The use ofexpect.any(Number)for the size is appropriate since video processing may result in variable file sizes.packages/core/src/util/ffmpeg-processor.ts (7)
1-8: Updated imports align with the new processing modelThe import statements have been correctly updated to include
ProcessedFilefrom the common package and add necessary utilities for working with file streams.
10-25: Enhanced function signature with improved flexibilityThe function has been renamed from
optimizeVideotoprocessFfmpegwith a more flexible signature that:
- Accepts multiple input types (string path, AsyncIterable, or Blob)
- Uses a callback for dynamic FFmpeg argument generation
- Supports queue management and abort signaling
This significantly improves the reusability of the function for different media types.
26-32: Improved input handling with temporary file creationThe code now handles different input types intelligently:
- Uses direct file paths when provided
- Creates temporary files for streams or blobs
- Generates unique IDs for temporary files
This implementation supports the PR objective of unifying the API to handle both file paths and binary data.
77-77: Updated to use ProcessedFile classThe function now correctly yields a
ProcessedFileinstance instead ofConvertedFile, maintaining consistency with the unified API model.
80-82: Improved cleanup for temporary filesThe cleanup logic has been enhanced to handle temporary input files created from streams or blobs, preventing file system leaks.
104-116: Well-implemented temporary file creationThe
createTmphelper function effectively:
- Creates a temporary file with a unique name
- Handles both AsyncIterable and Blob inputs
- Uses the efficient
exhaustAsyncIterableToWritableutilityThis implementation supports processing streams and blobs seamlessly.
118-120: Secure random ID generationThe
randomIdfunction uses cryptographically secure random values to generate unique identifiers, which is appropriate for temporary file names.packages/server/src/images/process-image-handler.ts (8)
1-1: Updated type import aligns with naming convention changesThe import now correctly uses
ImageOptionsinstead ofImageOptimizerOptions, maintaining consistency with the type system changes.
4-4: Updated imports for the new processing modelThe import now correctly includes
ProcessedFileandprocessImagefrom the m4k package, reflecting the API changes.
80-80: Type declaration updated to use the new options typeThe variable declaration now correctly uses
ImageOptions[]instead ofImageOptimizerOptions[], maintaining type consistency.
89-91: Updated function call with improved parameter passingThe function call has been updated to:
- Use
processImageinstead ofoptimizeImage- Pass the request body directly, which could be a stream or a blob
- Pass the request's abort signal for cancellation support
This implements the unified API approach for image processing.
97-97: Updated type check for processed filesThe type check now correctly looks for
ProcessedFileinstances instead ofConvertedFile, maintaining consistency with the new processing model.
109-109: Updated constructor call for processed filesThe code now correctly creates a new
ProcessedFileinstance instead ofConvertedFile, maintaining consistency with the new processing model.
131-131: Updated return type annotationThe function's return type has been correctly updated to
ImageOptionsinstead ofImageOptimizerOptions, maintaining type consistency.
143-149: Consistent object property formattingThe crop object properties are now consistently formatted, improving code readability.
packages/client/src/client.ts (7)
1-9: Updated imports align with the new type systemThe imports have been correctly updated to:
- Use
ProcessedFileinstead ofConvertedFile- Import the new types
AudioOptions,ImageOptions, andVideoOptions- Maintain necessary types for progress tracking and error handling
This supports the unified API approach.
18-35: Well-implemented audio processing functionThe new
processAudiofunction:
- Has comprehensive JSDoc comments
- Follows the same pattern as other processing functions
- Correctly handles input streams or blobs
- Leverages the common
optimizeFetchhelper functionThis satisfies the PR objective of adding a separate API for audio processing.
37-54: Renamed and updated image processing functionThe function has been renamed from
optimizeImagetoprocessImageand includes:
- Detailed JSDoc comments
- Simplified input types (AsyncIterable or Blob)
- Updated options type (ImageOptions)
This maintains consistency with the unified API approach.
56-73: Renamed and updated video processing functionThe function has been renamed from
optimizeVideotoprocessVideoand includes:
- Detailed JSDoc comments
- Simplified input types (AsyncIterable or Blob)
- Updated options type (VideoOptions)
This maintains consistency with the unified API approach.
75-85: Enhanced request handling in optimizeFetchThe helper function now correctly:
- Accepts simplified input types
- Converts inputs to streams when needed
- Properly constructs the request with options in headers
This implementation supports the unified API approach for all media types.
96-102: Updated to yield ProcessedFile instancesThe function now correctly yields
ProcessedFileinstances instead ofConvertedFile, maintaining consistency with the unified API model.
107-111: Efficient input conversion utilityThe
inputToStreamfunction intelligently handles different input types:
- Passes ReadableStream instances through directly
- Passes Blob instances through directly
- Converts AsyncIterable instances to streams
This enables the API to work with a variety of input types.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/server/src/audio/process-audio-handler.ts (1)
44-54: Potential temporary file cleanup issue.When no output path is specified, the temporary file created by
processAudioisn't explicitly cleaned up. This could lead to accumulation of temporary files over time.Consider adding cleanup for temporary files even when no output is specified:
if (opts.output) { await Bun.write(Bun.file(opts.output), Bun.file(value.name)); await rm(value.name, { force: true }); } else { yield new ProcessedFile( opts.name ?? basename(value.name), value.type, value.stream ?? Bun.file(value.name).stream() ); + // Clean up the temporary file after streaming + await rm(value.name, { force: true }); }test-client.ts (1)
8-60: Consider reducing code duplication across processing functionsThere's significant code duplication between the audio, video, and image processing sections. Each block uses nearly identical patterns for handling the processed files.
Consider creating a helper function to reduce duplication:
+async function processMediaFile(generator, outputPath) { + for await (const value of generator) { + if (value instanceof ProcessedFile) { + const file = Bun.file(value.name); + const writer = file.writer(); + for await (const chunk of value.stream) { + writer.write(chunk); + await writer.flush(); + } + await writer.end(); + } else { + console.log(value); + } + } +} + // Then use it like: +await processMediaFile( + processAudio( + "http://localhost:3000", + Bun.file("fixtures/audio.mp3"), + { format: "ogg", codec: "libvorbis", output: "output/audio.ogg" } + ) +);packages/server/src/videos/process-video-handler.ts (1)
49-53: Potential lack of feedback when using output optionWhen
opts.outputis provided, the function writes to that file but doesn't yield any value to the caller, which means they receive no feedback about the processing.Consider yielding a success message or status object when using the output option:
if (opts.output) { await Bun.write(Bun.file(opts.output), Bun.file(value.name)); await rm(value.name, { force: true }); + yield { success: true, output: opts.output }; } else {packages/core/src/util/ffmpeg-processor.ts (1)
17-92: Consider enhancing error handlingWhile the function handles basic errors, it could be improved for more robust error reporting, especially for FFmpeg process errors.
Consider enhancing error handling with more detailed information:
if (code !== 0) { - throw new Error(`ffmpeg exited with code ${code}: ${errStr}`); + const errorInfo = { + code, + message: errStr, + command: ["-y", ...inArgs, "-i", "(input)", ...outArgs, outputPath].join(" "), + format + }; + throw new Error(`FFmpeg processing failed: ${JSON.stringify(errorInfo)}`); }packages/client/src/client.spec.ts (1)
10-80: Consider reducing test code duplicationThere's significant code duplication between the three test functions. Each follows the same pattern for testing the respective processing functions.
Consider creating a helper function to reduce duplication:
+async function testProcessFunction( + processFn, + fileUrl, + options, + expectedResults +) { + const file = Bun.file(fileUrl); + const files: { name: string; type: string; size: number }[] = []; + + for await (const x of processFn("http://localhost:3000", file, options)) { + if (x instanceof ProcessedFile) { + const chunks = await Array.fromAsync(x.stream); + files.push({ + name: x.name, + type: x.type, + size: chunks.reduce((sum, chunk) => sum + chunk.length, 0), + }); + } + } + + expect(files).toEqual(expectedResults); +} + +// Example usage: +it("should query the server to process an audio file", async () => { + await testProcessFunction( + processAudio, + "../../fixtures/audio.mp3", + [{ format: "mp3", name: "audio.mp3" }], + [{ name: "audio.mp3", type: "audio/mpeg", size: 52288 }] + ); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockfixtures/audio.mp3is excluded by!**/*.mp3
📒 Files selected for processing (23)
.gitignore(1 hunks).husky/pre-commit(1 hunks)README.md(1 hunks)package.json(1 hunks)packages/client/README.md(2 hunks)packages/client/src/client.spec.ts(2 hunks)packages/client/src/client.ts(5 hunks)packages/client/src/index.ts(1 hunks)packages/common/src/index.ts(4 hunks)packages/core/README.md(1 hunks)packages/core/src/audio/audio-processor.spec.ts(1 hunks)packages/core/src/audio/audio-processor.ts(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/util/ffmpeg-processor.ts(3 hunks)packages/core/src/videos/video-processor.spec.ts(5 hunks)packages/core/src/videos/video-processor.ts(1 hunks)packages/server/README.md(2 hunks)packages/server/src/audio/process-audio-handler.spec.ts(1 hunks)packages/server/src/audio/process-audio-handler.ts(1 hunks)packages/server/src/images/process-image-handler.ts(5 hunks)packages/server/src/server.ts(2 hunks)packages/server/src/videos/process-video-handler.ts(3 hunks)test-client.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- .husky/pre-commit
- .gitignore
- packages/server/src/server.ts
- packages/client/src/index.ts
- packages/server/src/audio/process-audio-handler.spec.ts
- packages/server/README.md
- packages/core/src/audio/audio-processor.spec.ts
- package.json
- packages/core/src/index.ts
- packages/core/src/videos/video-processor.ts
- packages/core/src/audio/audio-processor.ts
- packages/common/src/index.ts
- packages/core/src/videos/video-processor.spec.ts
🧰 Additional context used
🧬 Code Definitions (5)
packages/server/src/audio/process-audio-handler.ts (2)
packages/common/src/index.ts (2)
AudioOptions(104-125)ProcessedFile(127-133)packages/core/src/audio/audio-processor.ts (1)
processAudio(13-40)
packages/server/src/videos/process-video-handler.ts (3)
packages/common/src/index.ts (2)
VideoOptions(67-102)ProcessedFile(127-133)packages/client/src/client.ts (1)
processVideo(63-73)packages/core/src/videos/video-processor.ts (1)
processVideo(12-46)
packages/client/src/client.spec.ts (5)
packages/client/src/index.ts (5)
setFetch(2-2)processAudio(2-2)ProcessedFile(1-1)processImage(2-2)processVideo(2-2)packages/core/src/audio/audio-processor.ts (1)
processAudio(13-40)packages/common/src/index.ts (1)
ProcessedFile(127-133)packages/core/src/images/image-processor.ts (1)
processImage(17-91)packages/core/src/videos/video-processor.ts (1)
processVideo(12-46)
packages/server/src/images/process-image-handler.ts (6)
packages/server/src/util/response.ts (2)
error(5-7)queueAndStream(18-53)packages/common/src/index.ts (2)
ImageOptions(13-65)ProcessedFile(127-133)packages/client/src/index.ts (2)
processImage(2-2)ProcessedFile(1-1)packages/client/src/client.ts (1)
processImage(44-54)packages/core/src/index.ts (1)
processImage(3-3)packages/core/src/images/image-processor.ts (1)
processImage(17-91)
packages/client/src/client.ts (1)
packages/common/src/index.ts (4)
AudioOptions(104-125)ImageOptions(13-65)VideoOptions(67-102)ProcessedFile(127-133)
🔇 Additional comments (43)
packages/core/README.md (3)
6-7: Well-designed API unification.Unifying the API across audio, images, and videos creates a more consistent developer experience. This approach follows the principle of least surprise by providing a standardized interface for similar operations.
10-21: Good addition of audio processing documentation.The new audio processing section maintains consistency with the image and video processing documentation. This ensures that developers have a clear understanding of how to process all supported media types.
22-44: Consistent naming convention applied correctly.Renaming from
optimize*toprocess*functions creates a more accurate description of the functionality while maintaining consistency across all media types. The updated examples properly reflect these changes.README.md (2)
9-37: Comprehensive server deployment instructions.The detailed instructions for running the server as a Docker container or as a local package provide clear guidance for users. Including the note about Bun runtime requirements is particularly helpful.
39-59: Clear client usage example.The client usage example effectively demonstrates how to process a video file using the API, including proper handling of both processed files and status updates. This aligns well with the unified API approach.
packages/client/README.md (4)
4-4: Improved clarity on progress reporting mechanism.Explicitly mentioning that progress reporting is via
AsyncIterables provides developers with a clear understanding of how to consume progress information.
8-25: Consistent API naming for audio processing.The updated section for audio processing maintains consistency with the other media types and properly reflects the renamed function from
optimizeAudiotoprocessAudio.
27-44: Consistent API naming for image processing.The renamed section for image processing aligns with the other media types and properly reflects the renamed function from
optimizeImagetoprocessImage.
46-63: Added video processing documentation.The new video processing section completes the documentation for all media types, providing consistency in the API documentation.
packages/server/src/audio/process-audio-handler.ts (3)
9-21: Well-structured options schema.The schema correctly defines all the potential options for audio processing, using appropriate type definitions for each field. The use of TypeBox for schema validation is a good practice for ensuring type safety at runtime.
25-39: Request validation and error handling is robust.The handler correctly validates the request body and options, providing appropriate error responses for invalid inputs. The explicit comment about supporting only one option set helps future maintainers understand the current limitation.
35-38: Add null check for request body before passing to processAudio.While there's a check at the beginning of the handler, passing
request.bodydirectly toprocessAudiowithout a type assertion or additional check could lead to type errors.test-client.ts (4)
1-6: Updated imports align with the new unified APIThe imports have been correctly updated to reflect the new API naming convention, replacing
ConvertedFilewithProcessedFileand using the newprocess*functions instead ofoptimize*functions.
8-24: New audio processing functionality works correctlyThe new audio processing code correctly implements the unified API approach, handling the input file and processing options appropriately.
26-30: Video processing now includes output path parameterThe updated video processing code correctly includes the output path parameter in the options, which aligns with the unified API approach.
44-48: Image processing now includes output path parameterThe updated image processing code correctly includes the output path parameter in the options, consistent with the other processing functions.
packages/server/src/videos/process-video-handler.ts (4)
3-4: Updated imports align with the unified APIThe imports have been correctly updated to reflect the new API naming convention, replacing
ConvertedFilewithProcessedFileand usingprocessVideoinstead ofoptimizeVideo.
31-33: Added proper request body validationGood addition of a check to ensure the request body is provided, returning an appropriate error response if missing.
41-43: Simplified video processing with unified APIThe code now directly calls
processVideowith the request body and options, which is cleaner than the previous approach of manually handling temporary files.
48-59: Improved output file handling logicThe updated code properly handles the output file path if provided, or yields a new
ProcessedFilewith the appropriate name and type.packages/core/src/util/ffmpeg-processor.ts (8)
1-9: Updated imports align with the unified APIThe imports have been correctly updated to include
ProcessedFileinstead ofConvertedFileand add necessary imports for the new functionality.
10-16: Improved function documentationThe JSDoc comment for the
processFfmpegfunction clearly explains its purpose and parameters, including the new flexibility to accept different input types.
17-25: Enhanced function signature with better flexibilityThe function signature has been significantly improved to:
- Accept multiple input types (file path, stream, or blob)
- Use a callback for dynamic argument generation
- Accept necessary context parameters like signal, queue, and tmpDir
This design provides much better flexibility and reusability.
26-29: Smart input handling with type checkingThe code efficiently determines how to handle the input based on its type, using a string path directly or creating a temporary file for streams/blobs.
77-77: Updated to use ProcessedFileCorrectly updated to use
ProcessedFileinstead ofConvertedFile, maintaining consistency with the rest of the codebase.
79-82: Improved cleanup with type checkingThe cleanup code now properly checks if the input path is not a string before attempting to remove it, preventing errors when the input is a file path string.
104-116: Well-designed helper function for temporary file creationThe
createTmpfunction is a clean, reusable helper that handles creating a temporary file from an input stream or blob.
118-120: Secure random ID generationThe
randomIdfunction uses cryptographically secure random values, which is appropriate for generating unique file identifiers.packages/client/src/client.spec.ts (4)
1-8: Updated imports and setup for the unified APIThe imports and setup have been correctly updated to reflect the new API naming convention and functions.
10-32: New comprehensive test for audio processingThe new test for
processAudiois well-structured and verifies that the function correctly processes audio files and returns the expected output.
34-58: Updated image processing testThe test for
processImagehas been properly updated to use the new function name and verify the correct processing of image files.
60-82: New comprehensive test for video processingThe new test for
processVideocorrectly verifies the function's ability to process video files and return the expected output.Note that using
expect.any(Number)for the size is appropriate since video processing may result in different file sizes depending on various factors.packages/server/src/images/process-image-handler.ts (5)
1-1: Import statements updated to reflect the unified API approach.The imports have been appropriately updated to use the new type names (
ImageOptionsinstead of what was likelyImageOptimizerOptions) and the renamed processing function (processImageinstead ofoptimizeImage), which aligns with the PR objective of unifying the API.Also applies to: 4-4
80-81: Good defensive coding with request body validation.Adding an explicit check for the request body before proceeding with processing is a good practice for robust error handling. The type declaration for
optsArrhas also been properly updated to match the new type name.
89-91: Function call updated to use abort signal for cancellation support.The function call has been updated to use the new
processImagefunction and now includes the request's abort signal. This is a good improvement as it allows for proper cancellation of in-progress operations when the client disconnects.
97-97: Instance checks and yield statements updated to use ProcessedFile.The code has been properly updated to check for instances of
ProcessedFileinstead ofConvertedFile, maintaining consistency with the type changes across the codebase.Also applies to: 109-109
131-131: Return type and crop object construction updated.The return type annotation has been updated to match the new
ImageOptionstype. The crop object creation has been maintained with the same structure, ensuring that optional properties are properly handled.Also applies to: 143-149
packages/client/src/client.ts (6)
2-9: Import statements updated to reflect the unified API approach.The imports have been properly updated to use the new
ProcessedFileclass and type definitions for the different media options (AudioOptions,ImageOptions,VideoOptions), which aligns with the PR objective of unifying the API.
18-35: New processAudio function added with proper documentation.A new function has been added to process audio files, following the same pattern as the existing functions for images and videos. The well-written JSDoc comments clearly explain the purpose, parameters, and return values, enhancing the API's usability.
37-54: Image processing function renamed and documented with JSDoc.The function has been renamed from
optimizeImagetoprocessImagefor consistency with the new unified API approach. Comprehensive JSDoc comments have been added to improve developer experience. The function signature has been updated to accept a more specific input type (AsyncIterable<Uint8Array> | Blob).
56-73: Video processing function renamed and documented with JSDoc.The function has been renamed from
optimizeVideotoprocessVideofor consistency with the new unified API approach. Comprehensive JSDoc comments have been added to improve developer experience. The function signature has been updated to accept a more specific input type (AsyncIterable<Uint8Array> | Blob).
75-105: Fetch implementation updated with modern Request API.The fetch implementation has been updated to use the modern Request constructor pattern and properly yield instances of
ProcessedFileinstead ofConvertedFile. This change maintains consistency with the type changes across the codebase.
107-111: Helper function renamed for clarity.The function has been renamed from what was likely
bodyToStreamtoinputToStream, which better reflects its purpose of converting various input types to a stream.
This leads to inconsistent test behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/client/src/client.ts (1)
107-111: Potential type checking inconsistency in inputToStream function.The function still checks if the input is an instance of
ReadableStream, butReadableStreamis no longer part of the parameter type. While this doesn't cause issues with the current implementation, it could be simplified.-function inputToStream(input: AsyncIterable<Uint8Array> | Blob) { - return input instanceof ReadableStream || !(Symbol.asyncIterator in input) - ? input - : iterableToStream(input); +function inputToStream(input: AsyncIterable<Uint8Array> | Blob) { + return !(Symbol.asyncIterator in input) + ? input + : iterableToStream(input); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockfixtures/audio.mp3is excluded by!**/*.mp3
📒 Files selected for processing (23)
.gitignore(1 hunks).husky/pre-commit(1 hunks)README.md(1 hunks)package.json(1 hunks)packages/client/README.md(2 hunks)packages/client/src/client.spec.ts(2 hunks)packages/client/src/client.ts(5 hunks)packages/client/src/index.ts(1 hunks)packages/common/src/index.ts(4 hunks)packages/core/README.md(1 hunks)packages/core/src/audio/audio-processor.spec.ts(1 hunks)packages/core/src/audio/audio-processor.ts(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/util/ffmpeg-processor.ts(3 hunks)packages/core/src/videos/video-processor.spec.ts(5 hunks)packages/core/src/videos/video-processor.ts(1 hunks)packages/server/README.md(2 hunks)packages/server/src/audio/process-audio-handler.spec.ts(1 hunks)packages/server/src/audio/process-audio-handler.ts(1 hunks)packages/server/src/images/process-image-handler.ts(5 hunks)packages/server/src/server.ts(2 hunks)packages/server/src/videos/process-video-handler.ts(3 hunks)test-client.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- .husky/pre-commit
- packages/server/src/server.ts
- .gitignore
- package.json
- packages/core/src/videos/video-processor.ts
- packages/client/src/index.ts
- packages/server/src/audio/process-audio-handler.spec.ts
- packages/core/src/videos/video-processor.spec.ts
- packages/server/src/audio/process-audio-handler.ts
- packages/core/src/audio/audio-processor.spec.ts
- packages/core/src/audio/audio-processor.ts
- packages/common/src/index.ts
- packages/server/src/images/process-image-handler.ts
🧰 Additional context used
🧬 Code Definitions (3)
packages/client/src/client.spec.ts (3)
packages/client/src/index.ts (4)
processAudio(2-2)ProcessedFile(1-1)processImage(2-2)processVideo(2-2)packages/core/src/index.ts (3)
processAudio(2-2)processImage(3-3)processVideo(5-5)packages/common/src/index.ts (1)
ProcessedFile(127-133)
test-client.ts (5)
packages/client/src/client.ts (3)
processAudio(25-35)processVideo(63-73)processImage(44-54)packages/core/src/audio/audio-processor.ts (1)
processAudio(13-40)packages/common/src/index.ts (1)
ProcessedFile(127-133)packages/core/src/videos/video-processor.ts (1)
processVideo(12-46)packages/core/src/images/image-processor.ts (1)
processImage(17-91)
packages/client/src/client.ts (1)
packages/common/src/index.ts (4)
AudioOptions(104-125)ImageOptions(13-65)VideoOptions(67-102)ProcessedFile(127-133)
🔇 Additional comments (41)
packages/core/src/index.ts (1)
2-5: Excellent API unification with consistent naming patternsThe export updates successfully implement the unified API approach mentioned in the PR objectives. The naming convention has been standardized from "optimize" to "process" across all media types (audio, image, video), which creates a more consistent and intuitive API.
The addition of audio processing functionality aligns with the PR's goal of adding a separate API for audio processing.
packages/core/README.md (4)
6-7: Good introduction to the unified API conceptThis addition clearly communicates the key concept of API unification for all media types, which matches the PR objectives.
10-21: Well-structured documentation for the new audio processing featureThe documentation for audio processing follows the same pattern as the existing media types, making it easy for users to understand. The comment about input types clearly explains the flexibility of the API to accept different input formats.
22-32: Consistent documentation across media typesThe image processing section has been updated to maintain consistency with the other sections. The renaming from "optimizeImage" to "processImage" and standardization of input types helps users understand the unified API approach.
34-44: Consistent documentation for video processingThe video processing section has been similarly updated for API consistency, following the same pattern as the other media types. This completes the unified API documentation.
packages/server/README.md (3)
15-36: Well-structured API documentation for the new audio processing endpointThe new audio processing endpoint documentation is well-structured and follows the same format as the existing endpoints. The request and response sections are clear and provide all necessary information for consumers of the API.
37-57: Improved formatting for image processing endpoint documentationThe image processing endpoint documentation has been reformatted to match the new structure, enhancing overall consistency in the API documentation.
59-79: Consistent formatting for video processing endpointThe video endpoint documentation has been updated to maintain consistency with the other endpoints, making the entire API documentation more cohesive and easier to follow.
README.md (4)
9-15: Improved server deployment documentationThe reorganization from "Usage" to "Running the server" with specific package references improves clarity. The updated Docker image version ensures users have access to the latest functionality including the new audio processing capabilities.
17-29: Valuable alternative installation and usage instructionsAdding documentation for local package installation provides flexibility for users who prefer not to use Docker. The clear note about Bun runtime requirements helps avoid potential confusion or deployment issues.
31-37: Useful embedded server implementation exampleThis addition provides a more advanced use case for developers who want to integrate the server into their own projects rather than running it standalone.
39-59: Clear client usage example with the unified APIThe client usage section effectively demonstrates how to use the new unified API, showing the consistent method signature pattern. The example provides a complete workflow including handling both processed files and status updates.
packages/client/README.md (2)
13-14: Input type documentation inconsistency.The comment states input is "an AsyncIterable or Blob", but according to the core implementation (
processAudioin core/src/audio/audio-processor.ts), the input can also be a file path string. This inconsistency exists for all processing functions in this README.To verify this inconsistency, let's review the function signatures:
#!/bin/bash # Check the input types for process functions in core package rg -A 1 "export function process(Audio|Image|Video)" packages/core/src/
51-52: Inconsistent input type documentation.For
processVideo, the comment states that input is "an AsyncIterable or Blob", but according to the core implementation, the input can also be a file path string.test-client.ts (4)
1-6: LGTM! Package imports updated correctly.Imports have been properly updated to use the new
ProcessedFileclass and process functions.
8-24: Good implementation of audio processing.The new audio processing code follows the same pattern as the existing video and image processing code, maintaining consistency across the different media types.
26-30: Updated video processing with output path.The video processing code has been correctly updated to use the new
processVideofunction with the additional output parameter.
44-48: Updated image processing with output path.The image processing code has been correctly updated to use the new
processImagefunction with the additional output parameter.packages/server/src/videos/process-video-handler.ts (4)
3-4: LGTM! Updated imports.Correctly updated the imports to use
ProcessedFileandprocessVideofrom the m4k package.
31-32: Good validation for request body.Added proper validation to check for the presence of a request body before processing.
41-43: Simplified video processing.The code has been streamlined to directly call
processVideowith the request body instead of creating temporary files first, which is more efficient.
48-62: Updated ProcessedFile handling logic.The handler has been properly updated to work with the new
ProcessedFileclass and handle output files correctly.packages/core/src/util/ffmpeg-processor.ts (6)
1-8: LGTM! Updated imports.The imports have been correctly updated to support the new functionality.
10-16: Improved documentation.The function documentation clearly explains that the input can be a file path, a stream, or a blob, which is a good improvement.
17-25: Good API unification.The function signature has been updated to accept multiple input types (string, AsyncIterable, or Blob) as specified in the PR objectives, making the API more flexible.
26-91: LGTM! Updated ffmpeg processing logic.The function has been refactored to handle different input types and properly process media using ffmpeg. The cleanup logic has been improved to handle temporary files correctly.
104-116: Good helper function for temporary files.The
createTmpfunction is a clean abstraction for handling temporary file creation from stream or blob inputs.
118-120: Secure random ID generation.The
randomIdfunction uses cryptographically secure random values, which is appropriate for generating temporary file names.packages/client/src/client.spec.ts (6)
1-4: Updated imports to reflect the new API design.The imports now correctly reflect the API renaming pattern with
ProcessedFilereplacingConvertedFile, and adding the new processing functions. The naming convention is more consistent as all media types now use a "process" prefix.
6-8: Simplified setFetch implementation.The updated implementation for mocking the fetch function uses cleaner TypeScript casting with
as anyfor better compatibility with the testing environment.
10-32: Well-structured test for the new audio processing functionality.This new test case for
processAudio()follows the same pattern as the other media processing tests, ensuring consistency across the test suite. It properly verifies the audio file processing by checking the returned file properties.
34-42: Updated image processing test to use the new function name.The test for image processing has been correctly updated to use
processImageinstead ofoptimizeImage, maintaining the same functionality but aligning with the new naming convention.
43-58: Correctly updated type checking for processed files.The test now checks for
ProcessedFileinstances instead ofConvertedFile, which aligns with the updated type system. The expectations for the processed files remain the same, ensuring backward compatibility.
60-82: Well-implemented test for video processing functionality.The new test for
processVideo()follows the same pattern as the audio and image tests, ensuring consistency. Usingexpect.any(Number)for the video file size is appropriate since video processing might result in variable file sizes.packages/client/src/client.ts (7)
1-9: Updated import statements to reflect the new type system.The imports now correctly include
ProcessedFileinstead ofConvertedFileand add the newAudioOptionstype. The naming convention is more consistent across all media types withImageOptionsandVideoOptions.
18-35: Well-implemented audio processing function with comprehensive documentation.The new
processAudiofunction follows the same pattern as the other media processing functions, ensuring a consistent API. The JSDoc comments clearly explain the purpose, parameters, and return values of the function.
37-54: Renamed and updated image processing function with improved documentation.The function has been renamed from
optimizeImagetoprocessImagefor consistency with other media types. The parameter types have been simplified to only acceptAsyncIterable<Uint8Array> | Blob, and comprehensive JSDoc comments have been added.
56-73: Renamed and updated video processing function with improved documentation.The function has been renamed from
optimizeVideotoprocessVideofor consistency with other media types. The parameter types have been simplified to only acceptAsyncIterable<Uint8Array> | Blob, and comprehensive JSDoc comments have been added.
75-79: Simplified the input parameter type for the optimizeFetch function.The input parameter type has been updated to match the simplified types used in the processing functions.
80-86: Improved request construction in optimizeFetch function.The code for creating and sending the HTTP request is cleaner and more readable.
96-100: Updated to use ProcessedFile instead of ConvertedFile.The code now yields instances of
ProcessedFileinstead ofConvertedFile, aligning with the updated type system.
Summary by CodeRabbit
New Features
Documentation