fix: stack viewport colormap preserve and 4d local improved support#2679
Merged
Conversation
…uring actor recreation - Updated _setPropertiesFromCache to retrieve additional properties (colormap, sharpening, smoothing) from cache. - Modified logic in getCameraCPU to ensure stack properties are maintained when actor recreation is triggered by data type mismatches. - Added unit test to verify preservation of stack properties under specific conditions. - Enhanced fakeImageLoader to support multiple data types for testing purposes.
…rmats - Updated `generateFrameImageId` to support DICOMweb and local loader image IDs, allowing for frame number replacement and appending. - Improved error handling by removing the requirement for a specific "/frames/" pattern in the base image ID. - Added unit tests to verify the new functionality, including handling of query parameters and local file image IDs. - Enhanced `retrieveFrameParameterIndex` and `isLoaded` functions to utilize regex for frame parameter detection.
sedghi
changed the title
jbocce
requested changes
Mar 30, 2026
jbocce
left a comment
jbocce
left a comment
Collaborator
There was a problem hiding this comment.
Overall the changes seem fine. I tested and the colormap example no longer resets while navigating the stack. However, the two (formerly) non-readable dicom files are still non-readable.
Something to consider beyond the changes you made...
in retrieveMultiframeDataset.ts, generateMultiframeWADOURIs still appends &frame=${i} unconditionally. If the base URI does not already contain query params, that may produce malformed IDs. Might be worth normalizing with ? vs & there as well.
jbocce
approved these changes
Apr 13, 2026
jbocce
left a comment
jbocce
left a comment
Collaborator
There was a problem hiding this comment.
The code is approved. However the OHIF downstream validation is failing.
Resolve conflict in packages/core/src/utilities/splitImageIdsBy4DTags.ts: main moved the implementation into @cornerstonejs/metadata (core file is now a deprecated re-export shim). Ported the PR's generateFrameImageId enhancement (query-param + local-file frame addressing) into the new metadata home.
📝 WalkthroughWalkthrough
ChangesStackViewport Property Preservation
Local imageId 4D Frame Query Parameter Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dicomImageLoader/src/imageLoader/wadouri/retrieveMultiframeDataset.ts (1)
29-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve non-frame query params when
frameis first in the query string.With the new
?[&]frame=support, URIs like... ?frame=2&token=abcnow match, butmultiframeURI = uri.slice(0, frameParameterIndex)dropstoken=abc. That can breakloadedDataSetslookup for valid signed/query-bearing URIs.Suggested fix
function retrieveMultiframeDataset(uri) { - const frameParameterIndex = retrieveFrameParameterIndex(uri); - const multiframeURI = - frameParameterIndex === -1 ? uri : uri.slice(0, frameParameterIndex); - const frame = parseInt(uri.slice(frameParameterIndex + 7), 10) || 1; + const [baseURI, queryString = ''] = uri.split('?'); + const params = queryString ? queryString.split('&') : []; + const frameParam = params.find((p) => p.startsWith('frame=')); + const frame = parseInt(frameParam?.slice('frame='.length), 10) || 1; + const remainingParams = params.filter((p) => !p.startsWith('frame=')); + const multiframeURI = remainingParams.length + ? `${baseURI}?${remainingParams.join('&')}` + : baseURI;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dicomImageLoader/src/imageLoader/wadouri/retrieveMultiframeDataset.ts` around lines 29 - 38, The issue is that when frame is the first query parameter (e.g., ?frame=2&token=abc), using uri.slice(0, frameParameterIndex) truncates the URI and loses subsequent query parameters. Instead of slicing at the match index, extract and remove only the frame parameter portion (including its separator) while preserving other query parameters. In the retrieveMultiframeDataset function, replace the uri.slice approach with logic that identifies the full frame parameter string (with its ? or & prefix and value) and removes only that portion from the URI, leaving the multiframeURI intact with all other query parameters preserved for proper loadedDataSets lookup.
🧹 Nitpick comments (2)
packages/core/src/RenderingEngine/StackViewport.ts (1)
1002-1008: ⚡ Quick winAvoid unconditional re-renders while restoring sharpening/smoothing cache.
getProperties()always returns numericsharpening/smoothing, so both branches always execute and each setter triggersrender()during cache restore, even when values did not change. This adds unnecessary render work in actor re-init paths.♻️ Proposed change
- if (typeof sharpening !== 'undefined') { - this.setSharpening(sharpening); - } - - if (typeof smoothing !== 'undefined') { - this.setSmoothing(smoothing); - } + if (typeof sharpening !== 'undefined' && this.sharpening !== sharpening) { + this.sharpening = sharpening; + } + + if (typeof smoothing !== 'undefined' && this.smoothing !== smoothing) { + this.smoothing = smoothing; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/RenderingEngine/StackViewport.ts` around lines 1002 - 1008, The setSharpening and setSmoothing methods are being called unconditionally during cache restoration even when the values haven't changed, causing unnecessary render calls. Instead of checking only if the properties are defined, compare the current sharpening and smoothing values (via appropriate getter methods) with the incoming values and only call setSharpening and setSmoothing if the values have actually changed. This prevents triggering render work when restoring cached values that are identical to the current state.packages/core/test/stackViewport_gpu_render_test.js (1)
896-913: ⚡ Quick winExtend this regression test to cover all newly restored properties.
The implementation now restores
VOILUTFunction,sharpening, andsmoothingtoo, but this test currently only checks colormap/interpolation/VOI/invert. Adding those assertions will lock in the full contract of this PR path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/stackViewport_gpu_render_test.js` around lines 896 - 913, The regression test in the then() block currently only asserts that colormap, interpolationType, voiRange, and invert properties are restored after calling setImageIdIndex(1), but the implementation now also restores VOILUTFunction, sharpening, and smoothing properties. Add three additional expect statements in the then() block after the existing assertions to verify that props.VOILUTFunction, props.sharpening, and props.smoothing all match their expected values. This ensures the full contract of the property restoration behavior is validated by the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@utils/test/testUtilsImageLoader.js`:
- Around line 190-193: The highBit property in the fake imagePixelModule
metadata is incorrectly set to bitsAllocated when it should represent the
zero-indexed position of the highest bit. In the test utility where
bitsAllocated, bitsStored, and highBit are defined together, change highBit from
bitsAllocated to bitsAllocated - 1 to correctly represent the bit index (for
example, 8-bit should have highBit of 7, not 8).
---
Outside diff comments:
In
`@packages/dicomImageLoader/src/imageLoader/wadouri/retrieveMultiframeDataset.ts`:
- Around line 29-38: The issue is that when frame is the first query parameter
(e.g., ?frame=2&token=abc), using uri.slice(0, frameParameterIndex) truncates
the URI and loses subsequent query parameters. Instead of slicing at the match
index, extract and remove only the frame parameter portion (including its
separator) while preserving other query parameters. In the
retrieveMultiframeDataset function, replace the uri.slice approach with logic
that identifies the full frame parameter string (with its ? or & prefix and
value) and removes only that portion from the URI, leaving the multiframeURI
intact with all other query parameters preserved for proper loadedDataSets
lookup.
---
Nitpick comments:
In `@packages/core/src/RenderingEngine/StackViewport.ts`:
- Around line 1002-1008: The setSharpening and setSmoothing methods are being
called unconditionally during cache restoration even when the values haven't
changed, causing unnecessary render calls. Instead of checking only if the
properties are defined, compare the current sharpening and smoothing values (via
appropriate getter methods) with the incoming values and only call setSharpening
and setSmoothing if the values have actually changed. This prevents triggering
render work when restoring cached values that are identical to the current
state.
In `@packages/core/test/stackViewport_gpu_render_test.js`:
- Around line 896-913: The regression test in the then() block currently only
asserts that colormap, interpolationType, voiRange, and invert properties are
restored after calling setImageIdIndex(1), but the implementation now also
restores VOILUTFunction, sharpening, and smoothing properties. Add three
additional expect statements in the then() block after the existing assertions
to verify that props.VOILUTFunction, props.sharpening, and props.smoothing all
match their expected values. This ensures the full contract of the property
restoration behavior is validated by the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65601f7e-8294-4175-8e13-99a88f00238e
📒 Files selected for processing (7)
packages/core/src/RenderingEngine/StackViewport.tspackages/core/test/stackViewport_gpu_render_test.jspackages/core/test/utilities/splitImageIdsBy4DTags.jest.jspackages/dicomImageLoader/src/imageLoader/wadouri/dataSetCacheManager.tspackages/dicomImageLoader/src/imageLoader/wadouri/retrieveMultiframeDataset.tspackages/metadata/src/utilities/splitImageIdsBy4DTags.tsutils/test/testUtilsImageLoader.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In stack viewport if we have different types of data colormap change reset on slice changes
4d for local drag and drop does not have good support
Ask alireza for data
Summary by CodeRabbit
New Features
Bug Fixes
Tests