Requests page #7537
Requests page #7537
Conversation
![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.
| 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.
| 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.
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:
| 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
|
Minor bug:
But export was local UPD: the same for annotations and probable for datasets export |
|
It looks like the feature to delete requests absent in the implementation
the method |
| } | ||
|
|
||
| .cvat-requests-card { | ||
| height: $grid-unit-size * 11; |
There was a problem hiding this comment.
96px looks better
| height: $grid-unit-size * 11; | |
| height: $grid-unit-size * 12; |
| export function defineActititiesField(instance: any): 'projects' | 'tasks' | 'jobs' { | ||
| if (instance instanceof core.classes.Project) { | ||
| export function defineActititiesField(instance: InstanceType | RequestInstanceType): 'projects' | 'tasks' | 'jobs' { | ||
| const instanceType = getInstanceType(instance); |
There was a problem hiding this comment.
return `${getInstanceType(instance)}s`;
| uploadStatusCallback: (status: RQStatus, progress: number, message: string): void => { | ||
| if (status === RQStatus.UNKNOWN) { | ||
| onProgress?.(`${message} ${progress ? `${Math.floor(progress * 100)}%` : ''}`); | ||
| } else if ([RQStatus.QUEUED, RQStatus.STARTED].includes(status)) { | ||
| const helperMessage = 'You may close the window.'; | ||
| onProgress?.(`${message} ${progress ? `${Math.floor(progress * 100)}%` : ''}. ${helperMessage}`); | ||
| } else { | ||
| onProgress?.(`${status}: ${message}`); | ||
| } | ||
| }, | ||
| requestStatusCallback(request) { | ||
| let { message } = request; | ||
| let helperMessage = ''; | ||
| const { status, progress } = request; | ||
| if (!message) { | ||
| if ([RQStatus.QUEUED, RQStatus.STARTED].includes(status)) { | ||
| message = 'CVAT queued the task to import'; | ||
| helperMessage = 'You may close the window.'; | ||
| } else if (status === RQStatus.FAILED) { | ||
| message = 'Images processing failed'; | ||
| } else if (status === RQStatus.FINISHED) { | ||
| message = 'Task creation finished'; | ||
| } else { | ||
| message = 'Unknown status received'; | ||
| } | ||
| } |
There was a problem hiding this comment.
What is the difference between these two callbacks?
Do we really need both? They look very similar
| import { | ||
| CombinedState, | ||
| RequestsQuery, StorageLocation, | ||
| } from 'reducers'; |
There was a problem hiding this comment.
| import { | |
| CombinedState, | |
| RequestsQuery, StorageLocation, | |
| } from 'reducers'; | |
| import { CombinedState, RequestsQuery, StorageLocation } from 'reducers'; |
| dispatch(requestsActions.getRequestsSuccess(requests)); | ||
|
|
||
| if (notify) { | ||
| const shownNotifications = JSON.parse(localStorage.getItem('requestsNotifications') || '[]'); |
There was a problem hiding this comment.
- Parsing item from local storage should be try catched, otherwise now it will get into global try catch and show "get requests failed message". That is not true.
- Local storage is limited by 5 MB, we will quickly make it full with big number of request (as I see you only append something inside. But never remove. So, gradually it will be full)
- Finally, I believe we may totally omit this logic. Thus, a user will only see notification if the process finished when the page is opened. I believe it is enough.
| dispatch(exportBackupAsync( | ||
| instance as RequestInstanceType, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| listeningPromise, | ||
| )); | ||
| } | ||
| } else if (!(state.export.tasks.dataset.current?.[(instance as RequestInstanceType).id] || | ||
| state.export.projects.dataset.current?.[(instance as RequestInstanceType).id] || | ||
| state.export.jobs.dataset.current?.[(instance as RequestInstanceType).id]) | ||
| ) { | ||
| dispatch(exportDatasetAsync( | ||
| instance as RequestInstanceType, | ||
| format, | ||
| operationTarget === 'dataset', | ||
| true, | ||
| undefined, | ||
| undefined, | ||
| listeningPromise, | ||
| )); | ||
| } | ||
| } else if (operationType === 'import') { | ||
| if (!(state.import.tasks.dataset.current?.[(instance as RequestInstanceType).id] || | ||
| state.import.projects.dataset.current?.[(instance as RequestInstanceType).id] || | ||
| state.import.jobs.dataset.current?.[(instance as RequestInstanceType).id]) | ||
| ) { | ||
| dispatch(importDatasetAsync( | ||
| instance as RequestInstanceType, | ||
| format, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| listeningPromise, | ||
| )); |
There was a problem hiding this comment.
May you please explain logic behind this?
It makes signature and implemenetion of exportBackupAsync, exportDatasetAsync, importDatasetAsync more difficult, but I can't see the reason.
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.