Requests page #7537
Requests page #7537
Conversation
| if (response.status === 202) { | ||
| resolve(response.data.rq_id); | ||
| } | ||
| resolve(); |
There was a problem hiding this comment.
Are there any cases, when success response does not include rq_id?
There was a problem hiding this comment.
Yes, in case of export and backups, if the export is finished but not downloaded, new export is not started and we get 200 response without rq_id (the same thing for other same comments)
There was a problem hiding this comment.
So, then something is not working here.
For example:
- serverProxy.backupTask or backupProject returns string or undefined.
- it means Task.backup, and Project.backup also returns string or undefined
- string or undefined is used to start listening (in case with undefined, it seems there is typing error)
There was a problem hiding this comment.
Perhaps the same for export, but I did not check
There was a problem hiding this comment.
Yes, there was an error. Now we check here if we have the rq_id and only then we listen.
If we dont have an rq_id and no error is raised, that means export finished successfully
| if (response.status === 202) { | ||
| resolve(response.data.rq_id); | ||
| } | ||
| resolve(); |
There was a problem hiding this comment.
The same question about rq_id here
| } else if (isCloudStorage && status === 200) { | ||
| resolve(); | ||
| if (response.status === 202) { | ||
| resolve(response.data.rq_id); |
There was a problem hiding this comment.
The same question about rq_id
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- cvat-core/src/annotations.ts (2 hunks)
- cvat-core/src/project-implementation.ts (4 hunks)
- cvat-core/src/project.ts (5 hunks)
- cvat-core/src/requests-manager.ts (1 hunks)
- cvat-core/src/server-proxy.ts (23 hunks)
- cvat-core/src/session-implementation.ts (10 hunks)
- cvat-core/src/session.ts (4 hunks)
- cvat-ui/src/actions/export-actions.ts (4 hunks)
- cvat-ui/src/actions/import-actions.ts (5 hunks)
- cvat-ui/src/actions/requests-actions.ts (1 hunks)
- cvat-ui/src/actions/requests-async-actions.ts (1 hunks)
- cvat-ui/src/components/tasks-page/task-item.tsx (4 hunks)
- cvat-ui/src/containers/tasks-page/task-item.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (9)
- cvat-core/src/annotations.ts
- cvat-core/src/requests-manager.ts
- cvat-core/src/session-implementation.ts
- cvat-core/src/session.ts
- cvat-ui/src/actions/export-actions.ts
- cvat-ui/src/actions/import-actions.ts
- cvat-ui/src/actions/requests-actions.ts
- cvat-ui/src/actions/requests-async-actions.ts
- cvat-ui/src/components/tasks-page/task-item.tsx
Additional Context Used
GitHub Check Runs (1)
Linter success (10)
cvat-core/src/project-implementation.ts: [warning] 17-17:
Missing return type on function
cvat-core/src/project.ts: [warning] 210-210:
Missing return type on function
cvat-core/src/project.ts: [warning] 215-215:
Missing return type on function
cvat-core/src/project.ts: [warning] 220-220:
Missing return type on function
cvat-core/src/project.ts: [warning] 225-225:
Missing return type on function
cvat-core/src/project.ts: [warning] 240-240:
Missing return type on function
Additional comments not posted (13)
cvat-ui/src/containers/tasks-page/task-item.tsx (4)
20-20: AddedactiveRequesttoStateToPropsto handle request-related state.This addition aligns with the PR's objective to enhance request management in the UI.
38-42: Integration of request handling inmapStateToProps.The logic to find the active request based on the task ID and request type is correctly implemented.
8-8: Ensure that theRequestimport is used appropriately in the file.Verification successful
The
Requesttype is used appropriately in the filecvat-ui/src/containers/tasks-page/task-item.tsx.
activeRequest: Request | null;const activeRequest = Object.values(requests).find((request: Request) => {Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `Request` type is used correctly in the file. # Test: Search for the usage of `Request` type. Expect: Proper usage in function signatures and type annotations. rg --type typescript $'Request' cvat-ui/src/containers/tasks-page/task-item.tsxLength of output: 113
Script:
#!/bin/bash # Description: Verify that the `Request` type is used correctly in the file. # Test: Search for the usage of `Request` type. Expect: Proper usage in function signatures and type annotations. rg --type ts 'Request' cvat-ui/src/containers/tasks-page/task-item.tsxLength of output: 281
50-50: EnsureactiveRequestis correctly passed down to components.cvat-core/src/project-implementation.ts (4)
116-118: Refactor to handle potential undefinedoptions.This change ensures that the function does not throw an error if
optionsis undefined.
140-142: Simplify the return statement inrestoremethod.This modification reduces the complexity by removing an unnecessary variable and directly returning the result of the
listenmethod.
17-17: Add a return type to theimplementProjectfunction to enhance code readability and maintainability.Specifying a return type for the
implementProjectfunction will help developers understand what type of object is expected to be returned, improving code clarity.
15-15: Ensure thatRQStatusis used appropriately throughout the file.Verification successful
The usage of
RQStatusincvat-core/src/project-implementation.tsappears to be correct. It is imported from./enumsand used in a return statement to set the status of a newRequest.
import { RQStatus } from './enums';return new Request({ status: RQStatus.FINISHED, message: '' });Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `RQStatus` is used correctly in the file. # Test: Search for the usage of `RQStatus`. Expect: Proper usage in function signatures and type annotations. rg --type typescript $'RQStatus' cvat-core/src/project-implementation.tsLength of output: 106
Script:
#!/bin/bash # Description: Verify that `RQStatus` is used correctly in the file. # Test: Search for the usage of `RQStatus`. Expect: Proper usage in function signatures and type annotations. rg --type ts $'RQStatus' cvat-core/src/project-implementation.tsLength of output: 171
cvat-core/src/project.ts (2)
16-16: Ensure that theRequestimport is used appropriately in the file.Verification successful
The
Requesttype is used correctly in the filecvat-core/src/project.ts, specifically in theoptionsobject for therequestStatusCallbackfunction.
import { Request } from './requests-manager';options?: { requestStatusCallback?: (request: Request) => void },Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `Request` type is used correctly in the file. # Test: Search for the usage of `Request` type. Expect: Proper usage in function signatures and type annotations. rg --type typescript $'Request' cvat-core/src/project.tsLength of output: 90
Script:
#!/bin/bash # Description: Verify that the `Request` type is used correctly in the file. # Test: Search for the usage of `Request` type. Expect: Proper usage in function signatures and type annotations. rg --type ts 'Request' cvat-core/src/project.tsLength of output: 167
285-285: EnsureuploadStatusCallbackis used appropriately throughout the file.cvat-core/src/server-proxy.ts (3)
1638-1638: Ensure proper error handling when returning the request ID inuploadAnnotations.- return rqId; + try { + return rqId; + } catch (error) { + throw new Error('Failed to return the request ID: ' + error.message); + }This change ensures that any exceptions during the return of the request ID are caught and handled appropriately.
Likely invalid or redundant comment.
865-868: Consider refining the handling of HTTP status codes inexportDataset.- resolve(); + else { + reject(new Error('Unexpected response status: ' + response.status)); + }This change ensures that the promise is correctly rejected when the response status is not 202, which is necessary for proper error handling. Please consider this approach unless there's a specific reason to handle it differently as discussed previously.
Likely invalid or redundant comment.
944-944: Ensure proper error handling when returning the request ID inimportDataset.- return rqId; + try { + return rqId; + } catch (error) { + throw new Error('Failed to return the request ID: ' + error.message); + }This change ensures that any exceptions during the return of the request ID are caught and handled appropriately.
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- cvat-core/src/requests-manager.ts (1 hunks)
- cvat-core/src/session-implementation.ts (10 hunks)
- cvat-core/src/session.ts (4 hunks)
- cvat-ui/src/components/tasks-page/task-item.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- cvat-core/src/requests-manager.ts
- cvat-core/src/session-implementation.ts
- cvat-core/src/session.ts
- cvat-ui/src/components/tasks-page/task-item.tsx
There was a problem hiding this comment.
Actionable comments posted: 7
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- cvat-core/src/api.ts (2 hunks)
- cvat-core/src/index.ts (2 hunks)
- cvat-core/src/project-implementation.ts (3 hunks)
- cvat-core/src/project.ts (4 hunks)
- cvat-core/src/requests-manager.ts (1 hunks)
- cvat-core/src/server-proxy.ts (22 hunks)
- cvat-core/src/server-response-types.ts (1 hunks)
- cvat-core/src/session-implementation.ts (10 hunks)
- cvat-core/src/session.ts (4 hunks)
- cvat-core/tests/mocks/server-proxy.mock.cjs (5 hunks)
- cvat-ui/src/actions/import-actions.ts (5 hunks)
- cvat-ui/src/actions/requests-actions.ts (1 hunks)
- cvat-ui/src/actions/requests-async-actions.ts (1 hunks)
- cvat-ui/src/cvat-core-wrapper.ts (6 hunks)
- cvat-ui/src/reducers/requests-reducer.ts (1 hunks)
Files skipped from review as they are similar to previous changes (11)
- cvat-core/src/api.ts
- cvat-core/src/index.ts
- cvat-core/src/requests-manager.ts
- cvat-core/src/server-response-types.ts
- cvat-core/src/session-implementation.ts
- cvat-core/src/session.ts
- cvat-core/tests/mocks/server-proxy.mock.cjs
- cvat-ui/src/actions/requests-actions.ts
- cvat-ui/src/actions/requests-async-actions.ts
- cvat-ui/src/cvat-core-wrapper.ts
- cvat-ui/src/reducers/requests-reducer.ts
Additional Context Used
GitHub Check Runs (1)
Linter success (10)
cvat-core/src/project-implementation.ts: [warning] 15-15:
Missing return type on function
cvat-core/src/project.ts: [warning] 209-209:
Missing return type on function
cvat-core/src/project.ts: [warning] 214-214:
Missing return type on function
cvat-core/src/project.ts: [warning] 219-219:
Missing return type on function
cvat-core/src/project.ts: [warning] 224-224:
Missing return type on function
cvat-core/src/project.ts: [warning] 239-239:
Missing return type on function
Additional comments not posted (7)
cvat-core/src/project.ts (1)
224-228: Add return type to thebackupmethod to enhance code readability.- async backup( + async backup(): Promise<any> {Specifying a return type for the
backupmethod will help developers understand what type of object is expected to be returned, improving code clarity.Likely invalid or redundant comment.
cvat-core/src/server-proxy.ts (6)
865-868: Consider refining the handling of HTTP status codes inexportDataset.
904-904: Remove unnecessary conditional logic as suggested.
964-967: Consider refining the handling of HTTP status codes inbackupTask.
1049-1052: Consider refining the handling of HTTP status codes inbackupProject.
2246-2256: Enhance error messages ingetRequestsListfor better debugging and user feedback.
2270-2278: Enhance error messages incancelRequestfor better debugging and user feedback.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat-ui/src/actions/export-actions.ts (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- cvat-ui/src/actions/export-actions.ts
| shouldLog: !(action.payload.error instanceof ServerError) && | ||
| !(action.payload.error instanceof RequestError), |
There was a problem hiding this comment.
I will suggest ro write a single function iif we do not want to log both ServerError and RequestError.
Like:
const shouldLog = (error) => ![ServerError, RequestError].some((ErrorClass) => error instanceof ErrorClass);
...
{
shouldLog: shouldLog(action.payload.error)
}
Code duplication will be reduced, also cvat-ui does not know which core methods may throw RequestError. But we have it only on some reducers
| GET_REQUESTS_STATUS_SUCCESS = 'GET_REQUESTS_STATUS_SUCCESS', | ||
| GET_REQUESTS_STATUS_FAILED = 'GET_REQUESTS_STATUS_FAILED', |
There was a problem hiding this comment.
| GET_REQUESTS_STATUS_SUCCESS = 'GET_REQUESTS_STATUS_SUCCESS', | |
| GET_REQUESTS_STATUS_FAILED = 'GET_REQUESTS_STATUS_FAILED', | |
| GET_REQUEST_STATUS_SUCCESS = 'GET_REQUEST_STATUS_SUCCESS', | |
| GET_REQUEST_STATUS_FAILED = 'GET_REQUEST_STATUS_FAILED', |
| import { | ||
| Request, InstanceType, getCore, | ||
| } from 'cvat-core-wrapper'; |
There was a problem hiding this comment.
| import { | |
| Request, InstanceType, getCore, | |
| } from 'cvat-core-wrapper'; | |
| import { Request, InstanceType, getCore } from 'cvat-core-wrapper'; |
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range and nitpick comments (9)
cvat-ui/src/components/import-dataset/import-dataset-status-modal.tsx (2)
Line range hint
15-15: Replace globalparseIntwithNumber.parseIntfor better clarity and consistency.- setImportingId(parseInt(id, 10)); + setImportingId(Number.parseInt(id, 10));
Line range hint
1-1: Consider sorting import statements to improve code readability.+ import Alert from 'antd/lib/alert'; + import Modal from 'antd/lib/modal'; + import Progress from 'antd/lib/progress'; + import React, { useEffect, useState } from 'react'; + import { useSelector } from 'react-redux'; + import './styles.scss'; - import './styles.scss'; - import React, { useState, useEffect } from 'react'; - import { useSelector } from 'react-redux'; - import Modal from 'antd/lib/modal'; - import Alert from 'antd/lib/alert'; - import Progress from 'antd/lib/progress';cvat-ui/src/index.tsx (1)
Line range hint
1-1: Consider sorting import statements to improve code readability.+ import { BrowserRouter } from 'react-router-dom'; + import { connect, Provider } from 'react-redux'; + import React from 'react'; + import { createRoot } from 'react-dom/client'; - import React from 'react'; - import { createRoot } from 'react-dom/client'; - import { connect, Provider } from 'react-redux'; - import { BrowserRouter } from 'react-router-dom';cvat-ui/src/components/import-dataset/import-dataset-modal.tsx (1)
Line range hint
363-371: Consider using optional chaining to improve code robustness.- description: notificationState?.description && ( - <ReactMarkdown>{notificationState?.description}</ReactMarkdown> - ), + description: notificationState?.description ? ( + <ReactMarkdown>{notificationState.description}</ReactMarkdown> + ) : undefined,This modification ensures that the description is only rendered if
notificationState.descriptionis not null or undefined, preventing potential runtime errors.cvat-ui/src/components/cvat-app.tsx (1)
Line range hint
290-347: Use template literals for string concatenation to enhance readability.- const error = _error?.message || _error.toString(); + const error = `${_error?.message || _error.toString()}`;This change uses template literals instead of the logical OR operator for string operations, improving the clarity and maintainability of the code.
cvat/apps/engine/serializers.py (2)
Line range hint
727-727: Remove unnecessary f-string.- slogger.glob.warning("Failed with creating storage\n{}".format(str(ex))) + slogger.glob.warning(f"Failed with creating storage\n{str(ex)}")
Line range hint
1167-1167: Remove unnecessary f-string.- slogger.glob.error("The resource {} not found. It may have been deleted.".format(storage.name)) + slogger.glob.error(f"The resource {storage.name} not found. It may have been deleted.")cvat/schema.yml (2)
4284-4382: Ensure the JSON Logic filter description is clear and actionable.Consider enhancing the clarity and precision of the JSON Logic filter description to ensure it is easily understandable and directly actionable. Additionally, verify that the example URL is accessible and correct as it serves as a critical resource for understanding the filter's syntax.
5344-5344: Clarify the operation status retrieval method.The description mentions retrieving operation status via the
/api/requestsendpoint. Ensure this is clearly explained in the API documentation to avoid confusion.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- cvat-core/src/requests-manager.ts (1 hunks)
- cvat-core/src/server-proxy.ts (20 hunks)
- cvat-ui/package.json (1 hunks)
- cvat-ui/src/components/cvat-app.tsx (9 hunks)
- cvat-ui/src/components/export-backup/export-backup-modal.tsx (2 hunks)
- cvat-ui/src/components/export-dataset/export-dataset-modal.tsx (3 hunks)
- cvat-ui/src/components/header/header.tsx (1 hunks)
- cvat-ui/src/components/import-dataset/import-dataset-modal.tsx (2 hunks)
- cvat-ui/src/components/import-dataset/import-dataset-status-modal.tsx (1 hunks)
- cvat-ui/src/components/tasks-page/task-item.tsx (4 hunks)
- cvat-ui/src/index.tsx (5 hunks)
- cvat/apps/engine/serializers.py (3 hunks)
- cvat/schema.yml (7 hunks)
- tests/cypress/e2e/actions_tasks2/case_97_export_import_task.js (2 hunks)
- tests/cypress/support/commands.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- cvat-ui/package.json
Additional Context Used
Ruff (3)
cvat/apps/engine/serializers.py (3)
30-30:
cvat.apps.engine.rq_job_handler.RQIdManagerimported but unused
727-727: f-string without any placeholders
1167-1167: f-string without any placeholders
Biome (120)
cvat-core/src/requests-manager.ts (7)
189-189: Prefer for...of instead of forEach.
200-201: Prefer for...of instead of forEach.
204-207: Prefer for...of instead of forEach.
216-220: Prefer for...of instead of forEach.
9-9: All these imports are only used as types.
11-11: All these imports are only used as types.
Import statements could be sorted:
cvat-core/src/server-proxy.ts (20)
59-59: Unexpected any. Specify a different type.
97-97: Unexpected any. Specify a different type.
97-97: Unexpected any. Specify a different type.
98-102: Prefer for...of instead of forEach.
105-105: Unexpected any. Specify a different type.
105-105: Unexpected any. Specify a different type.
617-617: This variable implicitly has the any type.
657-657: Unexpected any. Specify a different type.
860-860: void is not valid as a constituent in a union type
844-878: This function expression can be turned into an arrow function.
943-943: void is not valid as a constituent in a union type
952-952: void is not valid as a constituent in a union type
981-981: This variable implicitly has the any type.
1020-1020: void is not valid as a constituent in a union type
1031-1031: void is not valid as a constituent in a union type
1060-1060: This variable implicitly has the any type.
1101-1101: Unexpected any. Specify a different type.
1122-1122: Avoid the delete operator which can impact performance.
1413-1431: This function expression can be turned into an arrow function.
1457-1457: Unexpected any. Specify a different type.
cvat-ui/src/components/cvat-app.tsx (15)
115-115: Unexpected any. Specify a different type.
153-153: Prefer for...of instead of forEach.
231-233: Template literals are preferred over string concatenation.
380-380: Unexpected any. Specify a different type.
381-381: Unexpected any. Specify a different type.
421-421: Unexpected any. Specify a different type.
422-422: Unexpected any. Specify a different type.
489-489: Change to an optional chain.
568-568: Provide screen reader accessible content when using
aelements.
568-568: Provide a href attribute for the a element.
9-9: Some named imports are only used as types.
66-66: Some named imports are only used as types.
67-69: All these imports are only used as types.
565-565: Avoid using the index of an array as key property in an element.
Import statements could be sorted:
cvat-ui/src/components/export-backup/export-backup-modal.tsx (5)
15-15: Some named imports are only used as types.
17-17: Some named imports are only used as types.
85-86: This hook does not specify all of its dependencies: closeModal
85-86: This hook does not specify all of its dependencies: dispatch
Import statements could be sorted:
cvat-ui/src/components/export-dataset/export-dataset-modal.tsx (7)
119-121: Template literals are preferred over string concatenation.
20-20: Some named imports are only used as types.
22-24: Some named imports are only used as types.
65-65: This hook does not specify all of its dependencies: form.setFieldsValue
101-102: This hook does not specify all of its dependencies: closeModal
101-102: This hook does not specify all of its dependencies: dispatch
Import statements could be sorted:
cvat-ui/src/components/header/header.tsx (14)
51-51: Unexpected any. Specify a different type.
64-64: Unexpected any. Specify a different type.
114-114: Unexpected any. Specify a different type.
289-289: Unexpected any. Specify a different type.
12-12: All these imports are only used as types.
37-37: Some named imports are only used as types.
44-44: All these imports are only used as types.
46-46: Some named imports are only used as types.
159-159: This hook does not specify all of its dependencies: isMounted
174-174: This hook does not specify all of its dependencies: isMounted
174-174: This hook does not specify all of its dependencies: searchCallback
236-236: Avoid using the index of an array as key property in an element.
276-276: This hook does not specify all of its dependencies: switchSettingsModalVisible
Import statements could be sorted:
cvat-ui/src/components/import-dataset/import-dataset-modal.tsx (20)
66-66: Unexpected any. Specify a different type.
99-99: Unexpected any. Specify a different type.
371-371: Unexpected any. Specify a different type.
456-456: Change to an optional chain.
467-469: Template literals are preferred over string concatenation.
563-563: Unexpected any. Specify a different type.
570-570: Unexpected any. Specify a different type.
570-570: Unexpected any. Specify a different type.
572-572: Unexpected any. Specify a different type.
578-578: Unexpected any. Specify a different type.
657-658: Unexpected any. Specify a different type.
659-660: Unexpected any. Specify a different type.
660-661: Unexpected any. Specify a different type.
8-8: The default import is only used as a type.
12-12: Some named imports are only used as types.
17-17: Some named imports are only used as types.
23-23: Some named imports are only used as types.
27-27: Some named imports are only used as types.
29-29: Some named imports are only used as types.
336-336: This hook does not specify all of its dependencies: isProject
cvat-ui/src/components/import-dataset/import-dataset-status-modal.tsx (3)
14-14: All these imports are only used as types.
22-22: Use Number.parseInt instead of the equivalent global.
Import statements could be sorted:
cvat-ui/src/components/tasks-page/task-item.tsx (8)
26-26: Unexpected any. Specify a different type.
289-289: Unexpected any. Specify a different type.
290-290: Unexpected any. Specify a different type.
8-8: All these imports are only used as types.
18-18: Some named imports are only used as types.
21-21: All these imports are only used as types.
306-306: Avoid using the index of an array as key property in an element.
Import statements could be sorted:
cvat-ui/src/index.tsx (4)
51-51: Unexpected any. Specify a different type.
114-114: Unexpected any. Specify a different type.
29-29: All these imports are only used as types.
Import statements could be sorted:
tests/cypress/e2e/actions_tasks2/case_97_export_import_task.js (1)
88-88: Use a regular expression literal instead of the RegExp constructor.
tests/cypress/support/commands.js (16)
104-119: Prefer for...of instead of forEach.
131-137: Prefer for...of instead of forEach.
170-170: This default parameter should follow the last required parameter or should be a required parameter.
171-171: This default parameter should follow the last required parameter or should be a required parameter.
172-172: This default parameter should follow the last required parameter or should be a required parameter.
173-173: This default parameter should follow the last required parameter or should be a required parameter.
174-174: This default parameter should follow the last required parameter or should be a required parameter.
177-177: This default parameter should follow the last required parameter or should be a required parameter.
178-178: This default parameter should follow the last required parameter or should be a required parameter.
241-245: Prefer for...of instead of forEach.
511-513: Prefer for...of instead of forEach.
600-602: Prefer for...of instead of forEach.
764-766: Prefer for...of instead of forEach.
917-917: Change to an optional chain.
1022-1029: Prefer for...of instead of forEach.
1042-1042: This aliasing of this is unnecessary.
Additional comments not posted (31)
cvat-ui/src/components/import-dataset/import-dataset-status-modal.tsx (1)
46-46: Ensure the modal remains open until the import is complete.cvat-ui/src/components/export-backup/export-backup-modal.tsx (2)
8-8: ImportReactMarkdownto enable markdown rendering in notifications.
103-108: Ensure error handling around notification logic to prevent silent failures.tests/cypress/e2e/actions_tasks2/case_97_export_import_task.js (2)
93-93: Ensure the export download function is called correctly.
116-117: Verify the status request returns a successful response.cvat-ui/src/index.tsx (3)
26-26: Ensure requests are fetched asynchronously upon application initialization.
74-74: Initialize request handling and server API schema fetching in the application's startup sequence.Also applies to: 125-125
55-56: Consider specifying a more specific type thananyfor user-related data to enhance type safety.- user: any; + user: UserType; // Assuming UserType is defined elsewhere in your projectAlso applies to: 105-105
Likely invalid or redundant comment.
cvat-ui/src/components/export-dataset/export-dataset-modal.tsx (3)
105-105: Ensure the instance type is correctly cast for dataset export.
22-22: Ensure all necessary types are imported for handling the export process.
24-24: ImportReactMarkdownto enable markdown rendering in notifications.cvat-core/src/requests-manager.ts (3)
188-188: Ensure that the Request object is treated as read-only to prevent unintended modifications.
244-244: Ensure that request cancellation is handled correctly and efficiently.
5-5: Ensure all necessary imports are used correctly and are necessary for the functionality of the RequestsManager.cvat-ui/src/components/tasks-page/task-item.tsx (4)
102-102: TherenderPreviewfunction looks good.
102-102: TherenderDescriptionfunction looks good.
163-173: TherenderProgressfunction looks good.
102-102: TherenderNavigationfunction looks good.cvat-ui/src/components/header/header.tsx (1)
476-487: TheHeaderComponentfunction looks good.cvat-ui/src/components/cvat-app.tsx (2)
62-63: Ensure theRequestsPagecomponent is properly integrated and tested.Given the addition of the
RequestsPagecomponent, it's crucial to verify its integration within the application's routing. This includes checking that the component renders correctly and interacts seamlessly with other parts of the application.
539-539: Add screen reader accessible content for the anchor element.- <a id='downloadAnchor' target='_blank' style={{ display: 'none' }} download /> + <a id='downloadAnchor' target='_blank' style={{ display: 'none' }} download aria-label="Download link" />It's important to provide accessible content for all interactive elements. This change adds an
aria-labelto the anchor element, making it accessible to screen readers.Likely invalid or redundant comment.
tests/cypress/support/commands.js (1)
1279-1282: Add success verification after navigation in thegoBackcommand.As previously noted, the
goBackcommand navigates back in the browser but does not verify if the navigation was successful. It might be beneficial to add a check to ensure that the page has navigated back to the expected URL or that certain elements expected on the previous page are present.+ cy.url().should('include', '/expected-url'); // Adjust the URL to match the expected onecvat-core/src/server-proxy.ts (6)
865-868: Consider refining the handling of HTTP status codes inexportDataset.
957-960: Consider refining the handling of HTTP status codes inbackupTask.
1036-1039: Consider refining the handling of HTTP status codes inbackupProject.
2224-2234: Enhance error messages ingetRequestsListfor better debugging and user feedback.
2248-2256: Enhance error messages incancelRequestfor better debugging and user feedback.
888-890: Refactor to use arrow functions for consistency and modern JavaScript practices.- uploadStatusCallback: (s: string, n: number) => void, + uploadStatusCallback: (status: string, number: number) => void,Likely invalid or redundant comment.
cvat/apps/engine/serializers.py (2)
30-30: Remove unused importRQIdManager.
2204-2209: TheUserIdentifiersSerializerclass is well-defined and adheres to the project's coding standards.cvat/schema.yml (1)
5570-5570: Confirm deprecation process.Ensure that the deprecation of the endpoint is communicated clearly in the SDK and documentation, and verify that no critical functionalities are affected by this change.
| Cypress.Commands.add('downloadExport', () => { | ||
| cy.verifyNotification(); | ||
| cy.get('.cvat-header-requests-button').click(); | ||
| cy.get('.cvat-spinner').should('not.exist'); | ||
| cy.get('.cvat-requests-list').should('be.visible'); | ||
| cy.get('.cvat-requests-card').first().within(() => { | ||
| cy.get('.cvat-requests-page-actions-button').click(); | ||
| }); | ||
| cy.get('.ant-dropdown') | ||
| .not('.ant-dropdown-hidden') | ||
| .within(() => { | ||
| cy.contains('[role="menuitem"]', 'Download').click(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Consider verifying the download success in the downloadExport command.
The downloadExport command navigates to the requests page and initiates a download but does not verify if the download was successful. It might be beneficial to add a check to ensure that the file was actually downloaded, which could help in catching issues with the download functionality early during automated tests.
+ cy.verifyDownloads(); // Ensure the file is downloaded successfullyCommittable suggestion was skipped due low confidence.
| from copy import copy | ||
| from inspect import isclass | ||
| import os | ||
| import re | ||
| import shutil | ||
| import string | ||
| import rq.defaults as rq_defaults |
There was a problem hiding this comment.
Remove unused import rq.defaults.
- import rq.defaults as rq_defaultsCommittable 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.
| import rq.defaults as rq_defaults |
| async function cancelRequest(requestID): Promise<void> { | ||
| const { backendAPI } = config; | ||
|
|
||
| try { | ||
| await Axios.post(`${backendAPI}/requests/${requestID}/cancel`); | ||
| } catch (errorData) { | ||
| throw generateError(errorData); | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor to use early return to reduce nesting and improve readability.
- try {
- await Axios.post(`${backendAPI}/requests/${requestID}/cancel`);
- } catch (errorData) {
- throw generateError(errorData);
- }
+ await Axios.post(`${backendAPI}/requests/${requestID}/cancel`).catch(errorData => {
+ throw generateError(errorData);
+ });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.
| async function cancelRequest(requestID): Promise<void> { | |
| const { backendAPI } = config; | |
| try { | |
| await Axios.post(`${backendAPI}/requests/${requestID}/cancel`); | |
| } catch (errorData) { | |
| throw generateError(errorData); | |
| } | |
| } | |
| async function cancelRequest(requestID): Promise<void> { | |
| const { backendAPI } = config; | |
| await Axios.post(`${backendAPI}/requests/${requestID}/cancel`).catch(errorData => { | |
| throw generateError(errorData); | |
| }); | |
| } |
| @@ -6,6 +6,7 @@ | |||
| import './styles.scss'; | |||
| import React, { useCallback, useEffect, useReducer } from 'react'; | |||
| import { connect, useDispatch } from 'react-redux'; | |||
| import ReactMarkdown from 'react-markdown'; | |||
There was a problem hiding this comment.
Specify a more appropriate type instead of any for selectedLoader.
Using any type reduces the benefits of TypeScript's static type checking. It would be beneficial to define a more specific type that describes the structure of selectedLoader.
| const uploadConfig = { | ||
| chunkSize: config.uploadChunkSize * 1024 * 1024, | ||
| endpoint: `${origin}${backendAPI}/projects/${id}/dataset/`, | ||
| totalSentSize: 0, | ||
| totalSize: (file as File).size, | ||
| onUpdate: (percentage) => { | ||
| options.updateStatusCallback('The dataset is being uploaded to the server', percentage); | ||
| options.uploadStatusCallback('The dataset is being uploaded to the server', percentage); |
There was a problem hiding this comment.
Use arrow function for consistency and modern JavaScript practices.
- onUpdate: (percentage) => {
+ onUpdate: percentage => {Committable suggestion was skipped due low confidence.
| PaginatedRequestList: | ||
| type: object | ||
| properties: | ||
| count: | ||
| type: integer | ||
| example: 123 | ||
| next: | ||
| type: string | ||
| nullable: true | ||
| format: uri | ||
| example: http://api.example.org/accounts/?page=4 | ||
| previous: | ||
| type: string | ||
| nullable: true | ||
| format: uri | ||
| example: http://api.example.org/accounts/?page=2 | ||
| results: | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/Request' |
There was a problem hiding this comment.
Review required fields in PaginatedRequestList schema.
The PaginatedRequestList schema includes properties such as count, next, previous, and results. Ensure that all necessary fields are included and correctly marked as required if they are essential for the functionality.
| const description = `${resToPrint} import was started for ${instanceType}.` + | ||
| ' You can check progress [here](/requests)'; | ||
| Notification.info({ | ||
| message: `${resToPrint} import started`, | ||
| description: `${resToPrint} import was started for ${instanceType}. `, | ||
| description: ( | ||
| <ReactMarkdown>{description}</ReactMarkdown> | ||
| ), |
There was a problem hiding this comment.
Use template literals for better readability and maintainability.
- const description = `${resToPrint} import was started for ${instanceType}.` +
- ' You can check progress [here](/requests)';
+ const description = `${resToPrint} import was started for ${instanceType}. You can check progress [here](/requests)`;This change consolidates the string into a single template literal, enhancing readability and reducing the chance of errors in future modifications.
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.
| const description = `${resToPrint} import was started for ${instanceType}.` + | |
| ' You can check progress [here](/requests)'; | |
| Notification.info({ | |
| message: `${resToPrint} import started`, | |
| description: `${resToPrint} import was started for ${instanceType}. `, | |
| description: ( | |
| <ReactMarkdown>{description}</ReactMarkdown> | |
| ), | |
| const description = `${resToPrint} import was started for ${instanceType}. You can check progress [here](/requests)`; | |
| Notification.info({ | |
| message: `${resToPrint} import started`, | |
| description: ( | |
| <ReactMarkdown>{description}</ReactMarkdown> | |
| ), |
| cy.deleteTask(taskName); | ||
| }); | ||
|
|
||
| it('Import the task. Check id, labels, shape.', () => { | ||
| cy.intercept({ method: /PATCH|POST/, url: /\/api\/tasks\/backup.*/ }).as('importTask'); | ||
| cy.intercept({ method: /GET/, url: /\/api\/requests.*/ }).as('requestStatus'); |
There was a problem hiding this comment.
Use regular expression literals instead of the RegExp constructor for consistency and performance.
- cy.contains('[role="menuitem"]', new RegExp('^Backup Task$')).click();
+ cy.contains('[role="menuitem"]', /^Backup Task$/).click();Committable suggestion was skipped due low confidence.
| const description = `${resource} export was started for ${instanceType}. ` + | ||
| 'You can check progress and download the file [here](/requests).'; | ||
| Notification.info({ | ||
| message: `${resource} export started`, | ||
| description: | ||
| `${resource} export was started for ${instanceType}. ` + | ||
| `Download will start automatically as soon as the ${resource} is ready.`, | ||
| description: ( | ||
| <ReactMarkdown>{description}</ReactMarkdown> | ||
| ), |
There was a problem hiding this comment.
Use template literals for constructing strings to improve readability and maintainability.
- const description = `${resource} export was started for ${instanceType}. ` +
- 'You can check progress and download the file [here](/requests).';
+ const description = `${resource} export was started for ${instanceType}. You can check progress and download the file [here](/requests).`;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.
| const description = `${resource} export was started for ${instanceType}. ` + | |
| 'You can check progress and download the file [here](/requests).'; | |
| Notification.info({ | |
| message: `${resource} export started`, | |
| description: | |
| `${resource} export was started for ${instanceType}. ` + | |
| `Download will start automatically as soon as the ${resource} is ready.`, | |
| description: ( | |
| <ReactMarkdown>{description}</ReactMarkdown> | |
| ), | |
| const description = `${resource} export was started for ${instanceType}. You can check progress and download the file [here](/requests).`; | |
| Notification.info({ | |
| message: `${resource} export started`, | |
| description: ( | |
| <ReactMarkdown>{description}</ReactMarkdown> | |
| ), |
| @@ -0,0 +1,303 @@ | |||
| // Copyright (C) 2024 CVAT.ai Corporation | |||
There was a problem hiding this comment.
Consider sorting import statements to improve code readability.
+ import { PaginatedResource } from './core-types';
+ import { RQStatus } from './enums';
+ import { RequestError } from './exceptions';
+ import { SerializedRequest } from './server-response-types';
+ import User from './user';
+ import serverProxy from './server-proxy';
- import serverProxy from './server-proxy';
- import { RQStatus } from './enums';
- import User from './user';
- import { SerializedRequest } from './server-response-types';
- import { RequestError } from './exceptions';
- import { PaginatedResource } from './core-types';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.
| // Copyright (C) 2024 CVAT.ai Corporation | |
| import { PaginatedResource } from './core-types'; | |
| import { RQStatus } from './enums'; | |
| import { RequestError } from './exceptions'; | |
| import { SerializedRequest } from './server-response-types'; | |
| import User from './user'; | |
| import serverProxy from './server-proxy'; |
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- cvat-ui/src/components/requests-page/request-card.tsx (1 hunks)
- cvat-ui/src/components/requests-page/styles.scss (1 hunks)
- cvat-ui/src/components/tasks-page/styles.scss (1 hunks)
Files skipped from review due to trivial changes (1)
- cvat-ui/src/components/tasks-page/styles.scss
Files skipped from review as they are similar to previous changes (1)
- cvat-ui/src/components/requests-page/styles.scss
Additional Context Used
Biome (4)
cvat-ui/src/components/requests-page/request-card.tsx (4)
6-6: The default import is only used as a type.
20-20: Some named imports are only used as types.
163-163: This hook does not specify all of its dependencies: dispatch
Import statements could be sorted:
| Download | ||
| </Menu.Item>, | ||
| ); | ||
| } | ||
| if (request.status === RQStatus.STARTED) { | ||
| newMenuItems.push( | ||
| <Menu.Item | ||
| key='cancel' | ||
| onClick={() => { | ||
| dispatch(cancelRequestAsync(request, () => { | ||
| setIsActive(false); | ||
| })); | ||
| }} | ||
| > | ||
| Cancel | ||
| </Menu.Item>, | ||
| ); | ||
| } | ||
|
|
||
| setMenuItems(newMenuItems); | ||
| }, [request]); | ||
|
|
||
| return ( | ||
| <Card className='cvat-requests-card' style={style}> | ||
| <Row justify='space-between'> | ||
| <Col span={12}> | ||
| <Row style={{ paddingBottom: [RQStatus.FAILED].includes(request.status) ? '10px' : '0' }}> | ||
| <Col className='cvat-requests-type' {...dimensions}> | ||
| <Text> | ||
| {type.split(':').map((word) => word.charAt(0).toUpperCase() + word.slice(1)).join(' ')} | ||
| {' '} | ||
| </Text> | ||
| </Col> | ||
| <Col className='cvat-requests-name'> | ||
| {linkToEntity ? | ||
| (<Link to={linkToEntity}>{name}</Link>) : | ||
| <Text>{name}</Text>} | ||
| </Col> | ||
| </Row> | ||
| {timestamps} | ||
| </Col> | ||
| <Col span={10} className='cvat-request-item-progress-wrapper'> | ||
| <Row> | ||
| <Col span={21}> | ||
| <Row /> | ||
| <StatusMessage message={request.message} status={request.status} /> | ||
| <Row> | ||
| <Col span={18} className='cvat-requests-progress'> | ||
| { | ||
| request.status !== RQStatus.FAILED ? ( | ||
| <Progress | ||
| percent={percent} | ||
| strokeColor={{ | ||
| from: '#108ee9', | ||
| to: '#87d068', | ||
| }} | ||
| showInfo={false} | ||
| strokeWidth={5} | ||
| size='small' | ||
| /> | ||
| ) : null | ||
| } | ||
| </Col> | ||
| <Col span={2} className='cvat-requests-percent'> | ||
| {percentProgress} | ||
| </Col> | ||
| </Row> | ||
| { | ||
| operation?.format ? ( | ||
| <Row> | ||
| <Col className='cvat-format-name'> | ||
| <Text type='secondary'>{operation.format}</Text> | ||
| </Col> | ||
| </Row> | ||
| ) : null | ||
| } | ||
| </Col> | ||
| <Col span={3} style={{ display: 'flex', justifyContent: 'end' }}> | ||
| { | ||
| menuItems.length !== 0 ? ( | ||
| <Dropdown | ||
| destroyPopupOnHide | ||
| trigger={['click']} | ||
| overlay={() => ( | ||
| <Menu selectable={false} className='cvat-actions-menu cvat-request-actions-menu'> | ||
| { menuItems.map((element) => element)} | ||
| </Menu> | ||
| )} | ||
| > | ||
| <Button type='link' size='middle' className='cvat-requests-page-actions-button' icon={<MoreOutlined className='cvat-menu-icon' />} /> | ||
| </Dropdown> | ||
| ) : null | ||
| } | ||
| </Col> | ||
| </Row> | ||
| </Col> | ||
| </Row> | ||
| </Card> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Refactor RequestCard for better readability and maintainability.
The RequestCard component is quite large and handles multiple responsibilities, such as constructing links, formatting timestamps, truncating names, and rendering the UI. Consider breaking it down into smaller components or hooks to improve readability and maintainability.
| import React, { useEffect, useState } from 'react'; | ||
| import { Link } from 'react-router-dom'; | ||
|
|
||
| import { Row, Col } from 'antd/lib/grid'; | ||
| import { useDispatch } from 'react-redux'; | ||
|
|
||
| import Card from 'antd/lib/card'; | ||
| import Text from 'antd/lib/typography/Text'; | ||
| import Progress from 'antd/lib/progress'; | ||
| import { MoreOutlined } from '@ant-design/icons'; | ||
| import Dropdown from 'antd/lib/dropdown'; | ||
| import Button from 'antd/lib/button'; | ||
| import Menu from 'antd/lib/menu'; | ||
|
|
||
| import { RQStatus, Request } from 'cvat-core-wrapper'; | ||
|
|
||
| import moment from 'moment'; | ||
| import { cancelRequestAsync } from 'actions/requests-async-actions'; | ||
| import { StatusMessage } from './request-status'; |
There was a problem hiding this comment.
Consider sorting import statements for better readability and maintainability.
- import React, { useEffect, useState } from 'react';
- import { Link } from 'react-router-dom';
- import { Row, Col } from 'antd/lib/grid';
- import { useDispatch } from 'react-redux';
- import Card from 'antd/lib/card';
- import Text from 'antd/lib/typography/Text';
- import Progress from 'antd/lib/progress';
- import { MoreOutlined } from '@ant-design/icons';
- import Dropdown from 'antd/lib/dropdown';
- import Button from 'antd/lib/button';
- import Menu from 'antd/lib/menu';
- import { RQStatus, Request } from 'cvat-core-wrapper';
- import moment from 'moment';
- import { cancelRequestAsync } from 'actions/requests-async-actions';
- import { StatusMessage } from './request-status';
+ import React, { useEffect, useState } from 'react';
+ import { Link } from 'react-router-dom';
+ import { useDispatch } from 'react-redux';
+ import { Row, Col } from 'antd/lib/grid';
+ import Card from 'antd/lib/card';
+ import Text from 'antd/lib/typography/Text';
+ import Progress from 'antd/lib/progress';
+ import { MoreOutlined } from '@ant-design/icons';
+ import Dropdown from 'antd/lib/dropdown';
+ import Button from 'antd/lib/button';
+ import Menu from 'antd/lib/menu';
+ import moment from 'moment';
+ import { RQStatus, Request } from 'cvat-core-wrapper';
+ import { cancelRequestAsync } from 'actions/requests-async-actions';
+ import { StatusMessage } from './request-status';Committable suggestion was skipped due low confidence.
| useEffect(() => { | ||
| const newMenuItems = []; | ||
| if (request.url) { | ||
| newMenuItems.push( | ||
| <Menu.Item | ||
| key='download' | ||
| onClick={() => { | ||
| setIsActive(false); | ||
| const downloadAnchor = window.document.getElementById('downloadAnchor') as HTMLAnchorElement; | ||
| downloadAnchor.href = request.url; | ||
| downloadAnchor.click(); | ||
| }} | ||
| > | ||
| Download | ||
| </Menu.Item>, | ||
| ); | ||
| } | ||
| if (request.status === RQStatus.STARTED) { | ||
| newMenuItems.push( | ||
| <Menu.Item | ||
| key='cancel' | ||
| onClick={() => { | ||
| dispatch(cancelRequestAsync(request, () => { | ||
| setIsActive(false); | ||
| })); | ||
| }} | ||
| > | ||
| Cancel | ||
| </Menu.Item>, | ||
| ); | ||
| } | ||
|
|
||
| setMenuItems(newMenuItems); | ||
| }, [request]); |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure all dependencies are specified in the useEffect hook in request-card.tsx.
The useEffect hook in request-card.tsx currently only lists request as a dependency. However, it also uses setIsActive and dispatch, which should be included in the dependency array to avoid potential issues with stale closures.
- File:
cvat-ui/src/components/requests-page/request-card.tsx - Lines: 162-195
Please update the useEffect dependency array to include setIsActive and dispatch.
Analysis chain
Ensure all dependencies are specified in the useEffect hook.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all dependencies are specified in the useEffect hook.
# Test: Search for the useEffect usage. Expect: All dependencies are specified.
rg --type typescript $'useEffect'
Length of output: 67
Script:
#!/bin/bash
# Description: Verify that all dependencies are specified in the useEffect hook.
# Test: Search for the useEffect usage in TypeScript files. Expect: All dependencies are specified.
rg --type-add 'tsx:*.{ts,tsx}' --type tsx 'useEffect'
Length of output: 25075
Motivation and context
How has this been tested?
Checklist
developbranch(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
RequestCardcomponent to display detailed request information.Enhancements
Configuration
requestTimeoutsetting to Cypress configuration for better request handling.Deprecations
/api/tasks/{id}/statusendpoint.