-
Notifications
You must be signed in to change notification settings - Fork 340
🚸 Let loader during upload analyze #984
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
Conversation
141b25f
to
d006e5c
Compare
d006e5c
to
81f71f7
Compare
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.
Pull Request Overview
This PR ensures the upload button loader remains active until the server-side file analysis completes and extends backend media authorization to include HEAD requests.
- Frontend: Introduced
waitForUploadAnalyze
inuseUploadFile
to poll until analysis finishes. - Tests: Added unit tests covering retry logic and failure scenarios for the upload hook.
- Backend: Updated
media_auth
to inspectHTTP_X_ORIGINAL_METHOD
(allowing GET/HEAD) and modified S3 header utility to accept a method parameter.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/frontend/apps/impress/src/features/docs/doc-editor/hook/useUploadFile.tsx | Added polling helper to delay returning URL until analysis completes |
src/frontend/apps/impress/src/features/docs/doc-editor/tests/useUploadFile.test.tsx | Added tests covering retry, max retries, non-200, and error flows |
src/backend/core/tests/documents/test_api_documents_media_auth.py | Parameterized tests for GET, HEAD allowed and other methods denied |
src/backend/core/api/viewsets.py | Media auth updated to read and enforce HTTP_X_ORIGINAL_METHOD |
src/backend/core/api/utils.py | Extended generate_s3_authorization_headers to accept HTTP method |
CHANGELOG.md | Added entry for maintaining loader during upload analysis |
Comments suppressed due to low confidence (2)
src/frontend/apps/impress/src/features/docs/doc-editor/hook/useUploadFile.tsx:20
- [nitpick] Consider renaming SLEEP_TIME to RETRY_DELAY_MS or similar to clarify that this constant represents a delay in milliseconds.
const SLEEP_TIME = 4000;
src/backend/core/api/viewsets.py:1298
- Ensure that 'logger' is imported or defined in this module to avoid a ReferenceError when logging unsupported methods.
logger.debug("Unsupported HTTP method: %s", original_method)
src/frontend/apps/impress/src/features/docs/doc-editor/hook/useUploadFile.tsx
Outdated
Show resolved
Hide resolved
@@ -22,6 +49,8 @@ export const useUploadFile = (docId: string) => { | |||
body, | |||
}); | |||
|
|||
await waitForUploadAnalyze(`${mediaUrl}${ret.file}`); |
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.
The result of waitForUploadAnalyze is ignored; if analysis ultimately fails (returns false), the hook will still return a URL. Consider handling or surfacing failures explicitly.
await waitForUploadAnalyze(`${mediaUrl}${ret.file}`); | |
const isAnalysisSuccessful = await waitForUploadAnalyze(`${mediaUrl}${ret.file}`); | |
if (!isAnalysisSuccessful) { | |
throw new Error('File analysis failed. Please try again.'); | |
} |
Copilot uses AI. Check for mistakes.
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.
Yes, I am not sure what to do if it is unsuccessful, could just be that it takes more time than usually.
2a8e381
to
62abe47
Compare
0c459cf
to
88538f5
Compare
88538f5
to
2d27755
Compare
b41be7f
to
2b32915
Compare
With the usage of a malware detection system, we need a way to know the file status. The front will use it to display a loader while the analyse is not ended.
We want to have the media-check url returned on the attachment-upload response instead of the media url directly. The front will know the endpoint to use to check the media status.
2b32915
to
bb50332
Compare
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.
Pull Request Overview
This PR adds a backend endpoint to poll media status and updates the frontend to display a loader until file analysis completes.
- Expose a new
media_check
action on the document viewset and update attachment upload to return a media-check URL - Implement
useUploadStatus
and enhanceuseUploadFile
to poll themedia_check
endpoint and replace the upload button with a loader until ready - Extend backend and e2e tests to cover the analysis workflow and bump
django-lasuite
version
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
useUploadFile.tsx (frontend) | Added polling logic (loopCheckDocMediaStatus ), loader DOM updates, and switched to backendUrl |
viewsets.py (backend/core/api) | Added media_check endpoint, adjusted attachment_upload to return its URL, and added error handling |
test_api_documents_media_check.py (backend tests) | New test suite covering media_check responses under various scenarios |
doc-editor.spec.ts (e2e) | Added an integration test for the analyze flow with mocked media-check responses |
test_models_documents.py & related permission tests (backend models) | Added media_check to document abilities and adjusted tests accordingly |
pyproject.toml | Bumped django-lasuite from 0.0.8 to 0.0.9 |
CHANGELOG.md | Documented the addition of the media-check endpoint and loader behavior |
Comments suppressed due to low confidence (3)
src/frontend/apps/impress/src/features/docs/doc-editor/hook/useUploadFile.tsx:5
- The
useMediaUrl
import is no longer used after switching tobackendUrl
; consider removing this unused import to clean up the code and avoid lint warnings.
import { useMediaUrl } from '@/core/config';
src/frontend/apps/impress/src/features/docs/doc-editor/hook/useUploadFile.tsx:125
- There's a typo in
timoutId
; rename it totimeoutId
to match thetimeoutIds
key and improve readability.
const timoutId = setTimeout(() => {
src/backend/core/api/viewsets.py:1293
- The exception
ClientError
is not imported, which will cause a NameError at runtime. Please addfrom botocore.exceptions import ClientError
at the top of the file.
except ClientError as err:
* @returns Promise<CheckDocMediaStatusResponse> status_code | ||
* @description Waits for the upload to be analyzed by checking the status of the file. | ||
*/ | ||
const loopCheckDocMediaStatus = async (url: string) => { |
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.
[nitpick] The recursive polling has no maximum retry limit or timeout guard. Consider adding a retry counter or overall timeout to prevent potential infinite loops if the status never becomes ready
.
const loopCheckDocMediaStatus = async (url: string) => { | |
const loopCheckDocMediaStatus = async ( | |
url: string, | |
maxRetries: number = 10, | |
currentRetry: number = 0, | |
) => { |
Copilot uses AI. Check for mistakes.
The backend can analyze the upload file, this take time, so we need to show a loader until the backend finish the analysis.
We need version 0.0.9 to reduce the time to have a JCOP analysis result.
In the malware_detection callback we are using a different logger named docs.security. We want to configure a logger in the logging settings handling it.
bb50332
to
99ba414
Compare
Purpose
We use "Je Clique ou pas" to analyze the upload. the problem is that the analyze takes a few seconds (about 30 secondes).
It gives the feeling to the users that the upload failed.
Proposal
Let the loader be displayed until the analyze is done.
Demo