Fixed: Cannot read properties of undefined (reading 'push') #8648
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 WalkthroughThis pull request addresses an exception error in the codebase related to accessing properties of undefined. It modifies 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 (1)
cvat-core/src/requests-manager.ts (1)
Line range hint
74-152: Consider adding synchronization to prevent race conditions.While the promise creation and assignment logic is correct, there's a potential race condition between the initial check for an existing entry and the creation of a new one. Multiple simultaneous calls could create duplicate entries.
Consider this improvement:
async listen( requestID: string, options: { callback: (request: Request) => void, initialRequest?: Request, }, ): Promise<Request> { + // Use an atomic check-and-set operation + if (!this.listening[requestID]) { + this.listening[requestID] = { + onUpdate: [], + timeout: null, + request: null, + requestDelayIdx: 0, + promise: null as any, // Temporary placeholder + }; + } if (callback) { this.listening[requestID].onUpdate.push(callback); } + // Only create the promise if it doesn't exist + if (!this.listening[requestID].promise) { const promise = new Promise<Request>((resolve, reject) => { // ... existing promise logic ... }); this.listening[requestID].promise = promise; } return this.listening[requestID].promise;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
changelog.d/20241106_095739_sekachev.bs_fixed_exception_1.md(1 hunks)cvat-core/src/requests-manager.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- changelog.d/20241106_095739_sekachev.bs_fixed_exception_1.md
🔇 Additional comments (2)
cvat-core/src/requests-manager.ts (2)
135-136: LGTM! Proper cleanup on error.
Good addition of explicit cleanup in the error handler. This ensures consistent state management by removing the listening entry in both success and error cases.
37-37: LGTM! Type change prevents undefined access.
Making promise required instead of optional is a good fix that prevents the "Cannot read properties of undefined" error by ensuring all entries in listening have an associated promise.
Let's verify there are no remaining optional promise usages:
✅ Verification successful
Type change is safe and consistent with codebase patterns
The verification shows that:
- No optional promise declarations (
promise?:) remain in the codebase - Similar required promise patterns exist in
server-proxy.tswithLongProcessListenerinterface - The change aligns with existing promise handling patterns in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining optional promise declarations
rg -g '*.ts' 'promise\?:\s*Promise<'
Length of output: 38
Script:
#!/bin/bash
# Let's try a broader search for promise-related type definitions
rg -g '*.ts' 'promise.*Promise' -A 2 -B 2
# Also search for the specific interface containing this promise
ast-grep --pattern 'interface $_ {
$$$
promise: Promise<$_>
$$$
}'
Length of output: 1806
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8648 +/- ##
===========================================
- Coverage 74.30% 74.30% -0.01%
===========================================
Files 401 401
Lines 43421 43422 +1
Branches 3951 3951
===========================================
Hits 32265 32265
- Misses 11156 11157 +1
|
Motivation and context
To reproduce import annotations to a job for example:
Cannot read properties of undefined (reading 'push')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
Improvements