Fix UI crush on failed request status check #8575
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes involve significant updates across multiple files in the CVAT codebase, enhancing type definitions, request management, and error handling. Key modifications include renaming types, adding new properties to interfaces, and refining method signatures to improve the handling of initial requests. The Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant RequestsManager
participant Server
UI->>RequestsManager: listen(requestID, options)
RequestsManager->>Server: send initialRequest
Server-->>RequestsManager: response
RequestsManager->>UI: update based on response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
cvat-core/src/request.ts (1)
19-23: Consider using a more specific type for the 'owner' propertyThe addition of the
RequestInitialDatatype is a good improvement for type safety. However, theownerproperty is currently typed asany, which might be too permissive and could lead to potential type-related issues.Consider using a more specific type for the
ownerproperty. If it's supposed to be a user object, you could use theUsertype or create a specific type for the initial user data. For example:owner: { id: number; username: string; // Add other relevant properties } | null;This would provide better type checking and documentation for the expected structure of the owner data.
cvat-ui/src/cvat-core-wrapper.ts (1)
32-32: LGTM: New utility function imported correctly.The import of
fieldsToSnakeCaseis correct and aligns with the changes described in the summary.Consider grouping similar imports together for better code organization. You might want to move this import closer to other utility function imports if there are any.
cvat-core/src/requests-manager.ts (1)
133-138: Improved error handling with additional context.The enhanced error object now includes more detailed information from
initialRequest, which is beneficial for debugging and logging. However, consider extracting this error object creation into a separate method for better readability and reusability.Consider refactoring the error object creation into a separate method:
private createErrorRequest(requestID: string, error: Error, initialRequest: Request): Request { return new Request({ id: requestID, status: RQStatus.FAILED, message: `Could not get a status of the request ${requestID}. ${error.toString()}`, operation: fieldsToSnakeCase(initialRequest.operation) as SerializedRequest['operation'], created_date: initialRequest.createdDate, owner: { id: initialRequest.owner.id, username: initialRequest.owner.username, }, }); }Then use it in the catch block:
catch (error) { if (requestID in this.listening) { const { onUpdate } = this.listening[requestID]; const errorRequest = this.createErrorRequest(requestID, error, initialRequest); onUpdate.forEach((update) => update(errorRequest)); reject(error); } }cvat-ui/src/actions/tasks-actions.ts (1)
16-16: LGTM! Consider adding a comment for clarity.The addition of
generateInitialRequestand the newgetInitialRequestoption intaskInstance.savemethod improves the request handling process for task creation. This change aligns well with the overall enhancements in request management across the codebase.Consider adding a brief comment above the
getInitialRequestfunction to explain its purpose, e.g.:// Generate initial request object for task creation tracking getInitialRequest(taskID) { // ... (existing code) }Also applies to: 296-302
cvat-ui/src/components/requests-page/request-card.tsx (1)
103-111: Improved handling of failed request timestamps. Consider using early return for readability.The changes enhance the clarity of status messages for failed requests by differentiating between requests that started but failed and those that failed before starting. This improvement aligns well with the PR objective of fixing UI issues related to failed request status checks.
Consider refactoring the code to use early returns for improved readability:
if (request.startedDate) { return ( <Row> <Text type='secondary'>{`Started by ${request.owner.username} on ${started}`}</Text> </Row> ); } return ( <Row> <Text type='secondary'>{`Enqueued by ${request.owner.username} on ${created}`}</Text> </Row> );This structure eliminates the need for the ternary operator and makes the code easier to read and maintain.
cvat-core/src/server-response-types.ts (1)
508-515: LGTM! Consider adding JSDoc comments for clarity.The changes to the
SerializedRequestinterface improve type safety and flexibility. Makingoperationrequired ensures all requests have an operation defined, while allowingformatto benullaccommodates cases where a format might not be applicable. The newfunction_idproperty adds useful tracking capabilities.Consider adding JSDoc comments to explain the purpose of the new
function_idproperty and whenformatmight benull. This would enhance code readability and maintainability. For example:operation: { // ... other properties ... /** * The format of the operation, or null if not applicable. */ format: string | null; // ... other properties ... /** * Identifier for the specific function associated with this request. * Used for tracking or processing purposes. */ function_id: string | null; };cvat-ui/src/actions/requests-actions.ts (1)
14-20: Consider Thread Safety for Store InitializationThe lazy initialization of the
storevariable within thegetStore()function is suitable for a single-threaded environment. However, if the application runs in a context where multiple asynchronous operations might accessgetStore()simultaneously, there's a potential for race conditions.Although JavaScript environments are mostly single-threaded, consider adding a mutex or other synchronization mechanism if there's a possibility of concurrent access to ensure thread safety.
cvat-ui/src/actions/import-actions.ts (1)
Line range hint
131-149: Refactor to reduce code duplication inimportDatasetAsyncThe code blocks handling the 'project', 'task', and 'job' instances share similar logic for importing datasets and listening for progress. Refactoring these sections by creating common helper functions or unifying the control flow can reduce code duplication and enhance maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- cvat-core/src/request.ts (3 hunks)
- cvat-core/src/requests-manager.ts (3 hunks)
- cvat-core/src/server-response-types.ts (1 hunks)
- cvat-core/src/session-implementation.ts (1 hunks)
- cvat-core/src/session.ts (1 hunks)
- cvat-ui/src/actions/export-actions.ts (3 hunks)
- cvat-ui/src/actions/import-actions.ts (7 hunks)
- cvat-ui/src/actions/requests-actions.ts (2 hunks)
- cvat-ui/src/actions/requests-async-actions.ts (2 hunks)
- cvat-ui/src/actions/tasks-actions.ts (2 hunks)
- cvat-ui/src/components/requests-page/request-card.tsx (1 hunks)
- cvat-ui/src/cvat-core-wrapper.ts (5 hunks)
🧰 Additional context used
🔇 Additional comments (25)
cvat-core/src/request.ts (4)
9-12: LGTM: Improved type definition for RequestOperationThe changes to the
RequestOperationtype enhance clarity and flexibility:
- Renaming from
OperationtoRequestOperationimproves type naming consistency.- Allowing
nullfor theformatproperty accommodates operations without a specific format.These modifications align well with the PR's objective of improving request handling.
53-56: LGTM: Improved owner initialization in Request constructorThe changes to the
Requestconstructor enhance flexibility and error prevention:
- Initializing
#ownertonullby default allows for requests without an associated owner.- Creating a new
Userinstance only wheninitialData.owneris provided prevents potential errors.These modifications align well with the PR's objective of improving request handling and preventing UI crashes.
Line range hint
75-84: LGTM: Updated operation getter for consistencyThe changes to the
operationgetter in theRequestclass ensure consistency with the updatedRequestOperationtype:
- The return type is now correctly specified as
RequestOperation.- All properties are properly mapped, including the newly added ones like
functionID.These modifications improve type safety and maintain consistency across the codebase.
Line range hint
1-124: Overall assessment: Improvements in type safety and request handlingThe changes in this file significantly enhance the request handling mechanism:
- Improved type definitions with
RequestOperationandRequestInitialData.- More flexible handling of the
ownerproperty in theRequestclass.- Consistent updates to the
operationgetter.These modifications align well with the PR's objective of fixing UI crashes on failed request status checks by improving the robustness of request handling.
One minor suggestion was made regarding the
ownerproperty type inRequestInitialData, which could further improve type safety.Great job on these improvements!
cvat-ui/src/cvat-core-wrapper.ts (4)
18-18: LGTM: Import statement updated correctly.The addition of
SerializedRequestto the import statement is consistent with the other imports and aligns with the changes described in the summary.
45-45: LGTM: Import statement updated correctly.The addition of
RequestOperationto the import statement is consistent with the existing import and aligns with the changes described in the summary.
110-110: LGTM: Export statements updated correctly.The additions of
fieldsToSnakeCase,RequestOperation, andSerializedRequestto the export statements are consistent with the new imports and align with the changes described in the summary.Also applies to: 125-126
18-18: Summary: Import and export statements updated to support enhanced type definitions and request handling.The changes in this file are consistent and support the broader refactoring effort described in the PR summary. All new imports and exports are correctly placed and align with the described changes.
To ensure these changes are properly integrated, please run the following script to verify the usage of the new exports in other parts of the codebase:
This will help confirm that the new exports are being used as intended throughout the project.
Also applies to: 32-32, 45-45, 110-110, 125-126
✅ Verification successful
Verification Successful: New exports are consistently utilized across the codebase.
All new exports (
fieldsToSnakeCase,RequestOperation,SerializedRequest) are properly referenced in relevant modules, ensuring their integration and functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of new exports in the codebase # Test: Search for usage of fieldsToSnakeCase echo "Checking usage of fieldsToSnakeCase:" rg "fieldsToSnakeCase" --type ts # Test: Search for usage of RequestOperation echo "Checking usage of RequestOperation:" rg "RequestOperation" --type ts # Test: Search for usage of SerializedRequest echo "Checking usage of SerializedRequest:" rg "SerializedRequest" --type tsLength of output: 3299
cvat-ui/src/actions/requests-async-actions.ts (3)
11-13: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include
generateInitialRequest. This change is consistent with its usage in the file and aligns with the overall improvements in request initialization.
81-86: Approved: Enhanced request initialization for task creation.The changes improve the initialization of requests for task creation by using
generateInitialRequest. This is consistent with the overall improvements in request handling mentioned in the summary.However, could you please clarify how these changes specifically address the UI crash issue mentioned in the PR objectives? It would be helpful to understand the connection between this enhancement and the reported problem.
To verify the impact of these changes, please run the following script:
#!/bin/bash # Description: Check for UI-related error handling in the codebase # Test: Search for UI error handling related to failed request status rg -i 'error.*request.*status' --type ts --type tsx
Line range hint
1-114: Summary: Improved request initialization, but clarification needed on UI crash fix.The changes in this file focus on enhancing request initialization, particularly for task creation. While these improvements align with the overall goal of refining request handling, it's not immediately clear how they address the UI crash issue mentioned in the PR objectives.
Could you please provide more context on:
- How these changes contribute to fixing the UI crash on failed request status check?
- Are there additional changes in other files that work in conjunction with these modifications to resolve the issue?
This information would help in understanding the full scope of the fix and ensure that the changes adequately address the reported problem.
To get a broader view of the changes related to request handling and UI, please run:
cvat-core/src/requests-manager.ts (3)
8-8: LGTM: New imports added correctly.The new imports for
SerializedRequestandfieldsToSnakeCaseare necessary for the changes in thelistenmethod and are correctly placed.Also applies to: 11-11
68-68: LGTM: DestructuringinitialRequest.Destructuring
initialRequestfromoptionsis a good practice. It aligns with the updated method signature and improves code readability.
61-61: Verify impact of makinginitialRequestrequired.The change to make
initialRequesta required parameter improves type safety. However, ensure that all existing calls tolistenare updated to provide this parameter.Run the following script to check for any calls to
listenthat might need updating:cvat-core/src/session.ts (3)
1144-1147: Update tosavemethod signatureThe
savemethod's signature has been updated to include additional options. This change allows for more flexibility in handling requests during the save operation.The new options object with
requestStatusCallbackandgetInitialRequestparameters provides better control over the request lifecycle. This is a good improvement for managing asynchronous operations and initial request states.
Line range hint
1144-1155: Summary of changesThe modifications to the
Taskclass enhance its capability to handle requests and asynchronous operations:
- The
savemethod now accepts additional options for request status callbacks and initial request retrieval.- A new
listenToCreatemethod has been added, likely to support asynchronous task creation processes.These changes improve the flexibility and control over task-related operations, which is beneficial for managing complex workflows and providing better feedback to users during long-running processes.
The changes are well-structured and align with good practices for handling asynchronous operations in TypeScript.
Line range hint
1151-1155: NewlistenToCreatemethod addedA new asynchronous method
listenToCreatehas been added to theTaskclass.This new method enhances the Task class by providing a way to listen for the creation process. It's a good addition for handling asynchronous task creation operations.
To ensure this method is properly integrated, let's check its usage across the codebase:
cvat-ui/src/actions/requests-actions.ts (1)
98-123: 'generateInitialRequest' Function Implementation Looks GoodThe new
generateInitialRequestfunction effectively creates aRequestobject with the correct default values and properties derived frominitialData. The usage offieldsToSnakeCasemaintains the consistency of field naming conventions.Accessing the current user from the store ensures that the
ownerproperty is set appropriately. The function enhances the initialization process of requests.cvat-ui/src/actions/export-actions.ts (3)
11-11: ImportgenerateInitialRequestfor Consistent Request InitializationGood addition of the
generateInitialRequestimport. This ensures that thelistenfunction can properly initialize requests with the necessary parameters.
150-156: Ensure Correct Casting ofinstanceTypeCasting
instanceTypeas'project' | 'task'assumes that it will not be'job'. Verify that in all cases wherelistenExportBackupAsyncis called, theinstancecannot be aJobto prevent potential type errors.To check that
instanceTypeis never'job'in this context, you can run:#!/bin/bash # Description: Verify `instanceType` values for backup exports # Expected: `instanceType` should only be 'project' or 'task' in backup export contexts. rg 'exportBackupAsync\(' -A5 -t js | grep 'getInstanceType' -A1
92-98: Verify thetypeParameter ingenerateInitialRequestThe
typeparameter is set asexport:${resource}, whereresourcecan be'dataset'or'annotations'. Please ensure that this dynamictypealigns with the expected values in the request handling logic to prevent any mismatches.To confirm consistency of
typevalues in request initialization, run the following script:cvat-core/src/session-implementation.ts (1)
769-769: Addition ofinitialRequestenhances request handlingThe inclusion of
initialRequest: options?.getInitialRequest(taskID)at line 769 ensures that the initial request status is properly managed when listening for task creation updates. This improves the robustness of the request handling logic.cvat-ui/src/actions/import-actions.ts (3)
15-15: Correctly importing necessary functions for request handlingThe addition of
shouldListenForProgressandgenerateInitialRequestto the imports ensures that these functions are available for the updated request handling logic.
84-90: Proper usage ofinitialRequestinlistenfunctionThe
initialRequestparameter is correctly constructed usinggenerateInitialRequestand passed to thelistenfunction. This enhances the tracking and management of import requests and their progress.
189-194: Correct initialization of request inlistenImportBackupAsyncThe
initialRequestis properly constructed with the correct parameters and passed to thelistenfunction, ensuring consistent handling of backup import requests.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8575 +/- ##
===========================================
- Coverage 74.25% 74.24% -0.01%
===========================================
Files 403 403
Lines 43315 43335 +20
Branches 3914 3916 +2
===========================================
+ Hits 32162 32173 +11
- Misses 11153 11162 +9
|
There was a problem hiding this comment.
The logic surrounding generateInitialRequest seems a bit unclear.
From what I understand, it was introduced to handle cases where no response is received from the server, making it difficult to properly initialize the Request class.
However, I believe this approach may not be the best solution. It seems we are duplicating logic that the server already handles. Thus, we should ensure that generateInitialRequest mirrors the server API’s behavior.
Additionally, this logic is implemented outside of cvat-core and is duplicated across various actions in the cvat-ui module. It also introduces options like getInitialRequest in the Task.save() method, which makes the implementation less intuitive.
Instead, I’d suggest a simpler approach for handling situations where we can’t retrieve the status:
- We can use initialRequest when available to initialize certain fields, which is fine.
- If it’s unavailable and an exception occurs, we should properly handle the error, something is not handled in rendering logic if currently we see the error (we do not need to display Request object in the list because we do not actually have it)
- As a temporary workaround for server issues, we could attempt to retry the request on the server-proxy level, perhaps with a timeout and a limited number of retries.
|
@bsekachev do you mean 502 error in case of requesting GET
|
Implemented.
The issue was actually with
Added |
|
Motivation and context
When a status check for
requestis failing for any reason, eg something happened on the server and the request failed with 50X error. The requests page crushes with an error:Cannot read property 'target' of undefined:Now the error message is shown:
How has this been tested?
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have linked related issues (see GitHub docs)(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes
New Features
listenToCreatemethod in the Task class for better task creation management.generateInitialRequestfunction to standardize request initialization across imports and exports.Bug Fixes
Documentation
Chores