-
Notifications
You must be signed in to change notification settings - Fork 104
feat(cli): VIDEO-022 - Process video arguments with validation #895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Harshita-Rupani29
wants to merge
1
commit into
juspay:release
Choose a base branch
from
Harshita-Rupani29:feature/video-022-cli-video-argument-processing
base: release
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -547,14 +547,91 @@ export class CLICommandFactory { | |
| return resolveFilePaths(paths); | ||
| } | ||
|
|
||
| // Helper method to process CLI video files | ||
| // Supported video file extensions for CLI validation | ||
| private static readonly SUPPORTED_VIDEO_EXTENSIONS = [ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The extension list is too small. |
||
| ".mp4", | ||
| ".webm", | ||
| ".mov", | ||
| ".avi", | ||
| ".mkv", | ||
| ".m4v", | ||
| ".wmv", | ||
| ".flv", | ||
| ]; | ||
|
|
||
| /** | ||
| * Check if a string is a URL (http, https, file, or data URI) | ||
| */ | ||
| private static isURL(str: string): boolean { | ||
| const lower = str.toLowerCase(); | ||
| return ( | ||
| lower.startsWith("http://") || | ||
| lower.startsWith("https://") || | ||
| lower.startsWith("file://") || | ||
| lower.startsWith("data:") | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Validate video file paths and throw descriptive errors for invalid files | ||
| * | ||
| * @param filePaths - Array of file paths to validate | ||
| * @throws Error if a file doesn't exist or has an unsupported extension | ||
| */ | ||
| private static validateVideoFiles(filePaths: string[]): void { | ||
| for (const filePath of filePaths) { | ||
| // Skip URL validation - URLs are handled by the SDK | ||
| if (CLICommandFactory.isURL(filePath)) { | ||
| continue; | ||
| } | ||
|
|
||
| // Resolve relative paths to absolute | ||
| const absolutePath = path.resolve(process.cwd(), filePath); | ||
|
|
||
| // Check file exists | ||
| if (!fs.existsSync(absolutePath)) { | ||
| throw new Error( | ||
| `Video file not found: ${filePath}\n` + | ||
| ` Resolved path: ${absolutePath}\n` + | ||
| ` Please check the file path and try again.`, | ||
| ); | ||
| } | ||
|
|
||
| // Check file extension | ||
| const ext = path.extname(absolutePath).toLowerCase(); | ||
| if (!CLICommandFactory.SUPPORTED_VIDEO_EXTENSIONS.includes(ext)) { | ||
| throw new Error( | ||
| `Unsupported video format: ${ext}\n` + | ||
| ` File: ${filePath}\n` + | ||
| ` Supported formats: ${CLICommandFactory.SUPPORTED_VIDEO_EXTENSIONS.join(", ")}`, | ||
| ); | ||
| } | ||
|
|
||
| // Check file is readable | ||
| try { | ||
| fs.accessSync(absolutePath, fs.constants.R_OK); | ||
| } catch { | ||
| throw new Error( | ||
| `Cannot read video file: ${filePath}\n` + | ||
| ` Resolved path: ${absolutePath}\n` + | ||
| ` Please check file permissions.`, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Helper method to process CLI video files with validation | ||
| private static processCliVideoFiles( | ||
| videoFiles?: string | string[], | ||
| ): Array<Buffer | string> | undefined { | ||
| if (!videoFiles) { | ||
| return undefined; | ||
| } | ||
| const paths = Array.isArray(videoFiles) ? videoFiles : [videoFiles]; | ||
|
|
||
| // Validate video files before resolving paths | ||
| CLICommandFactory.validateVideoFiles(paths); | ||
|
|
||
| // Resolve relative paths to absolute paths before returning | ||
| // URLs are preserved as-is by resolveFilePaths | ||
| return resolveFilePaths(paths); | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add blank lines around fenced code blocks to satisfy MD031.
The new section has fenced blocks without surrounding blank lines, which markdownlint flags.
📝 Proposed fix
**Video Options Passed to SDK**: + ```typescript videoOptions: { frames: argv.videoFrames, // Number of frames to extract (default: 8) quality: argv.videoQuality, // Frame quality 0-100 (default: 85) format: argv.videoFormat, // jpeg or png (default: jpeg) transcribeAudio: argv.transcribeAudio // Audio transcription (default: false) }Error Handling:
@@
Example Commands:
+
Verify each finding against the current code and only fix it if needed.
In
@memory-bank/progress.mdaround lines 19 - 27, The fenced TypeScript and Bashcode blocks (e.g., the "videoOptions" block and the later example block around
lines 39-49) lack surrounding blank lines and violate MD031; insert a single
blank line immediately before and after each fenced code block so there is an
empty line above the opening
and an empty line below the closingforboth the videoOptions block and the example commands block.