Phase 4: SDK gaps for CLI usage#28
Open
alxgsv wants to merge 3 commits intocodex/phase3-sdk-enhancementsfrom
Open
Phase 4: SDK gaps for CLI usage#28alxgsv wants to merge 3 commits intocodex/phase3-sdk-enhancementsfrom
alxgsv wants to merge 3 commits intocodex/phase3-sdk-enhancementsfrom
Conversation
WalkthroughThis pull request introduces breaking API changes to file and webhook services, adds support for optional include parameters and metadata in uploads, implements path-building utilities for conversions, and enhances codec handling for map-based form fields. Multiple service signatures are updated, new optional parameter types are added, and comprehensive test coverage is provided. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
conversion/path_test.go (1)
30-35: Consider adding edge case tests for video paths.The basic test validates a minimal path with
Format: "mp4". Consider adding tests for:
- Empty
Formatto document expected behavior- Partial cut parameters (
CutStartonly orCutLengthonly)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conversion/path_test.go` around lines 30 - 35, Add edge-case unit tests for BuildVideoPath using the VideoPathOptions struct: add a test that sets Format to an empty string to assert the expected output when format is omitted, and two tests for partial cut parameters—one with CutStart set and CutLength zero/omitted, and one with CutLength set and CutStart zero/omitted—to assert the path produced when only one cut param is provided; locate and extend TestBuildVideoPath_Basic (or create new tests named TestBuildVideoPath_EmptyFormat and TestBuildVideoPath_PartialCutStart/PartialCutLength) to validate these behaviors against the canonical path format returned by BuildVideoPath.conversion/path.go (2)
56-58: Consider defaulting or validatingFormatfor video paths.Unlike
BuildDocumentPathwhich defaultsFormatto"pdf",BuildVideoPathproduces a path with an empty format segment (uuid/video/-/format//) whenFormatis empty. This could result in invalid conversion paths.🛠️ Option: Add a default format or document the requirement
Either default to a common format:
func BuildVideoPath(opts VideoPathOptions) string { + format := opts.Format + if format == "" { + format = "mp4" + } + var b strings.Builder - fmt.Fprintf(&b, "%s/video/-/format/%s/", opts.UUID, opts.Format) + fmt.Fprintf(&b, "%s/video/-/format/%s/", opts.UUID, format)Or document that
Formatis required via the struct comment on line 40.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conversion/path.go` around lines 56 - 58, BuildVideoPath currently emits an empty format segment when VideoPathOptions.Format is blank; update BuildVideoPath to validate/default the format (similar to BuildDocumentPath) by checking if opts.Format == "" and setting a sensible default like "mp4" before formatting the path, or alternatively document that VideoPathOptions.Format is required in the VideoPathOptions struct comment so callers must supply it; reference the BuildVideoPath function and VideoPathOptions type when making the change.
71-73: Partial cut parameters produce no output — document or handle.If only
CutStartor onlyCutLengthis provided, the cut segment is silently omitted. This is likely intentional (both are required for a valid cut), but documenting this behavior in the struct field comments would help users avoid mistakes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conversion/path.go` around lines 71 - 73, The code silently omits a cut when only one of opts.CutStart or opts.CutLength is provided; either document that both fields are required on the options struct or add validation in the path-building code to handle partial input. Locate the options type that defines CutStart and CutLength and add a clear comment stating “both CutStart and CutLength must be set to produce a cut”; alternatively (or additionally) modify the path-construction logic that checks opts.CutStart/opts.CutLength to detect when only one is set and return an error (or log and skip) rather than silently omitting the cut, updating any callers to handle the validation result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@conversion/path_test.go`:
- Around line 30-35: Add edge-case unit tests for BuildVideoPath using the
VideoPathOptions struct: add a test that sets Format to an empty string to
assert the expected output when format is omitted, and two tests for partial cut
parameters—one with CutStart set and CutLength zero/omitted, and one with
CutLength set and CutStart zero/omitted—to assert the path produced when only
one cut param is provided; locate and extend TestBuildVideoPath_Basic (or create
new tests named TestBuildVideoPath_EmptyFormat and
TestBuildVideoPath_PartialCutStart/PartialCutLength) to validate these behaviors
against the canonical path format returned by BuildVideoPath.
In `@conversion/path.go`:
- Around line 56-58: BuildVideoPath currently emits an empty format segment when
VideoPathOptions.Format is blank; update BuildVideoPath to validate/default the
format (similar to BuildDocumentPath) by checking if opts.Format == "" and
setting a sensible default like "mp4" before formatting the path, or
alternatively document that VideoPathOptions.Format is required in the
VideoPathOptions struct comment so callers must supply it; reference the
BuildVideoPath function and VideoPathOptions type when making the change.
- Around line 71-73: The code silently omits a cut when only one of
opts.CutStart or opts.CutLength is provided; either document that both fields
are required on the options struct or add validation in the path-building code
to handle partial input. Locate the options type that defines CutStart and
CutLength and add a clear comment stating “both CutStart and CutLength must be
set to produce a cut”; alternatively (or additionally) modify the
path-construction logic that checks opts.CutStart/opts.CutLength to detect when
only one is set and return an error (or log and skip) rather than silently
omitting the cut, updating any callers to handle the validation result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af485dbd-c2d8-4905-87f4-4fc73b5e730b
📒 Files selected for processing (25)
CHANGELOG.mdREADME.mdconversion/conversion.goconversion/conversion_test.goconversion/path.goconversion/path_test.gofile/file.gofile/list.gofile/service.gofile/service_test.gointernal/codec/codec.gointernal/codec/codec_test.gotest/file.gotest/webhook.goucare/doc.goucare/uploadapi.goucare/uploadapi_test.goupload/direct.goupload/fromurl.goupload/multipart.goupload/upload.goupload/upload_test.gowebhook/service.gowebhook/service_test.gowebhook/webhook.go
alxgsv
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
API Changes