From 303f614d035b97bcc4c1e7e85cbdf805a0f1f637 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 4 Sep 2026 15:05:43 -0400 Subject: [PATCH] fix(dataset): finish teardown before releasing image data --- src/components/ResizableNavDrawer.vue | 1 - src/components/SliceSlider.vue | 17 +++++---- src/components/__tests__/SliceSlider.spec.ts | 36 +++++++++++++++++++ .../tools/crop/Crop2DLineHandle.vue | 2 -- .../__tests__/datasetRemoveCascade.spec.ts | 22 +++++++++++- src/store/__tests__/image-cache.spec.ts | 36 +++++++++++++++++++ src/store/datasets-images.ts | 2 +- src/store/datasets.ts | 4 ++- src/store/image-cache.ts | 15 ++++---- 9 files changed, 116 insertions(+), 19 deletions(-) create mode 100644 src/components/__tests__/SliceSlider.spec.ts create mode 100644 src/store/__tests__/image-cache.spec.ts diff --git a/src/components/ResizableNavDrawer.vue b/src/components/ResizableNavDrawer.vue index 339ae5569..a49f98f66 100644 --- a/src/components/ResizableNavDrawer.vue +++ b/src/components/ResizableNavDrawer.vue @@ -104,7 +104,6 @@ export default { stopResize(ev) { if (ev) { if (!this.drawerBorder.hasPointerCapture(ev.pointerId)) return; - this.drawerBorder.releasePointerCapture(ev.pointerId); } this.$refs.infoPane.$el.style.transition = ''; diff --git a/src/components/SliceSlider.vue b/src/components/SliceSlider.vue index 456d40f9c..faa6d4ba9 100644 --- a/src/components/SliceSlider.vue +++ b/src/components/SliceSlider.vue @@ -63,6 +63,7 @@ export default { return { maxHandlePos: 0, dragging: false, + pointerId: null, initialHandlePos: 0, initialMousePosY: 0, yOffset: 0, @@ -96,19 +97,23 @@ export default { }, beforeUnmount() { - this.resizeObserver.disconnect(); + this.resizeObserver?.disconnect(); }, methods: { updateMaxHandlePos() { + if (!this.$refs.handleContainer) return; this.maxHandlePos = this.$refs.handleContainer.clientHeight - this.handleHeight; }, onDragStart(ev) { + const container = this.$refs.handleContainer; + if (!container) return; ev.preventDefault(); this.dragging = true; + this.pointerId = ev.pointerId; this.initialMousePosY = ev.pageY; if (ev.target === this.$refs.handle) { @@ -116,7 +121,7 @@ export default { this.initialHandlePos = getYOffsetFromTransform(handleStyles.transform); } else { // move handle to mouse pos - const { y } = this.$refs.handleContainer.getBoundingClientRect(); + const { y } = container.getBoundingClientRect(); this.initialHandlePos = Math.max( 0, Math.min(this.maxHandlePos, ev.pageY - y - this.handleHeight / 2) @@ -127,11 +132,11 @@ export default { this.yOffset = 0; - this.$refs.handleContainer.setPointerCapture(ev.pointerId); + container.setPointerCapture(ev.pointerId); }, onDragMove(ev) { - if (!this.$refs.handleContainer.hasPointerCapture(ev.pointerId)) return; + if (ev.pointerId !== this.pointerId) return; ev.preventDefault(); this.yOffset = ev.pageY - this.initialMousePosY; @@ -140,11 +145,11 @@ export default { }, onDragEnd(ev) { - if (!this.$refs.handleContainer.hasPointerCapture(ev.pointerId)) return; + if (ev.pointerId !== this.pointerId) return; ev.preventDefault(); - this.$refs.handleContainer.releasePointerCapture(ev.pointerId); this.dragging = false; + this.pointerId = null; const slice = this.getNearestSlice(this.handlePosition); this.$emit('update:modelValue', slice); }, diff --git a/src/components/__tests__/SliceSlider.spec.ts b/src/components/__tests__/SliceSlider.spec.ts new file mode 100644 index 000000000..074aacbad --- /dev/null +++ b/src/components/__tests__/SliceSlider.spec.ts @@ -0,0 +1,36 @@ +import { mount } from '@vue/test-utils'; +import { describe, expect, it } from 'vitest'; + +import SliceSlider from '@/src/components/SliceSlider.vue'; + +const mountSlider = () => + mount(SliceSlider, { + props: { min: 0, max: 10, step: 1 }, + }); + +describe('SliceSlider', () => { + it('ignores a late pointer event after its element is gone', () => { + const wrapper = mountSlider(); + const vm = wrapper.vm as unknown as { + onDragMove: (event: PointerEvent) => void; + }; + wrapper.unmount(); + + expect(() => vm.onDragMove({ pointerId: 7 } as PointerEvent)).not.toThrow(); + }); + + it('keeps dragging when an unrelated pointer ends', async () => { + const wrapper = mountSlider(); + await wrapper.trigger('pointerdown', { pointerId: 7, pageY: 10 }); + + const vm = wrapper.vm as unknown as { + dragging: boolean; + pointerId: number | null; + onDragEnd: (event: PointerEvent) => void; + }; + vm.onDragEnd({ pointerId: 8 } as PointerEvent); + + expect(vm.dragging).toBe(true); + expect(vm.pointerId).toBe(7); + }); +}); diff --git a/src/components/tools/crop/Crop2DLineHandle.vue b/src/components/tools/crop/Crop2DLineHandle.vue index c46a1d2eb..47eff107d 100644 --- a/src/components/tools/crop/Crop2DLineHandle.vue +++ b/src/components/tools/crop/Crop2DLineHandle.vue @@ -25,8 +25,6 @@ export default defineComponent({ const onPointer = (down: boolean, ev: PointerEvent) => { if (down) { grabLineEl.value?.setPointerCapture(ev.pointerId); - } else { - grabLineEl.value?.releasePointerCapture(ev.pointerId); } cursor.value = down ? 'grabbing' : 'grab'; }; diff --git a/src/store/__tests__/datasetRemoveCascade.spec.ts b/src/store/__tests__/datasetRemoveCascade.spec.ts index ff27f8d96..3ec1ef05a 100644 --- a/src/store/__tests__/datasetRemoveCascade.spec.ts +++ b/src/store/__tests__/datasetRemoveCascade.spec.ts @@ -1,5 +1,6 @@ -import { beforeEach, describe, expect, it } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import { setActivePinia, createPinia } from 'pinia'; +import { nextTick } from 'vue'; import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData'; import vtkDataArray from '@kitware/vtk.js/Common/Core/DataArray'; @@ -120,6 +121,25 @@ describe('dataset remove — synchronous reference cascade', () => { expect(viewStore.getViewsForData('img-1')).toEqual([]); }); + it('detaches consumers before disposing the removed image', async () => { + seatImage('img-1', 'CT'); + const imageCacheStore = useImageCacheStore(); + const image = imageCacheStore.imageById['img-1']; + const dispose = vi.spyOn(image, 'dispose'); + const viewStore = useViewStore(); + bindFirstViewTo('img-1'); + + useDatasetStore().remove('img-1'); + + expect(imageCacheStore.imageById).not.toHaveProperty('img-1'); + expect(viewStore.getViewsForData('img-1')).toEqual([]); + expect(dispose).not.toHaveBeenCalled(); + + await nextTick(); + + expect(dispose).toHaveBeenCalledOnce(); + }); + it('drops crop state keyed by the removed image', () => { seatImage('img-1', 'CT'); const cropStore = useCropStore(); diff --git a/src/store/__tests__/image-cache.spec.ts b/src/store/__tests__/image-cache.spec.ts new file mode 100644 index 000000000..e392cecde --- /dev/null +++ b/src/store/__tests__/image-cache.spec.ts @@ -0,0 +1,36 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { createPinia, setActivePinia } from 'pinia'; +import vtkDataArray from '@kitware/vtk.js/Common/Core/DataArray'; +import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData'; + +import { useImageCacheStore } from '@/src/store/image-cache'; + +const seatImage = () => { + const data = vtkImageData.newInstance(); + data.setDimensions(2, 2, 2); + data.getPointData().setScalars( + vtkDataArray.newInstance({ + name: 'scalars', + numberOfComponents: 1, + values: new Uint8Array(8), + }) + ); + const store = useImageCacheStore(); + store.addVTKImageData(data, 'CT', { id: 'img-1' }); + return store; +}; + +describe('image cache lifecycle', () => { + beforeEach(() => { + setActivePinia(createPinia()); + }); + + it('treats an image whose VTK data is unavailable as absent', () => { + const store = seatImage(); + vi.spyOn(store.imageById['img-1'], 'getVtkImageData').mockReturnValue( + undefined as never + ); + + expect(store.getVtkImageData('img-1')).toBeNull(); + }); +}); diff --git a/src/store/datasets-images.ts b/src/store/datasets-images.ts index 8c11a4216..29e2610c8 100644 --- a/src/store/datasets-images.ts +++ b/src/store/datasets-images.ts @@ -51,8 +51,8 @@ export const useImageStore = defineStore('images', () => { } function deleteData(id: string) { - useImageCacheStore().removeImage(id); removeFromArray(idList.value, id); + useImageCacheStore().removeImage(id); } function checkAllImagesSameSpace() { diff --git a/src/store/datasets.ts b/src/store/datasets.ts index ac3204ec0..e74af551c 100644 --- a/src/store/datasets.ts +++ b/src/store/datasets.ts @@ -256,10 +256,12 @@ export const useDatasetStore = defineStore('dataset', () => { // Anonymous volume. loadedData.value = loadedData.value.filter((d) => d.dataID !== id); dicomStore.deleteVolume(id); - imageStore.deleteData(id); layersStore.remove(id); useViewConfigStore().removeData(id); useImageStatsStore().removeData(id); + // Cache eviction disposes the VTK object after Vue has flushed consumers. + // Run every other synchronous reference cleanup before starting eviction. + imageStore.deleteData(id); }; const removeAll = () => { diff --git a/src/store/image-cache.ts b/src/store/image-cache.ts index f884a5309..62bf54da5 100644 --- a/src/store/image-cache.ts +++ b/src/store/image-cache.ts @@ -9,7 +9,7 @@ import { Maybe } from '@/src/types'; import { ImageMetadata } from '@/src/types/image'; import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData'; import { defineStore } from 'pinia'; -import { markRaw, reactive, ref } from 'vue'; +import { markRaw, nextTick, reactive, ref } from 'vue'; /** * An internal cache of progressively loadable images. @@ -37,7 +37,7 @@ export const useImageCacheStore = defineStore('image-cache', () => { const data = image.getVtkImageData(); // ProgressiveImage initializes with empty vtkImageData before actual data loads. // VTK.js volume renderer crashes on empty data (null scalar texture). - if (!data.getPointData().getScalars()?.getData()?.length) return null; + if (!data?.getPointData().getScalars()?.getData()?.length) return null; return data; } @@ -116,19 +116,20 @@ export const useImageCacheStore = defineStore('image-cache', () => { function removeImage(id: string) { if (!(id in imageById)) return; + const image = imageById[id]; unregisterListeners(id); - // Release vtk data and any per-image caches (e.g. cine compressed frames - // and decoded-frame LRU). Without this, removing a dataset leaks all of - // its memory until the page reloads. - imageById[id].dispose(); - const idx = imageIds.value.indexOf(id); if (idx > -1) imageIds.value.splice(idx, 1); delete imageById[id]; delete imageStatus[id]; delete imageLoading[id]; delete imageErrors[id]; + + // Vue tears down image consumers in its next update flush. Keep the VTK + // object alive until those consumers have detached their actors and event + // handlers, but make it unreachable from the cache immediately. + void nextTick(() => image.dispose()); [...deletionCallbacks].forEach((callback) => callback([id])); }