Layer tus-js-client on top of Axios #8281
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant AxiosHttpStack
participant AxiosHttpRequest
Client->>AxiosHttpStack: Create request
AxiosHttpStack->>AxiosHttpRequest: Generate new HTTP request
AxiosHttpRequest->>Server: Send upload request
Server->>AxiosHttpRequest: Process upload
AxiosHttpRequest->>AxiosHttpStack: Return response
AxiosHttpStack->>Client: Deliver response
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- cvat-core/package.json (1 hunks)
- cvat-core/src/axios-tus.ts (1 hunks)
- cvat-core/src/server-proxy.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- cvat-core/package.json
Additional context used
GitHub Check: Linter
cvat-core/src/axios-tus.ts
[warning] 35-35:
Missing return type on function
[warning] 38-38:
Missing return type on function
Additional comments not posted (9)
cvat-core/src/axios-tus.ts (7)
8-19: Ensure correct implementation ofAxiosHttpResponse.The
AxiosHttpResponseclass correctly implements thetus.HttpResponseinterface. The methodsgetStatus,getHeader,getBody, andgetUnderlyingObjectare implemented correctly. Ensure that thegetHeadermethod handles cases where the header might not exist, returningundefinedor a default value.
21-30: Ensure correct implementation ofAxiosHttpRequest.The
AxiosHttpRequestclass correctly implements thetus.HttpRequestinterface. The constructor initializes an Axios configuration with anAbortController. Ensure that thesetHeadermethod handles header name casing consistently, and verify that thesignalis correctly used for request cancellation.
42-46: Ensure correct implementation ofsetProgressHandler.The
setProgressHandlermethod correctly assigns a progress event handler to track upload progress. Ensure that this handler is compatible with the expected progress event structure.
48-51: Ensure correct implementation ofsend.The
sendmethod correctly uses Axios to send requests. Ensure that error handling is robust, especially for network errors or unexpected responses.
53-53: Ensure correct implementation ofabort.The
abortmethod correctly aborts the request using theAbortController. Ensure that this functionality is tested to confirm that requests are properly canceled.
58-63: Ensure correct implementation ofAxiosHttpStack.The
AxiosHttpStackclass correctly implements thetus.HttpStackinterface. ThecreateRequestmethod returns anAxiosHttpRequestinstance. Ensure that the integration withtus-js-clientis seamless.
65-65: Verify export ofaxiosTusHttpStack.The
axiosTusHttpStackis exported correctly. Verify that it is used appropriately in other parts of the codebase to ensure consistent request defaults.Verification successful
Export and Usage of
axiosTusHttpStackVerifiedThe
axiosTusHttpStackis exported and used correctly withincvat-core/src/server-proxy.tsas part of a configuration object for handling HTTP requests. This ensures consistent request defaults and error handling. No issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `axiosTusHttpStack` in the codebase. # Test: Search for the usage of `axiosTusHttpStack`. Expect: Occurrences in relevant files. rg --type ts 'axiosTusHttpStack'Length of output: 265
Script:
#!/bin/bash # Description: Search for the usage of `httpStack` in `cvat-core/src/server-proxy.ts` to verify the usage of `axiosTusHttpStack`. rg --type ts 'httpStack' -A 5 cvat-core/src/server-proxy.tsLength of output: 392
cvat-core/src/server-proxy.ts (2)
133-133: Ensure correct usage ofaxiosTusHttpStack.The
axiosTusHttpStackis used in thechunkUploadfunction for TUS uploads. Verify that it correctly handles the CSRF token inclusion and other Axios defaults.
13-13: Verify import ofaxiosTusHttpStack.The
axiosTusHttpStackis imported for use in thechunkUploadfunction. Ensure that it is correctly integrated and that the expected behavior aligns with the PR objectives.Verification successful
Verified import of
axiosTusHttpStack.The
axiosTusHttpStackis correctly imported and integrated into thechunkUploadfunction as part of the upload configuration, aligning with the PR objectives. No issues were found with its integration.
cvat-core/src/server-proxy.ts:axiosTusHttpStackis used in thehttpStackproperty.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct integration of `axiosTusHttpStack` in `chunkUpload`. # Test: Search for the usage of `axiosTusHttpStack` within `chunkUpload`. Expect: Correct integration. rg --type ts 'axiosTusHttpStack' -A 10Length of output: 2198
Currently, if `location` has a query string, the result is nonsense.
cvat-core/src/axios-tus.ts
Outdated
| } | ||
|
|
||
| getStatus(): number { return this.#axiosResponse.status; } | ||
| getHeader(header: string): string { return this.#axiosResponse.headers[header.toLowerCase()]; } |
There was a problem hiding this comment.
Probably it also may return undefined as in AxiosHttpRequest
| getHeader(header: string): string { return this.#axiosResponse.headers[header.toLowerCase()]; } | |
| getHeader(header: string): string | undefined { return this.#axiosResponse.headers[header.toLowerCase()]; } |
There was a problem hiding this comment.
Good point, fixed. This is actually a bug in the interface definition, so I also filed tus/tus-js-client#706.
This ensures that requests coming from tus-js-client have the same defaults as the ones coming from the rest of the UI. In particular, this ensures that TUS requests include the `X-CSRFTOKEN` header. Currently, this doesn't matter much, because TUS requests are authenticated using the token. However, I'd like to get rid of token authentication in the UI, after which `X-CSRFTOKEN` will become important.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8281 +/- ##
===========================================
- Coverage 83.37% 83.35% -0.02%
===========================================
Files 390 391 +1
Lines 41549 41533 -16
Branches 3861 3839 -22
===========================================
- Hits 34642 34621 -21
- Misses 6907 6912 +5
|
Motivation and context
This ensures that requests coming from tus-js-client have the same defaults as the ones coming from the rest of the UI. In particular, this ensures that TUS requests include the
X-CSRFTOKENheader.Currently, this doesn't matter much, because TUS requests are authenticated using the token. However, I'd like to get rid of token authentication in the UI, after which
X-CSRFTOKENwill become important.How has this been tested?
Manual testing.
Checklist
developbranch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
chunkUploadfunction by removing organization-specific parameters and callbacks, refining the upload process.