Support Video #81
Support Video #81
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce new features and enhancements to the Bluesky API, focusing on video management and service authentication. Key additions include constants for video-related operations, a new Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (6)
core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/com/atproto/server/ServerGetServiceAuthResponse.kt (1)
1-8: LGTM, but consider making the class immutable.The code changes are approved. The class follows the naming convention, uses the
@Serializableannotation correctly, and has a clear and descriptive property name.However, consider making the class immutable by using a constructor parameter for the
tokenproperty instead oflateinit. This ensures that the property is always initialized and prevents accidental reassignment.Here's how you can refactor the class:
@Serializable data class ServerGetServiceAuthResponse(val token: String)Using a data class with a constructor parameter automatically generates a
copyfunction,equals,hashCode, andtoStringmethods, making the class more concise and easier to work with.core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedDefsAspectRatio.kt (1)
6-10: Add documentation comments to improve code readability.To enhance the readability and maintainability of the code, consider adding documentation comments (
/**...*/) to the class and its properties. This will help other developers understand the purpose and usage of theEmbedDefsAspectRatioclass more easily.core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/video/JobStatus.kt (1)
10-11: Improve code documentation.To enhance the code documentation, consider expanding the comment for the
stateproperty to provide more details about the possible states of a video processing job. You can include information about the meaning of each known state value and clarify what the other values represent when the job is in progress.For example:
// The state of the video processing job. // Known values: // - "JOB_STATE_COMPLETED": The job has successfully completed. // - "JOB_STATE_FAILED": The job has failed. // All other values indicate that the job is currently in progress.core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/com/atproto/server/ServerGetServiceAuthRequest.kt (1)
6-21: Add documentation comments and consider renaming properties for better clarity.The
ServerGetServiceAuthRequestclass and its properties lack documentation comments explaining their purpose and usage. Consider adding documentation comments to help developers understand when and how to use this class.Additionally, the property names
aud,exp, andlxmare short and not very descriptive. Consider renaming them to more meaningful names or adding documentation comments to explain what they represent. For example:/** * Represents a request to get service authentication. * * @property aud The audience (did) for the authentication token. * @property exp The expiration time for the authentication token. * @property lxm The lexicon version for the authentication token. */ class ServerGetServiceAuthRequest( accessJwt: String, val aud: String, val exp: Long? = null, val lxm: String? = null, ) : MapRequest, AuthRequest(accessJwt) { // ... }This will improve the readability and maintainability of the code.
core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedVideoView.kt (1)
1-27: LGTM, but consider adding documentation comments.The code changes look good and follow the necessary conventions. The class structure, property definitions, and serialization annotations are all implemented correctly.
To improve code readability and maintainability, consider adding documentation comments for the class and its properties. For example:
/** * Represents a video embed view. */ @Serializable class EmbedVideoView : EmbedViewUnion() { // ... /** * The content identifier of the video. */ lateinit var cid: String /** * The URI of the video playlist. */ lateinit var playlist: String /** * The thumbnail URL of the video (optional). */ var thumbnail: String? = null /** * The alternative text for the video (optional). */ var alt: String? = null /** * The aspect ratio of the video (optional). */ var aspectRatio: EmbedDefsAspectRatio? = null }core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedVideo.kt (1)
12-29: LGTM, but address the TODO and add documentation.The
EmbedVideoclass is correctly defined and follows the Kotlin conventions. However, please consider the following suggestions:
- Address the TODO comment about adding support for captions. If you plan to implement this feature in the future, consider creating a separate issue to track it.
- Add documentation for the properties to improve the readability and maintainability of the code. For example:
/** * The video blob. */ var video: Blob? = null /** * The alternative text for the video. */ var alt: String? = null /** * The aspect ratio of the video. */ var aspectRatio: EmbedDefsAspectRatio? = null
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (29)
- core/src/commonMain/kotlin/work/socialhub/kbsky/ATProtocolTypes.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/Bluesky.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/BlueskyTypes.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/app/bsky/VideoResource.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/com/atproto/ServerResource.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoGetJobStatusRequest.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoGetJobStatusResponse.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoGetUploadLimitsRequest.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoGetUploadLimitsResponse.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoUploadVideoRequest.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoUploadVideoResponse.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/com/atproto/repo/RepoUploadBlobRequest.kt (1 8000 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/com/atproto/server/ServerGetServiceAuthRequest.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/com/atproto/server/ServerGetServiceAuthResponse.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/share/AuthRequest.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/internal/_Bluesky.kt (3 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/internal/app/bsky/_VideoResource.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/internal/com/atproto/_RepoResource.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/internal/com/atproto/_ServerResource.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedDefsAspectRatio.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedUnion.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedVideo.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedVideoView.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedViewUnion.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/video/JobStatus.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/util/json/EmbedPolymorphicSerializer.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/util/json/EmbedViewPolymorphicSerializer.kt (2 hunks)
- core/src/jvmTest/kotlin/work/socialhub/kbsky/app/bsky/feed/PostTest.kt (1 hunks)
- core/src/jvmTest/kotlin/work/socialhub/kbsky/com/atproto/repo/UploadBlobTest.kt (1 hunks)
Files skipped from review due to trivial changes (1)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoUploadVideoResponse.kt
Additional comments not posted (34)
core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedDefsAspectRatio.kt (1)
1-10: LGTM!The code follows Kotlin naming conventions and is properly annotated with
@Serializable. The class structure is clear and concise.core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoGetUploadLimitsRequest.kt (1)
1-7: LGTM!The code changes in this new file are approved. The
VideoGetUploadLimitsRequestclass is defined correctly, extending theAuthRequestclass and taking anaccessJwtparameter in its constructor. The class name and package structure follow the conventions of the project.The purpose of this class is clear, representing a request for getting video upload limits. It doesn't contain any additional functionality beyond extending the
AuthRequestclass, which is appropriate for its intended use.Overall, the code is clean, well-structured, and follows the project's conventions.
core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoGetJobStatusResponse.kt (1)
1-9: LGTM!The code in this file follows Kotlin conventions and best practices. The
VideoGetJobStatusResponseclass is well-structured and serves a clear purpose within the context of the video-related functionality. The use of@Serializableannotation and thelateinitkeyword for thejobStatusproperty is appropriate.Overall, the code changes in this file are approved.
core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/com/atproto/repo/RepoUploadBlobRequest.kt (1)
9-9: Approve the addition ofcontentTypebut consider a more generic default value and missing parameters.The addition of the
contentTypeproperty is a good change as it allows specifying the media type of the uploaded blob, enhancing the functionality of the request.However, the default value of
"image/jpeg"might not be suitable for all use cases, especially for uploading videos as mentioned in the PR objectives. Consider using a more generic default value like"application/octet-stream"to support various media types.-var contentType: String = "image/jpeg", +var contentType: String = "application/octet-stream",Also, the PR objectives mention that the
uploadVideofunction requires specific parameters in the URL, including the device ID (did) and the filename. These parameters are not reflected in theRepoUploadBlobRequestclass. Could you please clarify how these parameters are being handled and if they need to be added to this class?core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoGetUploadLimitsResponse.kt (1)
1-12: LGTM!The code changes are approved. The
VideoGetUploadLimitsResponsedata class is well-defined with clear and descriptive property names. The use of nullable types for optional properties and the@Serializableannotation are appropriate.core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoGetJobStatusRequest.kt (1)
1-16: LGTM!The code changes are approved. The
VideoGetJobStatusRequestclass is correctly implemented with the following observations:
- The package declaration and imports are correctly defined.
- The class is correctly declared with the appropriate name, constructor parameters, and inheritance.
- The
toMapfunction is correctly overridden to return a map representation of the request.- The code follows the Kotlin coding conventions, is properly formatted, and follows the naming conventions.
- The code does not have any error handling, concurrency, or security issues as it is a simple data class.
core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/video/JobStatus.kt (1)
1-17: LGTM!The
JobStatusdata class is well-defined and follows the project's conventions. The properties are appropriately typed, and the use of nullable types for optional properties is correct. The@Serializableannotation ensures proper serialization support.Great job!
core/src/commonMain/kotlin/work/socialhub/kbsky/Bluesky.kt (2)
8-8: LGTM!The import statement is necessary to use the
VideoResourceclass and aligns with the PR objective of implementing video-related functionality.
17-17: LGTM!The addition of the
video()function to theBlueskyinterface expands its functionality to support video-related resources, which aligns with the PR objective. The function signature is also consistent with the other resource functions in the interface.core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedUnion.kt (2)
10-10: LGTM!The documentation comment is correctly updated to reference
EmbedVideo, which is consistent with the addition of theasVideoproperty.
21-21: LGTM!The addition of the
asVideoproperty to theEmbedUnionclass is a valid change that enhances its functionality by providing an additional way to interact with video embeds, similar to the existing properties for other embed types. The implementation using the safe cast operator is correct and consistent with the existing code. This change aligns well with the PR objectives of implementing video functionality within the application.core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedViewUnion.kt (2)
10-10: LGTM!The new KDoc comment is consistent with the existing comments and provides a clear reference to the
EmbedVideoViewclass.
21-21: LGTM!The new
asVideoproperty is implemented consistently with the existing properties for other embed view types. It allows instances ofEmbedViewUnionto be safely cast toEmbedVideoView, which aligns with the class's purpose and behavior.core/src/jvmTest/kotlin/work/socialhub/kbsky/com/atproto/repo/UploadBlobTest.kt (1)
25-25: LGTM!The code change is approved. Adding the
contentTypeparameter with the value"image/png"is a good practice as it helps the server correctly handle the uploaded PNG image file.core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/share/AuthRequest.kt (2)
28-28: LGTM!The change to return the entire
Jwtobject instead of just thesubfield is approved. It allows for greater flexibility without introducing any breaking changes or new issues.
14-17: LGTM, but verifyjwt.subis always non-null.The change to make
didnon-nullable and retrieve its value fromjwt.subis approved as it enforces a more robust design. However, please ensure thatjwt.subwill always have a value. Ifjwt.subcan be null, it will result in a runtime exception when accessingdid.Run the following script to verify that
jwt.subis always non-null:Verification successful
Verification successful:
jwt.subis always non-null.The
jwt.subproperty is declared aslateinit var sub: String, which ensures it is initialized before use and cannot be null. Additionally, there are no assignments ofnulltosubin theAuthRequest.ktfile. This confirms that the change to makedidnon-nullable is safe and robust.
jwt.subis declared aslateinit var sub: String, ensuring non-nullability.- No
nullassignments tosubwere found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `jwt.sub` is always non-null. # Test 1: Search for nullable declarations of `sub` in `Jwt` class. Expect: No results. rg --type kotlin $'lateinit var sub: String\?' -g AuthRequest.kt # Test 2: Search for assignments of null to `sub` in `Jwt` class. Expect: No results. rg --type kotlin $'sub = null' -g AuthRequest.ktLength of output: 227
core/src/commonMain/kotlin/work/socialhub/kbsky/api/app/bsky/VideoResource.kt (2)
19-21: LGTM!The code changes are approved. The function signature follows the naming convention and takes the appropriate request and response types.
26-28: LGTM!The code changes are approved. The function signature follows the naming convention and takes the appropriate request and response types.
core/src/commonMain/kotlin/work/socialhub/kbsky/util/json/EmbedPolymorphicSerializer.kt (2)
13-13: LGTM!The code changes are approved.
26-26: LGTM!The code changes are approved. The addition of the
EmbedVideocase in thewhenexpression is consistent with the existing pattern and is necessary to support deserialization of theEmbedVideotype.core/src/commonMain/kotlin/work/socialhub/kbsky/util/json/EmbedViewPolymorphicSerializer.kt (2)
11-11: LGTM!The code changes are approved.
26-26: LGTM!The code changes are approved. The new case correctly handles the
EmbedVideoViewtype and enhances the functionality of theEmbedViewPolymorphicSerializer.core/src/commonMain/kotlin/work/socialhub/kbsky/internal/_Bluesky.kt (3)
10-10: LGTM!The imports are necessary to use the
VideoResourceand_VideoResourceclasses in the_Blueskyclass.Also applies to: 17-17
29-29: LGTM!The addition of the
videoproperty is consistent with the other resource properties in the_Blueskyclass. The initialization of thevideoproperty with an instance of_VideoResourceclass is also consistent with the initialization of other resource properties.
57-60: LGTM!The addition of the
video()method is consistent with the other resource methods in the_Blueskyclass. Thevideo()method correctly overrides an existing method and returns thevideoproperty.core/src/commonMain/kotlin/work/socialhub/kbsky/api/com/atproto/ServerResource.kt (1)
55-62: LGTM!The code changes are approved for the following reasons:
- The function signature and return type are consistent with the provided specification.
- The KDoc comment provides sufficient context about the purpose and usage of the function.
- The function is essential for the video upload feature, as mentioned in the PR objectives.
core/src/commonMain/kotlin/work/socialhub/kbsky/ATProtocolTypes.kt (1)
48-48: LGTM!The addition of the
ServerGetServiceAuthconstant to theATProtocolTypesobject is consistent with the existing pattern and naming convention. It aligns with the PR objective of implementing agetServiceAuthmethod for generating authentication tokens from the PDS.The code changes are approved.
core/src/commonMain/kotlin/work/socialhub/kbsky/BlueskyTypes.kt (1)
26-26: LGTM!The code changes are approved for the following reasons:
- The constants are correctly defined and follow the naming convention used in the file.
- The constants are public, which is consistent with the other constants in the file.
- The constants are grouped under the "Video" category, which is a new category in the file.
- The constants are consistent with the PR objectives, which mention the implementation of video playback and upload functionality.
Also applies to: 78-81
core/src/commonMain/kotlin/work/socialhub/kbsky/internal/com/atproto/_RepoResource.kt (1)
130-130: LGTM!The code change is approved. Replacing the hardcoded "Content-Type" value with a dynamic value derived from
request.contentTypeenhances the flexibility of the HTTP request handling and aligns with the PR objective of supporting video playback and upload functionality.core/src/commonMain/kotlin/work/socialhub/kbsky/internal/com/atproto/_ServerResource.kt (1)
75-88: LGTM!The code changes are approved. The new
getServiceAuthfunction is correctly implemented and follows the existing pattern of other functions in the class. It enhances the functionality of the_ServerResourceclass by allowing it to handle service authentication, which was not previously implemented.The function uses good practices such as:
- Using the
proceedfunction to handle the response.- Using the
runBlockingfunction to perform the HTTP request.- Using the
toMapfunction to convert the request parameters to a map.- Using the
xrpcfunction to construct the URL.- Using the
acceptfunction to specify the expected response format.- Using the
headerfunction to add the authorization header.core/src/commonMain/kotlin/work/socialhub/kbsky/internal/app/bsky/_VideoResource.kt (4)
29-46: LGTM!The
getJobStatusfunction looks good. It follows the necessary steps to retrieve the video job status by obtaining a video token from the PDS and making an HTTP request to the video service endpoint with the required headers and parameters.
48-64: LGTM!The
getUploadLimitsfunction looks good. It follows a similar pattern togetJobStatusand correctly retrieves the upload limits by obtaining a video token from the PDS and making an HTTP request to the video service endpoint with the required headers.
66-101: LGTM!The
uploadVideofunction looks good. It follows the necessary steps to upload a video by obtaining a video token from the PDS with the requiredaudandlxmparameters and making an HTTP POST request to the video service endpoint with the necessary headers, parameters, and the video file.
103-116: LGTM!The
getVideoTokenFromPdshelper function looks good. It correctly handles the process of obtaining a video token from the PDS by making a request to the_ServerResourcewith the provided parameters and extracting the bearer token from the response.
| var width: Int = 1 | ||
| var height: Int = 1 |
There was a problem hiding this comment.
Consider adding validation for positive values.
To ensure that the width and height properties always have positive values, consider adding validation in the constructor or setter methods. This can help prevent unexpected behavior if negative values are assigned.
Consider making the properties immutable if the aspect ratio shouldn't change.
If the aspect ratio is not expected to change once set, consider declaring the width and height properties as val instead of var. This will make the properties immutable and prevent accidental modifications after the object is created.
| class VideoUploadVideoRequest( | ||
| accessJwt: String, | ||
| var bytes: ByteArray, | ||
| var name: String = "data.mp4", | ||
| var contentType: String = "video/mp4", | ||
| ) : AuthRequest(accessJwt) |
There was a problem hiding this comment.
Make constructor parameters immutable and add validation.
Consider the following suggestions to improve the VideoUploadVideoRequest class:
-
Make
bytes,name, andcontentTypevals instead ofvars to ensure immutability. This will prevent unexpected modifications to these properties after the object is created. -
Add validation for the
bytesparameter to ensure that it's not empty. This will help catch invalid requests early and provide better error messages to the caller. -
Add validation for the
contentTypeparameter to ensure that it's a valid MIME type for videos. This will help catch invalid requests early and provide better error messages to the caller.
Apply this diff to make the suggested changes:
class VideoUploadVideoRequest(
accessJwt: String,
- var bytes: ByteArray,
- var name: String = "data.mp4",
- var contentType: String = "video/mp4",
+ val bytes: ByteArray,
+ val name: String = "data.mp4",
+ val contentType: String = "video/mp4",
) : AuthRequest(accessJwt) {
+ init {
+ require(bytes.isNotEmpty()) { "bytes must not be empty" }
+ require(contentType.startsWith("video/")) { "contentType must be a valid video MIME type" }
+ }
}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.
| class VideoUploadVideoRequest( | |
| accessJwt: String, | |
| var bytes: ByteArray, | |
| var name: String = "data.mp4", | |
| var contentType: String = "video/mp4", | |
| ) : AuthRequest(accessJwt) | |
| class VideoUploadVideoRequest( | |
| accessJwt: String, | |
| val bytes: ByteArray, | |
| val name: String = "data.mp4", | |
| val contentType: String = "video/mp4", | |
| ) : AuthRequest(accessJwt) { | |
| init { | |
| require(bytes.isNotEmpty()) { "bytes must not be empty" } | |
| require(contentType.startsWith("video/")) { "contentType must be a valid video MIME type" } | |
| } | |
| } |
| fun uploadVideo( | ||
| request: VideoUploadVideoRequest | ||
| ): Response<VideoUploadVideoResponse> |
There was a problem hiding this comment.
Tip
Codebase Verification
Missing did parameter in VideoUploadVideoRequest.
The VideoUploadVideoRequest class does not contain the did parameter as required by the PR objectives. The name parameter might serve as the filename, but this should be confirmed. Please verify how the did parameter is handled or consider adding it to the request class.
- Location of
VideoUploadVideoRequestclass:core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/video/VideoUploadVideoRequest.kt
Analysis chain
LGTM, but verify the request parameters.
The code changes are approved. The function signature follows the naming convention and takes the appropriate request and response types.
However, the PR objectives mention that the uploadVideo function requires specific parameters in the URL, including the device ID (did) and the filename. Please verify that the VideoUploadVideoRequest class contains these parameters.
Run the following script to verify the request parameters:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the `VideoUploadVideoRequest` class contains the required parameters.
# Test: Search for the class definition. Expect: The class definition with the required parameters.
ast-grep --lang kotlin --pattern $'data class VideoUploadVideoRequest(
$$$
val did: String,
val filename: String,
$$$
)'
Length of output: 129
Script:
#!/bin/bash
# Description: Search for the `VideoUploadVideoRequest` class definition to confirm its existence and inspect its properties.
# Search for the class definition without specifying parameter names.
ast-grep --lang kotlin --pattern $'data class VideoUploadVideoRequest($$$)'
Length of output: 76
Script:
#!/bin/bash
# Description: Search for occurrences of `VideoUploadVideoRequest` in the codebase to find its definition or references.
# Search for any occurrences of `VideoUploadVideoRequest`.
rg 'VideoUploadVideoRequest' -A 5
Length of output: 3517
| @@ -44,6 +44,7 @@ class PostTest : AbstractTest() { | |||
| accessJwt = accessJwt, | |||
| name = "icon.png", | |||
| bytes = stream.readBytes(), | |||
| contentType = "image/jpeg" | |||
There was a problem hiding this comment.
Inconsistent file extension and content type.
The file being uploaded is named "icon.png", which suggests it's a PNG image. However, the contentType parameter is set to "image/jpeg", indicating a JPEG image. This inconsistency could lead to issues if the server or client relies on the content type for processing the image.
Consider using the correct content type that matches the file extension:
-contentType = "image/jpeg"
+contentType = "image/png"Alternatively, rename the file to match the specified content type:
-name = "icon.png",
+name = "icon.jpg",Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and ha 8000 s no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contentType = "image/jpeg" | |
| contentType = "image/png" |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
core/src/jvmTest/kotlin/work/socialhub/kbsky/app/bsky/video/UploadVideoTest.kt (4)
19-20: Use a configuration file or environment variable for the video file path.The video file path is hard-coded in the test method. Consider using a configuration file or environment variable to specify the video file path. This will make the test more flexible and easier to maintain.
36-57: Use a configuration file or environment variable for the job timeout.The job timeout is hard-coded to 60 seconds. Consider using a configuration file or environment variable to specify the job timeout. This will make the test more flexible and easier to maintain.
60-62: Handle job failures and timeouts gracefully.The method throws an exception if the job fails or times out. Consider handling job failures and timeouts gracefully instead of throwing an exception. For example, you could log an error message and fail the test using an assertion.
94-98: Add assertions to validate the response data.The method only prints the response data and does not perform any assertions on the response. Consider adding assertions to validate the response data. For example, you could assert that the
canUploadfield istrue, theremainingDailyVideosandremainingDailyBytesfields are greater than or equal to 0, and themessageanderrorfields arenull.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
core/src/jvmTest/resources/video/heavy.mp4is excluded by!**/*.mp4core/src/jvmTest/resources/video/light.mp4is excluded by!**/*.mp4
Files selected for processing (10)
- core/src/commonMain/kotlin/work/socialhub/kbsky/ATProtocolException.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/BlueskyConfig.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/share/ErrorResponse.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/internal/app/bsky/_VideoResource.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/internal/com/atproto/_ServerResource.kt (3 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/internal/share/_InternalUtility.kt (3 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedVideo.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/plc/DIDDetails.kt (0 hunks)
- core/src/jvmTest/kotlin/work/socialhub/kbsky/app/bsky/feed/PostTest.kt (4 hunks)
- core/src/jvmTest/kotlin/work/socialhub/kbsky/app/bsky/video/UploadVideoTest.kt (1 hunks)
Files not reviewed due to no reviewable changes (1)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/plc/DIDDetails.kt
Files skipped from review as they are similar to previous changes (4)
- core/src/commonMain/kotlin/work/socialhub/kbsky/internal/app/bsky/_VideoResource.kt
- core/src/commonMain/kotlin/work/socialhub/kbsky/internal/com/atproto/_ServerResource.kt
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/embed/EmbedVideo.kt
- core/src/jvmTest/kotlin/work/socialhub/kbsky/app/bsky/feed/PostTest.kt
Additional context used
Learnings (1)
core/src/commonMain/kotlin/work/socialhub/kbsky/ATProtocolException.kt (1)
Learnt from: uakihir0 PR: uakihir0/kbsky#67 File: core/src/commonMain/kotlin/work/socialhub/kbsky/internal/_ATProtocol.kt:12-18 Timestamp: 2024-07-04T10:09:32.037Z Learning: The `_ATProtocol` class now uses `ATProtocolConfig` in its constructor instead of a URI string. Ensure all instances of `_ATProtocol` are updated accordingly.
Additional comments not posted (10)
core/src/commonMain/kotlin/work/socialhub/kbsky/BlueskyConfig.kt (3)
3-3: LGTM!The class declaration is syntactically correct. Extending
ATProtocolConfigis a design decision that cannot be evaluated without additional context.
5-5: LGTM!The property declaration is syntactically correct, and the default value aligns with the PR objectives.
6-6: LGTM!The property declaration is syntactically correct, and the default value aligns with the PR objectives.
core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/share/ErrorResponse.kt (1)
10-12: LGTM!The
messageForDisplayfunction is a nice addition to theErrorResponseclass. It enhances the usability by providing a user-friendly way to retrieve error messages, ensuring that a meaningful message is always displayed, either frommessageorerror.The implementation is concise and accurate, using the
ifBlankfunction to check if themessageproperty is blank and falling back to theerrorproperty if needed.core/src/commonMain/kotlin/work/socialhub/kbsky/ATProtocolException.kt (3)
8-9: LGTM!The addition of
statusandbodyproperties to theATProtocolExceptionclass is a good enhancement. It allows the exception to carry more context about the error, which can be helpful for debugging and error handling. The nullable nature of the properties ensures that the exception can still be used even if the status code or response body is not available.
16-16: LGTM!The modification to the secondary constructor is necessary to accommodate the new properties added to the primary constructor. By passing
nullforstatusandbody, the constructor maintains its original behavior of creating an exception with only a message.
17-17: LGTM!The modification to the secondary constructor is necessary to accommodate the new properties added to the primary constructor. By passing
nullforstatusandbody, the constructor maintains its original behavior of creating an exception with only an exception.core/src/commonMain/kotlin/work/socialhub/kbsky/internal/share/_InternalUtility.kt (3)
70-70: LGTM!Passing the
response.statusto thehandleErrorfunction is a good change. It provides additional context about the error state, which can aid in diagnosing issues more effectively.
96-96: LGTM!Adding the optional
statusparameter to thehandleErrorfunction is a good change. It allows the function to capture and propagate the HTTP response status code when an error occurs, enhancing the error handling by providing more context about the error.
108-111: LGTM!Including the
statusparameter in theATProtocolExceptioninstantiation is a good change. It further enriches the exception with relevant information, which is in line with the overall improvement to the error handling mechanism.
Support for playing and uploading videos.
動画の再生と投稿に対応しました。
再生は他とあまり変わりませんが、投稿についてはPDSと動画サーバが分かれているので連携がややこしいです。
PDS から
getServiceAuthでトークンを生成し、それを動画サーバに渡すことで認証します。uploadVideoはさらに url のパラメータに did とファイル名を含ませる必要があるなど、他とは色合いが違います。サンプルがなくて申し訳ないです。
よろしくお願いします。
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These updates significantly enhance user experience by providing robust video handling and secure service interactions within the application.