From ee286f81c1bae53331ae4a99a19133977325571e Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 20 Sep 2026 19:58:58 -0400 Subject: [PATCH 1/5] fix(layers): return an owned image from ensureSameSpace ensureSameSpace returned the candidate itself when the grids already matched, so the layer cached under `${parent}::${source}` and the source dataset referenced one vtkImageData. The cache disposes what it owns, so removing the layer or deleting the parent called delete() on an image the source still held, leaving a wiped model reachable from the cache. Return a geometry copy that shares the scalar array. vtk.js delete() only clears the object's own model, so the array survives either owner and the voxels are not duplicated. Both return paths now hand back an owned object, so the ownership rule sits with the borrow instead of at each call site. --- src/io/resample/resample.ts | 5 +- .../__tests__/layerImageOwnership.spec.ts | 70 +++++++++++++++++++ src/store/__tests__/segmentImportPath.spec.ts | 7 +- src/utils/vtk-helpers.ts | 22 ++++++ 4 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 src/store/__tests__/layerImageOwnership.spec.ts diff --git a/src/io/resample/resample.ts b/src/io/resample/resample.ts index 973b1dbe3..41b302ec2 100644 --- a/src/io/resample/resample.ts +++ b/src/io/resample/resample.ts @@ -2,6 +2,7 @@ import { Image } from 'itk-wasm'; import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData'; import vtkITKHelper from '@kitware/vtk.js/Common/DataModel/ITKHelper'; import { compareImageIndexGrids } from '@/src/utils/imageSpace'; +import { shallowCopyImageData } from '@/src/utils/vtk-helpers'; import { runWasm } from './itkWasmUtils'; export async function resample(fixed: Image, moving: Image, label = false) { @@ -27,8 +28,10 @@ export async function ensureSameSpace( resampleCandidate: vtkImageData, label = false ) { + // Callers own what they get back and may hand it to something that disposes + // it, so never return the candidate itself. if (compareImageIndexGrids(target, resampleCandidate)) { - return resampleCandidate; + return shallowCopyImageData(resampleCandidate); } const itkImage = await resample( vtkITKHelper.convertVtkToItkImage(target), diff --git a/src/store/__tests__/layerImageOwnership.spec.ts b/src/store/__tests__/layerImageOwnership.spec.ts new file mode 100644 index 000000000..7a0a48f90 --- /dev/null +++ b/src/store/__tests__/layerImageOwnership.spec.ts @@ -0,0 +1,70 @@ +import { describe, it, expect, beforeEach } 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'; + +// The real `ensureSameSpace` runs here. Matching grids take its fast path, so +// no wasm resampling is involved. +import { useLayersStore } from '@/src/store/datasets-layers'; +import { useImageCacheStore } from '@/src/store/image-cache'; + +const SIZE = 3; + +const seatImage = (id: string) => { + const image = vtkImageData.newInstance(); + image.setDimensions(SIZE, SIZE, SIZE); + image.getPointData().setScalars( + vtkDataArray.newInstance({ + name: 'scalars', + numberOfComponents: 1, + values: new Uint8Array(SIZE ** 3), + }) + ); + return useImageCacheStore().addVTKImageData(image, id, { id }); +}; + +const sourceScalars = () => + useImageCacheStore() + .getVtkImageData('source') + ?.getPointData() + .getScalars() + ?.getData(); + +beforeEach(() => { + setActivePinia(createPinia()); + seatImage('parent'); + seatImage('source'); +}); + +describe('layer image ownership', () => { + it('caches a distinct image from the source it was built from', async () => { + const imageCache = useImageCacheStore(); + + await useLayersStore().addLayer('parent', 'source'); + + expect(imageCache.getVtkImageData('parent::source')).not.toBe( + imageCache.getVtkImageData('source') + ); + }); + + it('leaves the source image readable after its parent is removed', async () => { + const store = useLayersStore(); + await store.addLayer('parent', 'source'); + + store.remove('parent'); + await nextTick(); + + expect(sourceScalars()).toHaveLength(SIZE ** 3); + }); + + it('leaves the source image readable after the layer is deleted', async () => { + const store = useLayersStore(); + await store.addLayer('parent', 'source'); + + store.deleteLayer('parent', 'source'); + await nextTick(); + + expect(sourceScalars()).toHaveLength(SIZE ** 3); + }); +}); diff --git a/src/store/__tests__/segmentImportPath.spec.ts b/src/store/__tests__/segmentImportPath.spec.ts index ff2b96beb..043e2e18e 100644 --- a/src/store/__tests__/segmentImportPath.spec.ts +++ b/src/store/__tests__/segmentImportPath.spec.ts @@ -44,7 +44,12 @@ describe('labelmap import index alignment', () => { const runWasm = useNodeResampling(); const parent = makeImage(); const child = makeImage(); - expect(await ensureSameSpace(parent, child, true)).toBe(child); + const result = await ensureSameSpace(parent, child, true); + // The contract is no resampling and no voxel copy. The wrapper is the + // caller's to own, so it is not the candidate itself. + expect(result.getPointData().getScalars().getData()).toBe( + child.getPointData().getScalars().getData() + ); expect(runWasm).not.toHaveBeenCalled(); }); diff --git a/src/utils/vtk-helpers.ts b/src/utils/vtk-helpers.ts index f0841039a..b6ea83384 100644 --- a/src/utils/vtk-helpers.ts +++ b/src/utils/vtk-helpers.ts @@ -12,6 +12,7 @@ import { OpacityNodes, } from '@/src/types/views'; import vtkFieldData from '@kitware/vtk.js/Common/DataModel/DataSetAttributes/FieldData'; +import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData'; import { Maybe } from '@/src/types'; export function computeWorldToDisplay( @@ -290,3 +291,24 @@ export function getDataArray( export function isZeroWidthRange(range: [number, number] | number[]) { return range[0] === range[1]; } + +/** + * Copies an image's geometry into a new vtkImageData sharing the same scalars. + * + * vtk.js `delete()` wipes only the object's own model, so the shared scalar + * array outlives either copy. Point data beyond the active scalars, cell data + * and field data are not copied. + * + * vtkImageData.shallowCopy assigns the index/world matrices by reference and + * computeTransforms rewrites them in place, so a later setSpacing on either + * image would corrupt the other. + */ +export function shallowCopyImageData(image: vtkImageData) { + const copy = vtkImageData.newInstance(); + copy.setExtent(image.getExtent()); + copy.setOrigin(image.getOrigin()); + copy.setSpacing(image.getSpacing()); + copy.setDirection(image.getDirection()); + copy.getPointData().setScalars(image.getPointData().getScalars()); + return copy; +} From 33755dfedec9c9dafa4596412b9cc741ce2f2f0e Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 20 Sep 2026 19:59:00 -0400 Subject: [PATCH 2/5] fix(image-cache): treat a disposed image as absent A deleted vtkImageData keeps its closures but loses its model, so getPointData() returns undefined rather than throwing on access. The existing optional chain guarded the wrapper, not the model, so any image disposed without being evicted threw on every read. --- src/store/__tests__/image-cache.spec.ts | 9 +++++++++ src/store/image-cache.ts | 5 ++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/store/__tests__/image-cache.spec.ts b/src/store/__tests__/image-cache.spec.ts index e392cecde..f4f002d0d 100644 --- a/src/store/__tests__/image-cache.spec.ts +++ b/src/store/__tests__/image-cache.spec.ts @@ -33,4 +33,13 @@ describe('image cache lifecycle', () => { expect(store.getVtkImageData('img-1')).toBeNull(); }); + + it('treats a disposed image still in the cache as absent', () => { + const store = seatImage(); + // vtk.js delete() strips the model but keeps the closures, so the getters + // return undefined instead of throwing. + store.imageById['img-1'].dispose(); + + expect(store.getVtkImageData('img-1')).toBeNull(); + }); }); diff --git a/src/store/image-cache.ts b/src/store/image-cache.ts index 62bf54da5..3e33722f8 100644 --- a/src/store/image-cache.ts +++ b/src/store/image-cache.ts @@ -35,9 +35,12 @@ export const useImageCacheStore = defineStore('image-cache', () => { const image = imageById[id]; if (!image) return null; const data = image.getVtkImageData(); + // A deleted vtkImageData keeps its closures but loses its model, so every + // getter returns undefined rather than throwing on access. + if (!data || data.isDeleted()) return null; // 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; } From 70bb17433868a1630c5627bae2c95d22dbc9fc2c Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 20 Sep 2026 19:59:02 -0400 Subject: [PATCH 3/5] fix(errors): keep a failed bug report from dropping the message addError builds its bug report from application state that the error may have just corrupted, and it does so before the message exists. A throw there costs the user the message and leaves the global handler reporting the reporter's own failure. --- src/store/__tests__/messages.spec.ts | 28 +++++++++++++++ src/store/messages.ts | 13 ++++++- src/utils/__tests__/bugReport.spec.ts | 51 +++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 src/utils/__tests__/bugReport.spec.ts diff --git a/src/store/__tests__/messages.spec.ts b/src/store/__tests__/messages.spec.ts index b31593dfa..27f58cb6e 100644 --- a/src/store/__tests__/messages.spec.ts +++ b/src/store/__tests__/messages.spec.ts @@ -1,5 +1,10 @@ import { describe, it, beforeEach, expect } from 'vitest'; import { setActivePinia, createPinia } from 'pinia'; +import { nextTick } from 'vue'; +import vtkDataArray from '@kitware/vtk.js/Common/Core/DataArray'; +import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData'; +import { useImageStore } from '@/src/store/datasets-images'; +import { useImageCacheStore } from '@/src/store/image-cache'; import { MessageType, useMessageStore } from '@/src/store/messages'; describe('Message store', () => { @@ -66,4 +71,27 @@ describe('Message store', () => { messageStore.clearAll(); expect(messageStore.messages).to.be.empty; }); + + it('still reports an error when a dataset image was disposed', async () => { + // addError builds a bug report from the image cache, so a dataset the + // cache can no longer read must not cost the user the message itself. + const image = vtkImageData.newInstance(); + image.setDimensions(2, 2, 2); + image.getPointData().setScalars( + vtkDataArray.newInstance({ + name: 'scalars', + numberOfComponents: 1, + values: new Uint8Array(8), + }) + ); + useImageStore().addVTKImageData('CT', image, { id: 'img-1' }); + await nextTick(); + useImageCacheStore().imageById['img-1'].dispose(); + + const messageStore = useMessageStore(); + messageStore.addError('an error', { error: new Error('boom') }); + + expect(messageStore.messages).toHaveLength(1); + expect(messageStore.messages[0].title).toBe('an error'); + }); }); diff --git a/src/store/messages.ts b/src/store/messages.ts index de6ce86cb..4e1e999cc 100644 --- a/src/store/messages.ts +++ b/src/store/messages.ts @@ -37,6 +37,17 @@ interface State { msgList: string[]; } +// The reporter reads application state that an error may have just corrupted, +// and it runs before the message exists, so a throw here costs the user the +// message itself. +function describeBug(error?: Error) { + try { + return generateBugReport(error); + } catch { + return 'Bug report unavailable'; + } +} + export const useMessageStore = defineStore('message', { state: (): State => ({ _nextID: 1, @@ -60,7 +71,7 @@ export const useMessageStore = defineStore('message', { { type: MessageType.Error, title, - bugReport: generateBugReport(opts?.error), + bugReport: describeBug(opts?.error), }, { details: opts?.details ?? opts?.error?.stack, diff --git a/src/utils/__tests__/bugReport.spec.ts b/src/utils/__tests__/bugReport.spec.ts new file mode 100644 index 000000000..d52fdc163 --- /dev/null +++ b/src/utils/__tests__/bugReport.spec.ts @@ -0,0 +1,51 @@ +import { beforeEach, describe, expect, it } from 'vitest'; +import { createPinia, setActivePinia } from 'pinia'; +import { nextTick } from 'vue'; +import vtkDataArray from '@kitware/vtk.js/Common/Core/DataArray'; +import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData'; + +import { useImageStore } from '@/src/store/datasets-images'; +import { generateBugReport } from '@/src/utils/bugReport'; + +const seatImage = (id = 'img-1') => { + const data = vtkImageData.newInstance(); + data.setDimensions(2, 2, 2); + data.getPointData().setScalars( + vtkDataArray.newInstance({ + name: 'scalars', + numberOfComponents: 1, + values: new Uint8Array(8), + }) + ); + useImageStore().addVTKImageData('CT', data, { id }); +}; + +describe('generateBugReport', () => { + beforeEach(() => { + setActivePinia(createPinia()); + }); + + it('describes a healthy dataset', async () => { + seatImage(); + // Image metadata is a Vue watcher, so it lands on the next flush. + await nextTick(); + + const report = generateBugReport(new Error('boom')); + + expect(report).toContain('Datasets: 1'); + expect(report).toContain('2×2×2 Uint8'); + expect(report).toContain('boom'); + }); + + it('counts every dataset it describes', async () => { + seatImage('img-1'); + seatImage('img-2'); + await nextTick(); + + const report = generateBugReport(new Error('boom')); + + expect(report).toContain('Datasets: 2'); + expect(report).toContain('[0]'); + expect(report).toContain('[1]'); + }); +}); From 9c3c579c9a4c99633a103e95b10ddf4337608cc0 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 20 Sep 2026 20:46:44 -0400 Subject: [PATCH 4/5] test(e2e): cover the lifetime of an image used as a layer Layers two images that share a grid, so building the layer reuses the source image rather than resampling it. Removing the layer and building it again reads the source image a second time, which fails when removing the layer released the image the source dataset still owns. Adds the dataset menu test ids the DICOM browser already carries, and moves the MetaImage fixture writer next to the other manifest helpers so the zero spacing spec can share it. --- src/components/ImageDataBrowser.vue | 9 ++- tests/specs/layer-source-lifetime.e2e.ts | 74 ++++++++++++++++++++++++ tests/specs/utils.ts | 27 +++++++++ tests/specs/zero-voxel-spacing.e2e.ts | 37 ++---------- 4 files changed, 114 insertions(+), 33 deletions(-) create mode 100644 tests/specs/layer-source-lifetime.e2e.ts diff --git a/src/components/ImageDataBrowser.vue b/src/components/ImageDataBrowser.vue index 113e5b2ad..b4d7b807b 100644 --- a/src/components/ImageDataBrowser.vue +++ b/src/components/ImageDataBrowser.vue @@ -288,11 +288,18 @@ export default defineComponent({ Spacing: ({{ image.spacing.join(', ') }}) - +