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: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat-ui/src/actions/requests-actions.ts (1 hunks)
Additional Context Used
Biome (5)
cvat-ui/src/actions/requests-actions.ts (5)
31-31: Unexpected any. Specify a different type.
40-40: Unexpected any. Specify a different type.
4-5: Some named imports are only used as types.
5-6: All these imports are only used as types.
6-7: Some named imports are only used as types.
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range and nitpick comments (11)
cvat-ui/src/containers/tasks-page/tasks-page.tsx (1)
Line range hint
6-7: Consider using type-only imports for type-only used imports.- import { TasksQuery, CombinedState } from 'reducers'; + import type { TasksQuery, CombinedState } from 'reducers';This change ensures that imports used only for type annotations do not contribute to the runtime bundle size.
cvat-ui/src/components/import-dataset/import-dataset-status-modal.tsx (1)
34-34: Visibility condition improvement.The visibility condition
open={!!importingId && progress < 100}is effective, but consider adding a comment explaining why the modal should not be visible when progress is 100% orimportingIdis not present, for clarity to other developers.cvat-ui/src/reducers/export-reducer.ts (1)
Line range hint
36-36: Default parameter placement issue.The default parameter
state: ExportState = defaultStateshould follow the last required parameter or should be a required parameter. Consider adjusting the function signature to comply with best practices.cvat-ui/src/components/projects-page/projects-page.tsx (1)
Line range hint
38-38: Missing dependencies inuseEffect.The
useEffecthooks on lines 38 and 43 do not specify all of their dependencies. This might lead to unexpected behavior if any of the omitted dependencies change. Consider addingdispatch,updatedQuery,history.replace, andisMountedto the dependency arrays.Also applies to: 43-43
cvat-ui/src/components/projects-page/actions-menu.tsx (1)
Line range hint
19-19: Specify a more appropriate type instead ofany.- interface Props { - projectInstance: any; + interface Props { + projectInstance: ProjectType; // Assuming ProjectType is the correct type }Replacing
anywith a more specific type enhances type safety and code maintainability.cvat-ui/src/components/tasks-page/tasks-page.tsx (2)
Line range hint
47-47: Ensure all dependencies are specified in the useEffect hook.- useEffect(() => { - dispatch(getTasksAsync({ ...updatedQuery })); - setIsMounted(true); - }, []); + useEffect(() => { + dispatch(getTasksAsync({ ...updatedQuery })); + setIsMounted(true); + }, [dispatch, updatedQuery]); // Added missing dependenciesThis change ensures that the effect correctly responds to changes in its dependencies, preventing potential bugs in your application's state management.
Line range hint
13-14: Consider using type-only imports for these utility imports.- import { useDispatch } from 'react-redux'; - import React, { useEffect, useState } from 'react'; - import { useHistory } from 'react-router'; + import type { useDispatch } from 'react-redux'; + import type React, { useEffect, useState } from 'react'; + import type { useHistory } from 'react-';This change ensures that these imports are not included in the JavaScript bundle since they are only used for type checking, which can help in reducing the bundle size.
cvat-ui/src/components/actions-menu/actions-menu.tsx (1)
Line range hint
50-50: Include all dependencies in the useCallback hook to avoid stale closures.- const onClickMenuWrapper = useCallback( - (params: MenuInfo) => { - if (!params) { - return; - } - - if (params.key === Actions.DELETE_TASK) { - Modal.confirm({ - title: `The task ${taskID} will be deleted`, - content: 'All related data (images, annotations) will be lost. Continue?', - className: 'cvat-modal-confirm-delete-task', - onOk: () => { - onClickMenu(params); - }, - okButtonProps: { - type: 'primary', - danger: true, - }, - okText: 'Delete', - }); - } else { - onClickMenu(params); - } - }, - [taskID], - ); + const onClickMenuWrapper = useCallback( + (params: MenuInfo) => { + if (!params) { + return; + } + + if (params.key === Actions.DELETE_TASK) { + Modal.confirm({ + title: `The task ${taskID} will be deleted`, + content: 'All related data (images, annotations) will be lost. Continue?', + className: 'cvat-modal-confirm-delete-task', + onOk: () => { + onClickMenu(params); + }, + okButtonProps: { + type: 'primary', + danger: true, + }, + okText: 'Delete', + }); + } else { + onClickMenu(params); + } + }, + [taskID, onClickMenu], // Added missing dependency + );This change ensures that the callback has the correct references to the dependencies, preventing potential bugs related to stale closures.
cvat-ui/src/reducers/index.ts (2)
11-11: Add a comment explaining the purpose of theRequestimport.Adding a brief comment above the import statement explaining its usage can improve code readability and maintainability.
Line range hint
40-40: Refactor to avoid usinganytype for better type safety.- export type Project = any; + export interface Project { + id: number; + name: string; + // Add other necessary fields + }Using
anytype reduces the benefits of TypeScript's type system. Defining a more specific type or interface forProjectwould enhance type safety and code maintainability.Also applies to: 72-72, 119-119, 122-122, 127-127, 130-130, 135-135, 144-144, 158-158, 172-172, 185-185, 204-204, 234-234, 251-251, 323-323, 413-413, 722-722, 731-731, 751-751
cvat-ui/src/reducers/notifications-reducer.ts (1)
Line range hint
224-224: This default parameter should follow the last required parameter or should be a required parameter.Please adjust the default parameter to ensure it follows the last required parameter or make it a required parameter to avoid potential runtime errors.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- cvat-ui/src/actions/export-actions.ts (3 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/actions-menu/actions-menu.tsx (4 hunks)
- cvat-ui/src/components/export-dataset/export-dataset-modal.tsx (8 hunks)
- cvat-ui/src/components/import-dataset/import-dataset-modal.tsx (7 hunks)
- cvat-ui/src/components/import-dataset/import-dataset-status-modal.tsx (2 hunks)
- cvat-ui/src/components/projects-page/actions-menu.tsx (3 hunks)
- cvat-ui/src/components/projects-page/projects-page.tsx (2 hunks)
- cvat-ui/src/components/projects-page/top-bar.tsx (3 hunks)
- cvat-ui/src/components/tasks-page/tasks-page.tsx (2 hunks)
- cvat-ui/src/components/tasks-page/top-bar.tsx (3 hunks)
- cvat-ui/src/containers/actions-menu/actions-menu.tsx (4 hunks)
- cvat-ui/src/containers/tasks-page/tasks-page.tsx (2 hunks)
- cvat-ui/src/reducers/export-reducer.ts (3 hunks)
- cvat-ui/src/reducers/import-reducer.ts (5 hunks)
- cvat-ui/src/reducers/index.ts (7 hunks)
- cvat-ui/src/reducers/notifications-reducer.ts (94 hunks)
Files not reviewed due to errors (3)
- cvat-ui/src/actions/requests-async-actions.ts (no review received)
- cvat-ui/src/components/projects-page/top-bar.tsx (no review received)
- cvat-ui/src/reducers/import-reducer.ts (no review received)
Additional Context Used
Biome (124)
cvat-ui/src/actions/export-actions.ts (9)
30-30: Unexpected any. Specify a different type.
33-33: Unexpected any. Specify a different type.
56-56: Unexpected any. Specify a different type.
66-66: Unexpected any. Specify a different type.
69-69: Unexpected any. Specify a different type.
75-75: Unexpected any. Specify a different type.
5-6: Some named imports are only used as types.
7-10: All these imports are only used as types.
10-14: Some named imports are only used as types.
cvat-ui/src/actions/import-actions.ts (6)
47-47: Unexpected any. Specify a different type.
66-66: Unexpected any. Specify a different type.
5-6: Some named imports are only used as types.
6-7: All these imports are only used as types.
7-10: Some named imports are only used as types.
13-17: Some named imports are only used as types.
cvat-ui/src/actions/requests-actions.ts (5)
33-33: Unexpected any. Specify a different type.
42-42: Unexpected any. Specify a different type.
4-5: Some named imports are only used as types.
5-6: All these imports are only used as types.
6-9: Some named imports are only used as types.
cvat-ui/src/actions/requests-async-actions.ts (5)
32-79: Prefer for...of instead of forEach.
4-5: All these imports are only used as types.
5-6: All these imports are only used as types.
6-9: Some named imports are only used as types.
10-11: Some named imports are only used as types.
cvat-ui/src/components/actions-menu/actions-menu.tsx (4)
8-9: All these imports are only used as types.
9-10: Some named imports are only used as types.
11-12: All these imports are only used as types.
50-50: This hook does not specify all of its dependencies: onClickMenu
cvat-ui/src/components/export-dataset/export-dataset-modal.tsx (6)
117-118: Template literals are preferred over string concatenation.
18-19: Some named imports are only used as types.
20-23: Some named imports are only used as types.
63-63: This hook does not specify all of its dependencies: form.setFieldsValue
99-99: This hook does not specify all of its dependencies: closeModal
99-99: This hook does not specify all of its dependencies: dispatch
cvat-ui/src/components/import-dataset/import-dataset-modal.tsx (20)
65-65: Unexpected any. Specify a different type.
98-98: Unexpected any. Specify a different type.
369-369: Unexpected any. Specify a different type.
454-454: Change to an optional chain.
465-466: Template literals are preferred over string concatenation.
561-561: Unexpected any. Specify a different type.
568-568: Unexpected any. Specify a different type.
568-568: Unexpected any. Specify a different type.
570-570: Unexpected any. Specify a different type.
576-576: Unexpected any. Specify a different type.
649-649: Unexpected any. Specify a different type.
651-651: Unexpected any. Specify a different type.
6-7: The default import is only used as a type.
10-11: Some named imports are only used as types.
15-16: Some named imports are only used as types.
21-22: Some named imports are only used as types.
25-26: Some named imports are only used as types.
27-28: Some named imports are only used as types.
334-334: This hook does not specify all of its dependencies: isProject
334-334: This hook does not specify all of its dependencies: isTask
cvat-ui/src/components/import-dataset/import-dataset-status-modal.tsx (1)
12-13: All these imports are only used as types.
cvat-ui/src/components/projects-page/actions-menu.tsx (5)
19-19: Unexpected any. Specify a different type.
8-9: All these imports are only used as types.
29-29: This hook does not specify all of its dependencies: dispatch
29-29: This hook does not specify all of its dependencies: projectInstance
29-29: This hook does not specify all of its dependencies: projectInstance.id
cvat-ui/src/components/projects-page/projects-page.tsx (5)
10-11: All these imports are only used as types.
38-38: This hook does not specify all of its dependencies: dispatch
38-38: This hook does not specify all of its dependencies: updatedQuery
43-43: This hook does not specify all of its dependencies: history.replace
43-43: This hook does not specify all of its dependencies: isMounted
cvat-ui/src/components/projects-page/top-bar.tsx (1)
14-15: All these imports are only used as types.
cvat-ui/src/components/tasks-page/tasks-page.tsx (5)
13-14: All these imports are only used as types.
47-47: This hook does not specify all of its dependencies: dispatch
47-47: This hook does not specify all of its dependencies: updatedQuery
52-52: This hook does not specify all of its dependencies: history.replace
52-52: This hook does not specify all of its dependencies: isMounted
cvat-ui/src/components/tasks-page/top-bar.tsx (1)
16-17: All these imports are only used as types.
cvat-ui/src/containers/actions-menu/actions-menu.tsx (15)
22-22: Unexpected any. Specify a different type.
27-27: Unexpected any. Specify a different type.
32-32: Unexpected any. Specify a different type.
33-33: Unexpected any. Specify a different type.
34-34: Unexpected any. Specify a different type.
35-35: Unexpected any. Specify a different type.
36-36: Unexpected any. Specify a different type.
54-54: Unexpected any. Specify a different type.
56-56: Unexpected any. Specify a different type.
63-63: Unexpected any. Specify a different type.
66-66: Unexpected any. Specify a different type.
69-69: Unexpected any. Specify a different type.
90-90: void is not valid as a constituent in a union type
8-9: All these imports are only used as types.
10-11: All these imports are only used as types.
cvat-ui/src/containers/tasks-page/tasks-page.tsx (1)
6-7: All these imports are only used as types.
cvat-ui/src/reducers/export-reducer.ts (3)
36-36: This default parameter should follow the last required parameter or should be a required parameter.
5-6: Some named imports are only used as types.
7-8: All these imports are only used as types.
cvat-ui/src/reducers/import-reducer.ts (5)
60-60: This default parameter should follow the last required parameter or should be a required parameter.
5-6: Some named imports are only used as types.
6-7: Some named imports are only used as types.
7-8: All these imports are only used as types.
8-9: All these imports are only used as types.
cvat-ui/src/reducers/index.ts (20)
40-40: Unexpected any. Specify a different type.
72-72: Unexpected any. Specify a different type.
119-119: Unexpected any. Specify a different type.
122-122: Unexpected any. Specify a different type.
127-127: Unexpected any. Specify a different type.
130-130: Unexpected any. Specify a different type.
135-135: Unexpected any. Specify a different type.
144-144: Unexpected any. Specify a different type.
158-158: Unexpected any. Specify a different type.
172-172: Unexpected any. Specify a different type.
185-185: Unexpected any. Specify a different type.
204-204: Unexpected any. Specify a different type.
234-234: Unexpected any. Specify a different type.
251-251: Unexpected any. Specify a different type.
323-323: Unexpected any. Specify a different type.
413-413: Unexpected any. Specify a different type.
673-673: This enum declaration contains members that are implicitly initialized.
722-722: Unexpected any. Specify a different type.
731-731: Unexpected any. Specify a different type.
751-751: Unexpected any. Specify a different type.
cvat-ui/src/reducers/notifications-reducer.ts (7)
224-224: This default parameter should follow the last required parameter or should be a required parameter.
529-530: Template literals are preferred over string concatenation.
640-642: Template literals are preferred over string concatenation.
653-655: Template literals are preferred over string concatenation.
841-843: Template literals are preferred over string concatenation.
5-6: All these imports are only used as types.
30-31: All these imports are only used as types.
Additional comments not posted (8)
cvat-ui/src/components/import-dataset/import-dataset-status-modal.tsx (1)
16-16: Ensure proper handling whenimportingIdisnull.Consider adding a check or default value for
importingIdto handle cases where it might benull, ensuring the UI behaves predictably.cvat-ui/src/components/actions-menu/actions-menu.tsx (1)
8-9: Use type-only imports for these utility imports to reduce bundle size.- import React, { useCallback } from 'react'; - import Modal from 'antd/lib/modal'; + import type React, { useCallback } from 'react'; + import type Modal from 'antd/lib/modal';This change ensures that these imports are not included in the JavaScript bundle since they are only used for type checking, which can help in reducing the bundle size.
Likely invalid or redundant comment.
cvat-ui/src/components/tasks-page/top-bar.tsx (2)
39-39: Consider using destructuring directly in the function parameter to simplify the code.
[REFACTOR_SUGGESTion]- const { query, onApplyFilter, onApplySorting, onApplySearch } = props; + export default function TopBarComponent({ query, onApplyFilter, onApplySorting, onApplySearch }: VisibleTopBarProps): JSX.Element {
6-6: Ensure proper cleanup of state and effects if using hooks.cvat-ui/src/actions/import-actions.ts (1)
9-17: Use type-only imports for types that are only used for type annotations to optimize bundle size.- import { getCore, Storage, Job, Task, Project, InstanceType } from 'cvat-core-wrapper'; + import type { Storage, Job, Task, Project, InstanceType } from 'cvat-core-wrapper'; + import { getCore } from 'cvat-core-wrapper';Skipped due to learnings
User: klakhov PR: cvat-ai/cvat#7537 File: cvat-ui/src/actions/import-actions.ts:9-16 Timestamp: 2024-05-28T10:29:32.384Z Learning: For the CVAT project, do not suggest using type-only imports as per user preference.cvat-ui/src/reducers/notifications-reducer.ts (3)
33-35: The functionshouldLogis a good approach to centralize the logging decision logic. Ensure that it is used consistently across all error handling cases.
529-530: Use template literals for string concatenation to improve readability and maintainability.
[REFACTOR_SUGGESTion]- 'Could not export dataset for the ' + `[${instanceType} ${instance.id}](/${instanceType}s/${instance.id})` + `Could not export dataset for the [${instanceType} ${instance.id}](/${instanceType}s/${instance.id})`Also applies to: 640-642, 653-655, 841-843
8-8: Consider consolidating imports from 'cvat-core-wrapper' to improve readability and maintainability.- import { ServerError, RequestError } from 'cvat-core-wrapper'; + import { ServerError, RequestError, Storage, Request } from 'cvat-core-wrapper';Likely invalid or redundant comment.
Actions PerformedReviews paused. |
…into kl/data-processing
|
Something does not work when I am testing the feature with organization. I see three problems:
@Marishka17 they are probably server-side issues. |
| const updatedQuery = { ...query }; | ||
| const queryParams = new URLSearchParams(history.location.search); | ||
| for (const key of Object.keys(updatedQuery)) { | ||
| (updatedQuery as Indexable)[key] = queryParams.get(key) || null; | ||
| if (key === 'page') { | ||
| updatedQuery.page = updatedQuery.page ? +updatedQuery.page : 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
We only have one query parameter, the code may be significantly simpler
There was a problem hiding this comment.
Yes but its our general approach. We do the same on other pages, I dont see the reason not to use it here. We can add another params later
| .cvat-requests-scrollbar { | ||
| &::-webkit-scrollbar { | ||
| background-color: #fff; | ||
| width: 16px; | ||
| } | ||
|
|
||
| &::-webkit-scrollbar-track { | ||
| background-color: #fff; | ||
| } | ||
|
|
||
| &::-webkit-scrollbar-thumb { | ||
| background-color: #babac0; | ||
| border-radius: 16px; | ||
| border: 6px solid #fff; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Why did you decide to use vendor-specific css selectors?
There was a problem hiding this comment.
I believe we cant make pretty scrollbar with standard selectors. It works fine in chrome as its our main supported browser
| dispatch(getRequestsAsync({ | ||
| ...query, | ||
| page: newPage, | ||
| })); |
There was a problem hiding this comment.
serverProxy:getRequestsAsync uses fetchAll. By the way, why???
If it already fetched all list, why do you call getRequestsAsync?
There was a problem hiding this comment.
Removed that call.
Dont we need to check for all requests status? There can be more than 10 in progress. We want notifications for those when they are completed
| @@ -495,3 +495,26 @@ export interface SerializedAPISchema { | |||
| url: string; | |||
| }; | |||
| } | |||
|
|
|||
| export interface SerializedRequest { | |||
| id?: string; | |||
There was a problem hiding this comment.
why having id is not guaranteed?
tha same about created_date and owner
There was a problem hiding this comment.
Yes. When we create task, we use the same request callback, so we create fake Request instances. We dont have an ID while uploading data, also it wouldnt be pretty to construct a created_date and owner here.
|
|
I already mentioned this, but did not get answer. Why is TTL 20 minutes for exports??? |
|
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
Release Notes
New Features
Bug Fixes
cy.goBack()after export operations.Refactor
importingandbackupIsActiveacross multiple components.Tests
Documentation
Chores