[GSoC2024] Cannot read properties of null (reading 'annotations') #7363 #7704
Conversation
cvat-core/tests/api/annotations.cjs
Outdated
| test('handle page interrupt during annotation retrieval', async () => { | ||
| const job = (await cvat.jobs.get({ jobID: 101 }))[0]; | ||
|
|
||
| // Mocking the behavior of job.annotations.get() to simulate a page interrupt | ||
| const mockAnnotationsGet = jest.spyOn(job.annotations, 'get'); | ||
|
|
||
| // Reject the promise with an error to simulate a page interrupt | ||
| mockAnnotationsGet.mockRejectedValueOnce(new Error("Cannot read properties of null (reading 'annotations')")); | ||
|
|
||
| // Expectation: Retrieving annotations should throw an error | ||
| await expect(job.annotations.get(0)).rejects.toThrow("Cannot read properties of null (reading 'annotations')"); | ||
|
|
||
| // Ensuring that job.annotations.get() was called with the correct arguments | ||
| expect(mockAnnotationsGet).toHaveBeenCalledWith(0); |
There was a problem hiding this comment.
- The code does not make sense, because the test tests itself
- Perhaps we need Cypress E2E tests to cover the bugfix, not jest. Because issue on
cvat-uilevel. Oncvat-corelevel we can't even reproduce the problem.
There was a problem hiding this comment.
The test was aiming to simulate a scenario where fetching annotations encounters an error. It was meant for verifying that the method correctly throws an error with the expected message when encountering a page interrupt.
If the issue is more related to UI behavior, then let me focus on E2E tests with Cypress.
| let currentPath = window.location.pathname; | ||
|
|
||
| // Listen for popstate events (back/forward navigation) | ||
| window.addEventListener('popstate', () => { |
There was a problem hiding this comment.
popstate event listener is never removed. Memory leak, because fetchAnnotationsAsync may be called plenty of times.
There was a problem hiding this comment.
Ok, I should remove the event listener after it's triggered
| window.addEventListener('popstate', () => { | ||
| const newPath = window.location.pathname; | ||
| if (newPath !== currentPath) { | ||
| console.log('Navigation interruption detected.'); |
There was a problem hiding this comment.
we do not need logging in production code
| maxZ, | ||
| }, | ||
| // Check if navigation interruption occurred | ||
| navigationInterruptionDetected((interrupted: boolean) => { |
There was a problem hiding this comment.
From the code I see:
- We fetch annotations in line 318
- We call
navigationInterruptionDetectedwith callback passed
2.1. Callback withtrueargument will be called only ifpostateevent was dispatched.
2.2. Callback never called withfalseargument.
Condition in line 322 is always false, so, I am not sure how this code is supposed to work. Did you test it?
There was a problem hiding this comment.
Ok, I also see errors in my logic of handling this now. What do you think of handling this issue in such away.
- I separate the handling of navigation interruptions from the handling of fetching errors.
- I use a flag to track whether a navigation interruption occurred during the fetching process.
- Then check if navigation interruption occurred , dispatch success action if no interruption AND dispatch FETCH_ANNOTATIONS_FAILED only if not due to navigation interruption
- Then navigationInterruptionDetected function now returns a function that removes the event listener when it's no longer needed. So that that the event listener is cleaned up to prevent memory leaks.
|
Thank you for trying to contribute. |
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.