Fix an issue with deleting frames #8872
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 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
🧹 Nitpick comments (2)
cvat-core/src/frames.ts (2)
570-572: Restore previous cache carefully.Restoring from
previousCacheis a neat fallback mechanism if new metadata loading fails. Consider logging or handling partial updates for better debugging, especially if a half-updated cache can cause inconsistent data.
631-631: Improve clarity on destructured meta.
const { meta: prevMeta } = cached;is good, but consider a more direct naming (e.g.,oldMeta) for clarity and consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cvat-core/src/frames.ts(6 hunks)
🔇 Additional comments (8)
cvat-core/src/frames.ts (8)
42-43: Consider adopting consistent resolved/promise states.
The union approach (holding either a FramesMetaData or a Promise<FramesMetaData>) helps lazy-load metadata. However, be mindful of potential edge cases if code assumes a resolved state prematurely. Continue ensuring all consumers handle both states safely.
44-47: Enum-based approach improves readability.
Using DeletedFrameState clarifies frame deletion statuses and avoids "magic strings." This design is more resilient and discoverable.
90-96: Validate opposite state reset workflow.
The logic for toggling between DELETED and RESTORED is solid. Nonetheless, ensure that simultaneous toggles (e.g., concurrent modifications) do not cause race conditions. If concurrency is a concern, consider adding suitable synchronization or queueing.
566-566: Cache assignment could introduce race conditions.
Reassigning frameMetaCache[id] after loading new metadata is acceptable, but be cautious of concurrent requests that might overwrite or revert the cache unexpectedly.
578-580: Immediate promise resolution ensures efficiency.
Returning a pre-resolved promise when the cache is already available is an efficient short-circuit. This prevents unnecessary server calls and reduces latency.
Line range hint 633-646: Ensure chunk redefinition handles unsaved data.
When meta.chunksUpdatedDate changes, clearing old data is correct. However, double-check for any unsaved or ephemeral data that could be lost in the process.
647-658: Merging updated fields with new metadata is robust.
Applying the local changes from prevMeta to meta ensures user actions aren't lost. This approach is well-organized. Nonetheless, consider adding logs or metrics if frequent merges lead to confusion or performance concerns.
810-816: Throwing an error on uninitialized or still-pending metadata is prudent.
The explicit error message helps debugging. Consider clarifying next steps in a user-facing message if this error can bubble up to the UI.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8872 +/- ##
===========================================
- Coverage 73.90% 73.90% -0.01%
===========================================
Files 408 408
Lines 44131 44163 +32
Branches 3986 3993 +7
===========================================
+ Hits 32615 32637 +22
- Misses 11516 11526 +10
|
klakhov
requested review from
zhiltsov-max
and removed request for
bsekachev
|
### Motivation and context Fix #8872 dealt with incorrect states of UI and cache after frame deletion. The frame was not being deleted and the cache was not updated properly to reflect the changes. ### How has this been tested? #### Set up - open Main task job - save default `cvat.config.jobMetaDataReloadPeriod` value from `cvat-core/src/config.ts` - temporarily set `jobMetaDataReloadPeriod` to a small value (100 ms) for the sake of easier testing ### Test suite - Inside the job, wait for the small job metadata reload period to elapse - Transition to the next frame by clicking the next button on the player - Verify that `GET /data/meta` is sent and that the response has frames in it. - Save current frame number to the variable `frameNum` - Delete the frame: click on the bin, click on 'Delete' button in the modal - Ensure that 'Restore frame' button is absent - Make sure that the frame is deleted by asserting that current frame has number `frameNum + 1` (the next frame in the dataset) - Click Save to send `PATCH` to `/data/meta`. - Intercept the request to validate that response has `deleted_frames` property, that it's an `Array` and that it contains the deleted frame number `frameNum` - Assert that `deleted_frames` array contains one element: `frameNum` #### Tear down - Set `jobMetaDataReloadPeriod` back to the saved default value ### Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. If an item isn't applicable for some reason, then ~~explicitly strikethrough~~ the whole line. If you don't do that, GitHub will show incorrect progress for the pull request. If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] I submit my changes into the `develop` branch - [ ] I have created a changelog fragment <!-- see top comment in CHANGELOG.md --> - [ ] I have updated the documentation accordingly - [ ] I have added tests to cover my changes - [ ] I have linked related issues (see [GitHub docs]( https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)) - [ ] I have increased versions of npm packages if it is necessary ([cvat-canvas](https://github.com/cvat-ai/cvat/tree/develop/cvat-canvas#versioning), [cvat-core](https://github.com/cvat-ai/cvat/tree/develop/cvat-core#versioning), [cvat-data](https://github.com/cvat-ai/cvat/tree/develop/cvat-data#versioning) and [cvat-ui](https://github.com/cvat-ai/cvat/tree/develop/cvat-ui#versioning)) ### License - [ ] I submit _my code changes_ under the same [MIT License]( https://github.com/cvat-ai/cvat/blob/develop/LICENSE) that covers the project. Feel free to contact the maintainers if that's a concern. --------- Co-authored-by: Oleg Valiulin <[email protected]> Co-authored-by: Boris Sekachev <[email protected]> Co-authored-by: Kirill Lakhov <[email protected]>
Motivation and context
Previously, in the Honeypots patch, we introduced periodic metadata updates to allow the UI to reflect chunk changes dynamically. However, this introduced a couple of issues:
Steps to Reproduce the Frame Deletion Issue
jobMetaDataReloadPeriodinconfig.tsofcvat-coreto a small interval, such as 15 seconds (the default is 1 hour)./data/metarequest is sent.jobMetaDataReloadPeriodto elapse./data/metarequest is sent.Resolution
To fix this, I removed the secondary storage of
jobMetainframeDataCache. Now, it uses an asynchronous getter that fetches data directly fromframeMetaCache. Having two caches for the same data caused synchronization issues, which led to this bug.To fix second issue I added merge logic inside getFramesMeta function that will update new object with unsaved data.
How has this been tested?
Checklist
developbranch[ ] I have updated the documentation accordingly(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
Bug Fixes
Refactor