Fixed issue: Cannot read properties of undefined (reading 'getUpdated') #8785
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 in this pull request address a runtime error related to accessing an undefined object's property and update the version number of the Changes
Poem
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: 0
🧹 Outside diff range and nitpick comments (3)
cvat-core/src/frames.ts (3)
563-577: Approve the fix for undefined property accessThe changes properly handle the Promise chain and error cases, preventing the "Cannot read properties of undefined" error. The implementation now ensures that
frameMetaCache[jobID]always contains a valid Promise, even in error scenarios.However, consider adding a comment explaining the error handling strategy:
frameMetaCache[jobID] = new Promise<FramesMetaData>((resolve) => { + // Resolve with original meta on error to maintain cache consistency serverProxy.frames.saveMeta('job', jobID, {
563-577: Consider improving error propagationWhile the current implementation prevents the undefined error, it might mask other legitimate errors by always resolving the Promise. Consider using Promise.reject for error cases that should be handled by the caller.
}).catch((error) => { - resolve(meta); - throw error; + // Reject the promise to allow proper error handling upstream + return Promise.reject(error); });
563-577: Document the caching strategyThe frame meta caching logic is complex and crucial for the system's operation. Consider adding documentation that explains:
- The caching lifecycle
- Error handling strategy
- Cache invalidation rules
+/** + * Saves job meta data to the server and updates the cache + * @param meta The meta data to save + * @param jobID The job ID + * @returns A promise that resolves with the updated meta data + * + * Cache behavior: + * - On success: Cache is updated with new server response + * - On error: Cache maintains the original meta to prevent undefined access + * - Cache is invalidated when the job is cleared + */ async function saveJobMeta(meta: FramesMetaData, jobID: number): Promise<FramesMetaData> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
changelog.d/20241206_153723_sekachev.bs_fixed_issue.md(1 hunks)cvat-core/package.json(1 hunks)cvat-core/src/frames.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- cvat-core/package.json
- changelog.d/20241206_153723_sekachev.bs_fixed_issue.md
🔇 Additional comments (1)
cvat-core/src/frames.ts (1)
563-577: Verify error handling in the UI layer
The changes in error handling behavior might affect how errors are displayed to users in the UI.
✅ Verification successful
Based on the search results, I can now provide a final response regarding the error handling verification:
Error handling is properly implemented in the UI layer
The error from frames.saveMeta is already handled appropriately:
- The error is caught in the core layer and propagated up while maintaining the previous state through
resolve(meta) - The UI layer has a comprehensive error notification system that displays errors through
notification.error() - Errors are displayed with proper context, descriptions, and custom CSS classes for styling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any UI components that handle errors from saveJobMeta
rg -l "saveJobMeta.*catch" --type ts
Length of output: 38
Script:
#!/bin/bash
# Let's search for UI components that might handle frame deletion or meta updates
rg -A 5 "frames\.saveMeta" --type ts
Length of output: 551
Script:
#!/bin/bash
# Let's also check for any error handling patterns in UI components related to frames
rg -A 5 "deletedFrames|frameMetaCache" --type ts
Length of output: 7245
Script:
#!/bin/bash
# Search for components that might show error notifications
ast-grep --pattern 'catch (error) { $$$ }'
Length of output: 44
Script:
#!/bin/bash
# Let's search for UI components that might handle frame-related errors
rg -A 5 "notification.*error|showError|handleError" --type ts
Length of output: 35412
Script:
#!/bin/bash
# Let's check how errors are handled in the UI layer
rg -A 5 "catch.*error.*notification" --type ts
Length of output: 1330
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8785 +/- ##
========================================
Coverage 73.92% 73.92%
========================================
Files 409 409
Lines 43950 43958 +8
Branches 3985 3986 +1
========================================
+ Hits 32488 32495 +7
- Misses 11462 11463 +1
|
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
Bug Fixes
New Features
Chores
cvat-coreproject from 15.3.0 to 15.3.1.