[WIP] feat/decimated loading#6034
Open
mbellehumeur wants to merge 62 commits into
Open
Conversation
…mands and components
…ds for sample distance multiplier and volume reload with decimation
…on logic for improved performance and memory management
…h parsing from volumeId for improved clarity and maintainability
…nfig threshold and refactoring related functions
…s and display sets
…nt to use AllInOneMenu for improved layout and organization
…nality and update related components for improved clarity
… in display sets and enhance event broadcasting
…etrieve display set from volume ID
…umeLoader format and update IJK decimation parsing to handle baseVolumeId with colons
…cessary nesting and improving readability
…e volume ID generation logic for improved clarity and performance
…ommented code and unnecessary whitespace for improved readability. Add doc.
… and remove unused import for improved clarity
…RenderingPresets components by integrating props instead of hooks for improved flexibility and clarity
…ecimation messages in the viewport, enhancing user feedback on volume rendering changes
…omizationService for volume decimation settings, improving configuration flexibility and clarity
…bling volume-specific options and enhancing user interaction with volume rendering settings
…CompassMenu component for enhanced 3D viewport orientation control
…port3DCompassMenu with spin controls for improved 3D viewport interaction
…r center sphere and adjust layout for spin controls
…sMenu for accurate 3D viewport interaction
…nto VolumeOptionsMenu for enhanced volume manipulation features
… and integrate GPU performance testing utility for improved volume rendering settings
…stone tools and integrate it into volume3D tool group with configuration options
…d lerna for improved project management
…playSetFromVolume for improved clarity and functionality
… settings and broadcast updated viewport data
…g default volume options
…nd adjust default volume options
…js to netlify.js for improved configuration management
…lehumeur/Viewers into feat/decimatedVolumeLoading
…texture management
… is fixed in CS3D.
Co-authored-by: Dan Rukas <dan.rukas@gmail.com>
…ys.tsx Co-authored-by: Dan Rukas <dan.rukas@gmail.com>
…ys.tsx Co-authored-by: Dan Rukas <dan.rukas@gmail.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
❌ Deploy Preview for ohif-dev failed. Why did it fail? →
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ving redundant conditions
…ts and simplify volume options
…r and enhance overlay visibility logic
…image sets and improve clarity of data source handling
📝 WalkthroughWalkthroughThis PR adds GPU-adaptive volume decimation (automatic IJK downsampling based on WebGL benchmarks and voxel-count thresholds), a new Changes3D Volume Rendering: Decimation, SmartScrollbar & Viewport Controls
Sequence Diagram(s)sequenceDiagram
participant App as init.tsx
participant GPU as GPUPerformanceTester
participant Cache as CornerstoneCacheService
participant DL as decimatedVolumeLoader
participant Cmd as commandsManager
participant VP as OHIFCornerstoneViewport
App->>GPU: gpuPerformanceTest(appConfig)
GPU-->>App: setVolumeOptions(rotateSampleDistanceFactor, sampleDistanceMultiplier)
App->>DL: registerVolumeLoader('decimatedVolumeLoader')
VP->>Cache: createViewportData(displaySets, viewportOptions)
Cache->>Cache: applyAutoDecimationIfNecessary → enrichedViewportOptions + ijkDecimation
Cache->>DL: createAndCacheVolume('decimatedVolumeLoader:...', {ijkDecimation})
Cache-->>VP: VolumeViewportData {viewportOptions: enriched}
VP->>VP: optionsToUse = viewportData.viewportOptions
Note over VP: Decimation overlay message shown if threshold exceeded
Cmd->>Cmd: reloadVolumeWithDecimation({ijkDecimation})
Cmd->>Cache: remove old volume, create new decimated volume
Cmd->>VP: restore camera + preset, render, broadcast VIEWPORT_DATA_CHANGED
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 unit tests (beta)
⚔️ Resolve merge conflicts
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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
extensions/cornerstone/src/services/CornerstoneCacheService/CornerstoneCacheService.ts (1)
146-150: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve viewport options during invalidation reloads.
This path calls
_getVolumeViewportDatawithoutviewportData.viewportOptions, so auto/manualijkDecimationandautoDecimationInfoare dropped after metadata invalidation.Proposed fix
const newViewportData = await this._getVolumeViewportData( dataSource, displaySets, - viewportData.viewportType + viewportData.viewportType, + viewportData.viewportOptions );🤖 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 `@extensions/cornerstone/src/services/CornerstoneCacheService/CornerstoneCacheService.ts` around lines 146 - 150, The invalidation reload path in CornerstoneCacheService drops viewport-specific options because _getVolumeViewportData is called without viewportData.viewportOptions. Update the reload flow to pass the existing viewportOptions through this call so auto/manual ijkDecimation and autoDecimationInfo are preserved when rebuilding the viewport data.extensions/cornerstone/src/Viewport/OHIFCornerstoneViewport.tsx (1)
284-304: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude decimation options in the memo comparison.
optionsToUsenow makesijkDecimation/autoDecimationInfoaffect viewport data, butareEqualbelow does not compare those fields. Prop-only decimation changes can be memo-skipped and never reachsetViewportData.Proposed fix
function areEqual(prevProps, nextProps) { + if ( + prevProps.viewportOptions.ijkDecimation?.join('_') !== + nextProps.viewportOptions.ijkDecimation?.join('_') + ) { + return false; + } + + if ( + prevProps.viewportOptions.autoDecimationInfo?.message !== + nextProps.viewportOptions.autoDecimationInfo?.message + ) { + return false; + } + if (nextProps.needsRerendering) { return false; }🤖 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 `@extensions/cornerstone/src/Viewport/OHIFCornerstoneViewport.tsx` around lines 284 - 304, `optionsToUse` now depends on decimation settings, but the memoization check in `areEqual` does not include `ijkDecimation` and `autoDecimationInfo`, so prop-only decimation updates can be skipped. Update `areEqual` in `OHIFCornerstoneViewport` to compare those fields alongside the existing viewport props, ensuring changes force `setViewportData` to run when decimation options change.modes/basic/src/initToolGroups.ts (1)
296-338: 🎯 Functional Correctness | 🟠 MajorResolve conflicting mouse bindings between
VolumeCroppingandTrackballRotateTool.Both tools are configured as
activein the same tool group usingEnums.MouseBindings.Primary. In this configuration, theVolumeCroppingtool (listed first) will capture all primary mouse interactions, rendering theTrackballRotateToolunresponsive to mouse dragging.Remove the active
Primarybinding fromTrackballRotateToolor change it to a modifier key if rotation is intended alongside cropping.Confirmed Issue Context
const tools = { active: [ // VolumeCropping captures Primary bindings first { toolName: 'VolumeCropping', bindings: [ { mouseButton: Enums.MouseBindings.Primary }, // Conflict point ... ], }, // TrackballRotateTool's Primary binding is effectively ignored { toolName: toolNames.TrackballRotateTool, bindings: [{ mouseButton: Enums.MouseBindings.Primary }], // Never receives event }, ... ], };🤖 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 `@modes/basic/src/initToolGroups.ts` around lines 296 - 338, The active tool bindings in initToolGroups.ts have a mouse conflict: VolumeCropping and TrackballRotateTool both use Enums.MouseBindings.Primary, so the first tool wins and rotation becomes unreachable. Update the tools.active configuration by removing the plain Primary binding from toolNames.TrackballRotateTool or moving it behind a modifier key, while keeping VolumeCropping on Primary, so both tools can be used without shadowing each other.
🧹 Nitpick comments (5)
extensions/cornerstone/src/types/ViewportPresets.ts (1)
55-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider tightening
dimensionsto a 3-tuple for consistency.
dimensionsis typed asnumber[]whileijkDecimationandoriginalDecimationare[number, number, number]. Ifdimensionsalways represents IJK extents, aPoint3/[number, number, number]type would prevent malformed shapes and align with the decimation tuples in this same type.🤖 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 `@extensions/cornerstone/src/types/ViewportPresets.ts` around lines 55 - 60, The VolumeDecimationInfo type is using an overly broad dimensions array, which should be narrowed for consistency with the other 3-value tuples in this type. Update the dimensions property in VolumeDecimationInfo to use a 3-tuple/Point3-style type, matching ijkDecimation and appliedDecimation.originalDecimation, so the shape is fixed and cannot accept malformed lengths.platform/core/src/utils/index.ts (1)
52-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent export surface: helpers added to named exports but not the
utilsobject.
getVolumeOptions/setVolumeOptionsare re-exported as named exports but omitted from theutilsaggregate object (lines 60–112), unlike the surrounding utilities. Consumers usingOHIF.utils.getVolumeOptionswould getundefined. If only named-import access is intended this is fine; otherwise add them toutilsfor consistency.Also applies to: 154-155
🤖 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 `@platform/core/src/utils/index.ts` around lines 52 - 55, The utils export surface is inconsistent because getVolumeOptions and setVolumeOptions are only re-exported as named exports from index.ts and are missing from the utils aggregate object. Update the utils object in index.ts to include these volumeOptionsStorage helpers alongside the other utility entries so OHIF.utils.getVolumeOptions and OHIF.utils.setVolumeOptions resolve correctly; use the existing getVolumeOptions/setVolumeOptions imports and keep the named exports intact.extensions/cornerstone/src/components/ViewportOrientationMenu/Viewport3DCompassMenu.tsx (1)
26-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSpin UI state can desync from the store on viewport switch.
spinis seeded once fromgetVolume3dSpin(viewportIdToUse)via theuseStateinitializer, but it never re-reads whenviewportIdToUsechanges (e.g., active viewport switches) or when the store is mutated elsewhere. The popover would then display/animate a stale value for the newly-targeted viewport. Consider reading viauseSyncExternalStore(asVolume3dSpinDriverdoes) so the UI reflects the authoritative store value.🤖 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 `@extensions/cornerstone/src/components/ViewportOrientationMenu/Viewport3DCompassMenu.tsx` around lines 26 - 29, The Compass menu’s local `spin` state is initialized once in `Viewport3DCompassMenu` from `getVolume3dSpin(viewportIdToUse)` and can become stale when `viewportIdToUse` changes or the store updates elsewhere. Update this component to read the spin value from the authoritative store on every relevant change, ideally by mirroring `Volume3dSpinDriver` with `useSyncExternalStore` (or equivalent subscription-based state) keyed by the current viewport id. Keep the UI bound to the current viewport’s store value instead of the one-time `useState` initializer.extensions/cornerstone/src/components/ViewportOrientationMenu/ViewportOrientationMenuWrapper.tsx (2)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDestructure the service from
servicesManager.services.Access the service via destructuring to follow the established pattern instead of chained property access.
♻️ Proposed refactor
- const { servicesManager } = useSystem(); - const viewportInfo = servicesManager.services.cornerstoneViewportService.getViewportInfo( - viewportId - ); + const { servicesManager } = useSystem(); + const { cornerstoneViewportService } = servicesManager.services; + const viewportInfo = cornerstoneViewportService.getViewportInfo(viewportId);As per coding guidelines: "access via
const { ServiceName } = servicesManager.services".🤖 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 `@extensions/cornerstone/src/components/ViewportOrientationMenu/ViewportOrientationMenuWrapper.tsx` around lines 21 - 24, The `ViewportOrientationMenuWrapper` is accessing `cornerstoneViewportService` through chained property lookup instead of the standard destructuring pattern. Update the service lookup in this component by destructuring `cornerstoneViewportService` from `servicesManager.services` before calling `getViewportInfo`, matching the established pattern used elsewhere and keeping the service reference easy to locate.Source: Coding guidelines
26-28: 📐 Maintainability & Code Quality | 🔵 TrivialRemove redundant string literal fallback
Enums.ViewportType.VOLUME_3Dis defined as the string'volume3d', making the explicit string comparison redundant.🤖 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 `@extensions/cornerstone/src/components/ViewportOrientationMenu/ViewportOrientationMenuWrapper.tsx` around lines 26 - 28, Remove the redundant fallback string check in ViewportOrientationMenuWrapper by relying on Enums.ViewportType.VOLUME_3D alone when computing isVolume3D. Update the condition in the viewport type comparison so it only uses the enum constant, since it already resolves to the same value and this keeps the logic centralized and easier to maintain.
🤖 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 `@extensions/cornerstone/src/commandsModule.ts`:
- Around line 3174-3179: The `definitions` object in `commandsModule.ts`
contains duplicate entries for `setViewport3DViewDirection` and
`rotateViewport3DBy`, with this object-form block being overwritten by the later
raw-function definitions. Remove one of the two definitions for each
command—prefer keeping the intended single form in `definitions`—so there is
only one entry per command and the dead code/lint conflict is eliminated.
- Around line 1768-1806: The volume load and viewport restore flow is split
across separate error handling, so a rejection from
volumeLoader.createAndCacheVolume or later restoration can leave affected
viewports blank. Move the creation of newVolume, the load call, and the viewport
restoration loop into one unified try/catch around this whole sequence in
commandsModule.ts, using the existing cleanup/recovery path and
uiNotificationService to report failures. Keep the old-volume cleanup and cache
removal in the same recovery flow so the actors are re-added whenever loading or
restoration fails.
In `@extensions/cornerstone/src/components/VolumeOptionsMenu/VolumeOptions.tsx`:
- Around line 55-104: The volume state in VolumeOptions.tsx is only recalculated
on viewportId/service changes, so it misses reloads when the underlying volume
is swapped in place. Update the VolumeOptions component’s useEffect around
getCornerstoneViewport and cs3DCache.getVolume to also react to volume updates
by subscribing to cornerstoneViewportService’s VIEWPORT_DATA_CHANGED event, or
otherwise depend on the current volumeId if it changes on reload. Make sure the
effect reruns the existing dimension/decimation recalculation and cleanup is
handled when the subscription is removed.
In `@extensions/cornerstone/src/constants/index.ts`:
- Line 2: The default volume loader scheme is causing double-prefixed decimated
IDs and breaking loader resolution. Update the fallback in
CornerstoneCacheService and the decimated ID construction logic so the generated
baseVolumeId and the decimatedVolumeLoader ID are not prefixed twice; use a
standard scheme for the default case or make the prefixing conditional in the
code paths around VOLUME_LOADER_SCHEME and the decimated ID builder.
In `@extensions/cornerstone/src/init.tsx`:
- Around line 71-96: The startup guard in init.tsx is treating the synthetic
“skipped” gpuTestResults from the Cypress/CPU path as a completed probe, which
blocks later real benchmarking. Update the logic around getVolumeOptions and
setVolumeOptions so skipped results are distinguishable from actual GPU test
output, and only bypass gpuPerformanceTest when a genuine benchmark has already
been recorded; keep the existing gpuPerformanceTest(appConfig) path available
when rendering mode changes.
In `@extensions/cornerstone/src/initCornerstoneTools.js`:
- Around line 191-193: The OrientationControllerTool mapping in
initCornerstoneTools still unconditionally reads toolName, which can throw if
the tool is missing despite the earlier guard. Update the
OrientationControllerTool entry to use optional chaining (matching the defensive
check around OrientationControllerTool) so module initialization won’t crash
when the export is absent or incompatible.
In
`@extensions/cornerstone/src/services/CornerstoneCacheService/CornerstoneCacheService.ts`:
- Around line 116-138: Invalidate decimated volume cache entries alongside the
existing base and suffixed volume IDs in CornerstoneCacheService’s invalidation
flow. Update the candidate ID collection in the volume invalidation logic to
also derive and delete the decimated cache keys used by decimatedVolumeLoader
for the same baseVolumeId variants, so stale decimated volumes are removed from
cs3DCache._volumeCache and any related image load objects are cleaned up
consistently.
- Around line 326-354: The volume caching logic in CornerstoneCacheService
should resolve the decimated volume ID before the cache check, because the
current `(!volumeImageIds || !volume)` guard can skip the decimated-loader path
when a base volume already exists and cause full-resolution reuse. Update the
flow around the volume lookup in the cache creation branch so the requested
`ijkDecimation` is computed first, then derive and check the appropriate
`volumeId` (including the decimated `decimatedVolumeLoader:` variant) before
deciding whether to reuse or create the volume. Focus on the logic in the volume
creation block that uses `volumeImageIds`, `volumeId`, and `useDecimatedLoader`.
In
`@extensions/cornerstone/src/utils/decimation/applyAutoDecimationIfNecessary.ts`:
- Around line 200-217: The auto-decimation check in
applyAutoDecimationIfNecessary is short-circuiting too early because it uses
displaySets.some(), so maxVoxelCount may not reflect the largest volume in the
viewport. Update the logic to iterate over all display sets in
applyAutoDecimationIfNecessary, still tracking the maximum voxel count from
estimateDisplaySetVoxelCount, and then decide whether decimation is needed after
the full scan rather than inside the loop.
In `@extensions/cornerstone/src/utils/decimation/constants.ts`:
- Line 3: Update AUTO_DECIMATION_VIEWPORT_TYPES in the decimation constants so
it matches the viewportType values returned by resolveViewportType; replace the
incorrect 'volume' entry with 'orthographic' so
AUTO_DECIMATION_VIEWPORT_TYPES.has(viewportType) works for orthographic
viewports and auto-decimation runs as intended.
In `@extensions/cornerstone/src/utils/gpuPerformanceTest.ts`:
- Around line 528-553: The GPU sampling tier logic is misaligned with the
benchmark thresholds, causing `getGpuTier()` and `getSamplingDefaults()` to
assign the wrong defaults for most fast GPUs. Update `getGpuTier()` in
`gpuPerformanceTest.ts` to use the same cutoff values as `getPerformanceLevel()`
(for example, the `80+` and `60+` ranges) so `high`, `mid`, and `low` match the
benchmark scoring, and keep the tier-to-default mapping in
`getSamplingDefaults()` consistent with that corrected tiering.
In `@extensions/cornerstone/src/Viewport/Overlays/AutoDecimationOverlay.tsx`:
- Around line 15-27: `AutoDecimationOverlay` is reading `autoDecimationInfo`
imperatively, so it won’t re-render when `reloadVolumeWithDecimation` updates
viewport options. Update the component to subscribe to
`cornerstoneViewportService.EVENTS.VIEWPORT_DATA_CHANGED` (or make the options
source reactive) so changes to
`getViewportInfo(viewportId).getViewportOptions()` trigger a render. Keep the
existing `useViewportRendering(viewportId)` usage, but ensure the overlay’s
message visibility is driven by `autoDecimationInfo`, `viewportType`, and the
new event subscription.
In
`@extensions/cornerstone/src/Viewport/Overlays/ViewportSliceProgressScrollbar/helpers.ts`:
- Around line 4-7: getImageIndexFromEvent currently destructures event.detail
directly, which can throw if detail is missing. Update this helper to
defensively read from event?.detail, matching the pattern used by
getImageIdFromCacheEvent, and return undefined when no detail payload is
present. Keep the existing fallback logic for imageIndex, newImageIdIndex, and
imageIdIndex inside the guarded detail access.
In `@extensions/default/src/Panels/StudyBrowser/PanelStudyBrowser.tsx`:
- Around line 492-493: The StudyBrowser panel is using ds.numImageFrames
directly for numInstances, which can leave the value undefined when frame
metadata is missing. Update the PanelStudyBrowser assignment to match the
tracking panel’s fallback behavior by using ds.numImageFrames ??
ds.instances?.length, keeping the numInstances handling consistent and resilient
across PanelStudyBrowser and PanelStudyBrowserTracking.
In `@platform/core/src/utils/volumeOptionsStorage.ts`:
- Around line 42-46: `setVolumeOptions` currently writes to `localStorage`
without protection, so startup flows that call it can fail if storage is
unavailable or full. Update `setVolumeOptions` in `volumeOptionsStorage` to
mirror the defensive pattern used by `getVolumeOptions`: wrap the
merge-and-`localStorage.setItem` path in try/catch, and safely no-op or log on
failure so GPU probe startup from `init.tsx`/`gpuPerformanceTest.ts` cannot
throw an unhandled error. Keep the behavior localized to `setVolumeOptions` and
preserve the existing `getVolumeOptions` fallback semantics.
In `@platform/docs/docs/configuration/configurationFiles.md`:
- Line 136: The documentation entry for volumeAutoDecimationThreshold
incorrectly claims a default is defined in config/default.js. Update the text in
configurationFiles.md to remove that file reference or replace it with the
correct source of truth, and make sure the description matches the actual
behavior exposed by volumeAutoDecimationThreshold rather than attributing a
default value to default.js.
In `@platform/i18n/src/locales/test-LNG/WindowLevelActionMenu.json`:
- Line 10: The “Volume Options” localization key is missing from the ru and ar
locale sets, so add it to those locale JSON files to match the existing entries
in en-US, fr, nl, zh, and test-LNG. Use the same key name and add appropriate
translated values alongside the other WindowLevelActionMenu strings so the
locale coverage stays consistent across all supported languages.
In `@platform/ui-next/src/components/SmartScrollbar/SmartScrollbar.tsx`:
- Around line 178-189: The SmartScrollbar state logic keeps the track expanded
when the view never enters loading, because `wasEverLoading.current` stays false
and `hasSettled` never flips back. Update the `SmartScrollbar` component so
`hasSettled` starts as true by default, while still letting the `useEffect`
reset it to false during loading and settle it after loading in the existing
`isLoading` flow. This will let `isExpanded` and `effectiveWidth` contract
correctly for instantly loaded viewports.
- Around line 320-342: The SmartScrollbar slider is always focusable because
`tabIndex={0}` is unconditional, but `handleKeyDown` is only attached when
`enableKeyboardNavigation` is true, so `ViewportSliceProgressScrollbar` can
expose a non-operable keyboard target. Update the `SmartScrollbar` component so
the tab stop is gated by the same `enableKeyboardNavigation` flag used for
`onKeyDown`, keeping the slider focusable only when keyboard interaction is
actually supported.
---
Outside diff comments:
In
`@extensions/cornerstone/src/services/CornerstoneCacheService/CornerstoneCacheService.ts`:
- Around line 146-150: The invalidation reload path in CornerstoneCacheService
drops viewport-specific options because _getVolumeViewportData is called without
viewportData.viewportOptions. Update the reload flow to pass the existing
viewportOptions through this call so auto/manual ijkDecimation and
autoDecimationInfo are preserved when rebuilding the viewport data.
In `@extensions/cornerstone/src/Viewport/OHIFCornerstoneViewport.tsx`:
- Around line 284-304: `optionsToUse` now depends on decimation settings, but
the memoization check in `areEqual` does not include `ijkDecimation` and
`autoDecimationInfo`, so prop-only decimation updates can be skipped. Update
`areEqual` in `OHIFCornerstoneViewport` to compare those fields alongside the
existing viewport props, ensuring changes force `setViewportData` to run when
decimation options change.
In `@modes/basic/src/initToolGroups.ts`:
- Around line 296-338: The active tool bindings in initToolGroups.ts have a
mouse conflict: VolumeCropping and TrackballRotateTool both use
Enums.MouseBindings.Primary, so the first tool wins and rotation becomes
unreachable. Update the tools.active configuration by removing the plain Primary
binding from toolNames.TrackballRotateTool or moving it behind a modifier key,
while keeping VolumeCropping on Primary, so both tools can be used without
shadowing each other.
---
Nitpick comments:
In
`@extensions/cornerstone/src/components/ViewportOrientationMenu/Viewport3DCompassMenu.tsx`:
- Around line 26-29: The Compass menu’s local `spin` state is initialized once
in `Viewport3DCompassMenu` from `getVolume3dSpin(viewportIdToUse)` and can
become stale when `viewportIdToUse` changes or the store updates elsewhere.
Update this component to read the spin value from the authoritative store on
every relevant change, ideally by mirroring `Volume3dSpinDriver` with
`useSyncExternalStore` (or equivalent subscription-based state) keyed by the
current viewport id. Keep the UI bound to the current viewport’s store value
instead of the one-time `useState` initializer.
In
`@extensions/cornerstone/src/components/ViewportOrientationMenu/ViewportOrientationMenuWrapper.tsx`:
- Around line 21-24: The `ViewportOrientationMenuWrapper` is accessing
`cornerstoneViewportService` through chained property lookup instead of the
standard destructuring pattern. Update the service lookup in this component by
destructuring `cornerstoneViewportService` from `servicesManager.services`
before calling `getViewportInfo`, matching the established pattern used
elsewhere and keeping the service reference easy to locate.
- Around line 26-28: Remove the redundant fallback string check in
ViewportOrientationMenuWrapper by relying on Enums.ViewportType.VOLUME_3D alone
when computing isVolume3D. Update the condition in the viewport type comparison
so it only uses the enum constant, since it already resolves to the same value
and this keeps the logic centralized and easier to maintain.
In `@extensions/cornerstone/src/types/ViewportPresets.ts`:
- Around line 55-60: The VolumeDecimationInfo type is using an overly broad
dimensions array, which should be narrowed for consistency with the other
3-value tuples in this type. Update the dimensions property in
VolumeDecimationInfo to use a 3-tuple/Point3-style type, matching ijkDecimation
and appliedDecimation.originalDecimation, so the shape is fixed and cannot
accept malformed lengths.
In `@platform/core/src/utils/index.ts`:
- Around line 52-55: The utils export surface is inconsistent because
getVolumeOptions and setVolumeOptions are only re-exported as named exports from
index.ts and are missing from the utils aggregate object. Update the utils
object in index.ts to include these volumeOptionsStorage helpers alongside the
other utility entries so OHIF.utils.getVolumeOptions and
OHIF.utils.setVolumeOptions resolve correctly; use the existing
getVolumeOptions/setVolumeOptions imports and keep the named exports intact.
🪄 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: 90369be4-028a-441b-8857-a4c71aeaa28d
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (75)
extensions/cornerstone/src/Viewport/OHIFCornerstoneViewport.tsxextensions/cornerstone/src/Viewport/Overlays/AutoDecimationOverlay.tsxextensions/cornerstone/src/Viewport/Overlays/CornerstoneOverlays.tsxextensions/cornerstone/src/Viewport/Overlays/CustomizableViewportOverlay.tsxextensions/cornerstone/src/Viewport/Overlays/ViewportSliceProgressScrollbar/ViewportSliceProgressScrollbar.tsxextensions/cornerstone/src/Viewport/Overlays/ViewportSliceProgressScrollbar/helpers.tsextensions/cornerstone/src/Viewport/Overlays/ViewportSliceProgressScrollbar/hooks.tsextensions/cornerstone/src/Viewport/Overlays/ViewportSliceProgressScrollbar/index.tsextensions/cornerstone/src/Viewport/Overlays/ViewportSliceProgressScrollbar/types.tsextensions/cornerstone/src/commandsModule.tsextensions/cornerstone/src/components/ViewportOrientationMenu/Viewport3DCompassMenu.tsxextensions/cornerstone/src/components/ViewportOrientationMenu/ViewportOrientationMenuWrapper.tsxextensions/cornerstone/src/components/ViewportOrientationMenu/Volume3dSpinDriver.tsxextensions/cornerstone/src/components/ViewportOrientationMenu/viewport3dSpinStore.tsextensions/cornerstone/src/components/VolumeOptionsMenu/VolumeCropping.tsxextensions/cornerstone/src/components/VolumeOptionsMenu/VolumeOptions.tsxextensions/cornerstone/src/components/VolumeOptionsMenu/VolumeOptionsMenuWrapper.tsxextensions/cornerstone/src/components/VolumeOptionsMenu/index.tsextensions/cornerstone/src/components/WindowLevelActionMenu/VolumeRenderingOptions.tsxextensions/cornerstone/src/components/WindowLevelActionMenu/VolumeRenderingPresets.tsxextensions/cornerstone/src/components/WindowLevelActionMenu/VolumeRenderingQuality.tsxextensions/cornerstone/src/components/WindowLevelActionMenu/WindowLevelActionMenu.tsxextensions/cornerstone/src/constants/index.tsextensions/cornerstone/src/customizations/viewportScrollbarCustomization.tsxextensions/cornerstone/src/getCustomizationModule.tsxextensions/cornerstone/src/getToolbarModule.tsxextensions/cornerstone/src/hooks/useViewportRendering.tsxextensions/cornerstone/src/index.tsxextensions/cornerstone/src/init.tsxextensions/cornerstone/src/initCornerstoneTools.jsextensions/cornerstone/src/services/CornerstoneCacheService/CornerstoneCacheService.tsextensions/cornerstone/src/services/ViewedDataService/ViewedDataService.tsextensions/cornerstone/src/services/ViewedDataService/index.tsextensions/cornerstone/src/services/ViewportService/Viewport.tsextensions/cornerstone/src/types/AppTypes.tsextensions/cornerstone/src/types/CornerstoneCacheService.tsextensions/cornerstone/src/types/ViewportPresets.tsextensions/cornerstone/src/utils/decimation/applyAutoDecimationIfNecessary.tsextensions/cornerstone/src/utils/decimation/constants.tsextensions/cornerstone/src/utils/gpuPerformanceTest.tsextensions/default/src/Panels/StudyBrowser/PanelStudyBrowser.tsxextensions/default/src/getSopClassHandlerModule.jsextensions/measurement-tracking/src/panels/PanelStudyBrowserTracking/PanelStudyBrowserTracking.tsxmodes/basic/src/index.tsxmodes/basic/src/initToolGroups.tsmodes/basic/src/toolbarButtons.tspackage.jsonplatform/app/public/config/default.jsplatform/app/public/config/netlify.jsplatform/core/src/index.tsplatform/core/src/services/ServicesManager.tsplatform/core/src/types/AppTypes.tsplatform/core/src/types/ViewportGridType.tsplatform/core/src/utils/index.tsplatform/core/src/utils/volumeOptionsStorage.tsplatform/docs/docs/configuration/configurationFiles.mdplatform/docs/docs/platform/services/ui/viewport-action-menu.mdplatform/i18n/src/locales/en-US/WindowLevelActionMenu.jsonplatform/i18n/src/locales/fr/WindowLevelActionMenu.jsonplatform/i18n/src/locales/nl/WindowLevelActionMenu.jsonplatform/i18n/src/locales/test-LNG/WindowLevelActionMenu.jsonplatform/i18n/src/locales/zh/WindowLevelActionMenu.jsonplatform/ui-next/src/components/SmartScrollbar/SmartScrollbar.tsxplatform/ui-next/src/components/SmartScrollbar/SmartScrollbarEndpoints.tsxplatform/ui-next/src/components/SmartScrollbar/SmartScrollbarFill.tsxplatform/ui-next/src/components/SmartScrollbar/SmartScrollbarIndicator.tsxplatform/ui-next/src/components/SmartScrollbar/SmartScrollbarTrack.tsxplatform/ui-next/src/components/SmartScrollbar/defaultSmartScrollbarIndicatorConfig.tsxplatform/ui-next/src/components/SmartScrollbar/index.tsplatform/ui-next/src/components/SmartScrollbar/useByteArray.tsplatform/ui-next/src/components/SmartScrollbar/utils.test.tsplatform/ui-next/src/components/SmartScrollbar/utils.tsplatform/ui-next/src/components/Thumbnail/Thumbnail.tsxplatform/ui-next/src/components/ThumbnailList/ThumbnailList.tsxplatform/ui-next/src/components/index.ts
💤 Files with no reviewable changes (1)
- platform/app/public/config/default.js
Comment on lines
+1768
to
+1806
| try { | ||
| const oldVolume = cache.getVolume(currentVolumeId); | ||
| if (oldVolume) { | ||
| if ( | ||
| typeof (oldVolume as { cancelLoading?: () => void }) | ||
| .cancelLoading === 'function' | ||
| ) { | ||
| (oldVolume as { cancelLoading: () => void }).cancelLoading(); | ||
| } | ||
| if ( | ||
| (oldVolume as { imageData?: { delete: () => void } }).imageData | ||
| ) { | ||
| (oldVolume as { imageData: { delete: () => void } }).imageData.delete(); | ||
| } | ||
| } | ||
| cache.removeVolumeLoadObject(currentVolumeId); | ||
| } catch (e) { | ||
| console.error('Failed to remove old volume from cache:', e); | ||
| } | ||
|
|
||
| const parts = currentVolumeId.split(':'); | ||
| const baseVolumeId = | ||
| parts[0] === 'decimatedVolumeLoader' && parts.length >= 3 | ||
| ? parts.slice(1, -1).join(':') | ||
| : currentVolumeId; | ||
| const decimationSuffix = decimationValues.join('_'); | ||
| const newVolumeId = `decimatedVolumeLoader:${baseVolumeId}:${decimationSuffix}`; | ||
|
|
||
| const newVolume = await volumeLoader.createAndCacheVolume(newVolumeId, { | ||
| imageIds: originalImageIds, | ||
| progressiveRendering: true, | ||
| ijkDecimation: decimationValues, | ||
| }); | ||
| const volumeWithLoad = newVolume as unknown as { | ||
| load?: () => void | Promise<void>; | ||
| }; | ||
| if (typeof volumeWithLoad.load === 'function') { | ||
| await Promise.resolve(volumeWithLoad.load()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
@cornerstonejs/core 4.20.0 volumeLoader.createAndCacheVolume rejection behavior
💡 Result:
In Cornerstone.js (@cornerstonejs/core), the createAndCacheVolume function is an asynchronous operation that returns a Promise [1][2]. The behavior regarding rejection is as follows: 1. Promise Rejection: The promise returned by createAndCacheVolume will reject if an error occurs during the volume loading process [1][2]. This includes scenarios where the underlying volume loader fails to fetch or initialize the volume data (e.g., due to network issues, invalid image IDs, or processing errors) [1][2][3]. 2. Input Validation: The function explicitly checks if the provided volumeId is undefined at the beginning of its execution. If it is, it throws an Error synchronously: "createAndCacheVolume: parameter volumeId must not be undefined" [1][2]. 3. Caching Behavior: The function first checks the cache using cache.getVolumeLoadObject(volumeId). If a volume load object already exists for that ID, it returns the existing promise, which may already be resolved or rejected [1][2]. If not, it triggers the loading process via the registered volume loader, caches the resulting load object, and returns its promise [1][2]. Because this is a Promise-based API, you should handle rejections using standard JavaScript patterns, such as a.catch block or a try-catch statement within an async function [4][5]: try { const volume = await volumeLoader.createAndCacheVolume(volumeId, options); volume.load; } catch (error) { console.error("Failed to create or cache volume:", error); } Note: If you encounter specific errors during loading, such as "Array buffer allocation failed" or "attempt to read past end of buffer," these typically indicate issues with the data payload, browser memory limits, or incompatible DICOM transfer syntaxes, rather than the rejection logic of the createAndCacheVolume function itself [6][7].
Citations:
- 1: https://github.com/cornerstonejs/cornerstone3D/blob/2b17e979df54cdc1fd91df95141feaa273a29de7/packages/core/src/loaders/volumeLoader.ts
- 2: https://www.cornerstonejs.org/coverage/core/src/loaders/volumeloader.ts
- 3: https://v1.cornerstonejs.org/api/core/namespace/volumeLoader/
- 4: https://cornerstonejs.org/docs/tutorials/basic-volume
- 5: https://github.com/cornerstonejs/cornerstone3D/blob/b922cdc85eba1e60b7a7bb6cb7d7309921aefcbe/packages/core/examples/volumeBasicWadoUri/index.ts
- 6: [Bug] Couldn't load a volume with wadouri - React + Vite TS cornerstonejs/cornerstone3D#1478
- 7: [Bug] When loading the imageId, I encountered the error: RangeError: Array buffer allocation failed cornerstonejs/cornerstone3D#1777
🏁 Script executed:
#!/usr/bin/env node
const fs = require('fs');
const path = require('path');
const fileBuffer = fs.readFileSync('extensions/cornerstone/src/commandsModule.ts', 'utf-8');
const lines = fileBuffer.split('\n');
let startLine = 1756; // 1-based index from original comment
const endLine = 1810;
// We need to extract the surrounding logic to see if the try-catch covers the entire "remove old -> create new -> restore" sequence.
// The original comment mentioned:
// 1. Actors removed (Lines 1756-1766)
// 2. Old volume removed (Line 1768)
// 3. New volume created/loaded (Line 1800+)
// Let's print the specific block from 1750 to 1810 to see the control flow.
const snippet = lines.slice(startLine - 1, endLine).join('\n');
console.log(`Snippet from lines ${startLine}-${endLine}:\n`);
console.log(snippet);Repository: OHIF/Viewers
Length of output: 245
🏁 Script executed:
node - <<'JS'
const fs = require('fs');
const path = require('path');
try {
const fileBuffer = fs.readFileSync('extensions/cornerstone/src/commandsModule.ts', 'utf-8');
const lines = fileBuffer.split('\n');
// We need to check the control flow.
// The review comment suggested that actors are removed (1756-1766),
// then volume removed (1768+), then create is called (1800+).
// We need to see if the create is inside a try block that wraps the whole operation.
const startLine = 1750;
const endLine = 1820;
console.log(`Code around lines ${startLine}-${endLine}:\n`);
for (let i = startLine - 1; i < Math.min(endLine, lines.length); i++) {
const num = (i + 1).toString().padEnd(5, ' ');
console.log(`${num} | ${lines[i]}`);
}
} catch (e) {
console.error("Error reading file:", e.message);
}
JSRepository: OHIF/Viewers
Length of output: 3300
Wrap volume creation and viewport restoration in a unified exception handler.
The current code removes actors from all affected viewports (Lines 1756-1766) and cleans up the old volume (Lines 1768-1786), but the await volumeLoader.createAndCacheVolume call (Line 1796) and the subsequent viewport restoration loop (Lines 1811+) fall outside the try/catch block.
Since createAndCacheVolume returns a Promise that rejects on failure (e.g., network errors, invalid data), any rejection will skip the restoration loop entirely, leaving the viewports blank.
Modify the code to wrap both the volume loading logic and the viewport restoration loop in a single try/catch block to ensure actors are re-added or an error is reported via uiNotificationService if loading fails.
🤖 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 `@extensions/cornerstone/src/commandsModule.ts` around lines 1768 - 1806, The
volume load and viewport restore flow is split across separate error handling,
so a rejection from volumeLoader.createAndCacheVolume or later restoration can
leave affected viewports blank. Move the creation of newVolume, the load call,
and the viewport restoration loop into one unified try/catch around this whole
sequence in commandsModule.ts, using the existing cleanup/recovery path and
uiNotificationService to report failures. Keep the old-volume cleanup and cache
removal in the same recovery flow so the actors are re-added whenever loading or
restoration fails.
Comment on lines
+3174
to
+3179
| setViewport3DViewDirection: { | ||
| commandFn: actions.setViewport3DViewDirection, | ||
| }, | ||
| rotateViewport3DBy: { | ||
| commandFn: actions.rotateViewport3DBy, | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate command definitions for setViewport3DViewDirection and rotateViewport3DBy.
Both keys are defined here (object { commandFn } form) and again at Lines 3318-3319 (raw-function form) inside the same definitions object. The later entries silently overwrite these, making this block dead code and tripping the lint error. Remove one set to avoid confusion.
Proposed fix (drop the duplicate object-form entries)
setSampleDistanceMultiplier: {
commandFn: actions.setSampleDistanceMultiplier,
},
- setViewport3DViewDirection: {
- commandFn: actions.setViewport3DViewDirection,
- },
- rotateViewport3DBy: {
- commandFn: actions.rotateViewport3DBy,
- },
setVolumeRenderingQuality: {
commandFn: actions.setVolumeRenderingQuality,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| setViewport3DViewDirection: { | |
| commandFn: actions.setViewport3DViewDirection, | |
| }, | |
| rotateViewport3DBy: { | |
| commandFn: actions.rotateViewport3DBy, | |
| }, | |
| setSampleDistanceMultiplier: { | |
| commandFn: actions.setSampleDistanceMultiplier, | |
| }, | |
| setVolumeRenderingQuality: { | |
| commandFn: actions.setVolumeRenderingQuality, | |
| }, |
🧰 Tools
🪛 Biome (2.5.0)
[error] 3174-3176: This property is later overwritten by an object member with the same name.
(lint/suspicious/noDuplicateObjectKeys)
[error] 3177-3179: This property is later overwritten by an object member with the same name.
(lint/suspicious/noDuplicateObjectKeys)
🤖 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 `@extensions/cornerstone/src/commandsModule.ts` around lines 3174 - 3179, The
`definitions` object in `commandsModule.ts` contains duplicate entries for
`setViewport3DViewDirection` and `rotateViewport3DBy`, with this object-form
block being overwritten by the later raw-function definitions. Remove one of the
two definitions for each command—prefer keeping the intended single form in
`definitions`—so there is only one entry per command and the dead code/lint
conflict is eliminated.
Source: Linters/SAST tools
Comment on lines
+55
to
+104
| useEffect(() => { | ||
| try { | ||
| const viewport = cornerstoneViewportService.getCornerstoneViewport(viewportId); | ||
| if (!viewport) return; | ||
| const actors = viewport.getActors(); | ||
| if (!actors?.length) return; | ||
|
|
||
| const volumeActor = actors.find( | ||
| (a: { actor?: { getClassName?: () => string }; referencedId?: string }) => | ||
| a?.actor?.getClassName?.() === 'vtkVolume' | ||
| ); | ||
| if (!volumeActor?.referencedId) return; | ||
|
|
||
| const volumeId = volumeActor.referencedId; | ||
| const volume = cs3DCache.getVolume(volumeId) as | ||
| | VolumeDecimationInfo | ||
| | undefined; | ||
| if (!volume?.dimensions || volume.dimensions.length < 3) return; | ||
|
|
||
| const dimensions = volume.dimensions; | ||
| const currentIjk = getIjkDecimationFromVolumeId(volumeId); | ||
| const meta = volume.originalMetadata; | ||
|
|
||
| const inPlane = Math.max(1, Math.min(currentIjk[0], MAX_IN_PLANE_DECIMATION)); | ||
| const kAxis = Math.max(1, Math.min(currentIjk[2], MAX_K_AXIS_DECIMATION)); | ||
| if (meta) { | ||
| const originalDims: [number, number, number] = [ | ||
| meta.Columns ?? dimensions[0], | ||
| meta.Rows ?? dimensions[1], | ||
| Math.floor(dimensions[2] * currentIjk[2]), | ||
| ]; | ||
| setVolumeDimensions(originalDims); | ||
| setCurrentInPlaneDecimation(inPlane); | ||
| setCurrentKAxisDecimation(kAxis); | ||
| } else { | ||
| const originalDims: [number, number, number] = [ | ||
| dimensions[0] * currentIjk[0], | ||
| dimensions[1] * currentIjk[1], | ||
| dimensions[2] * currentIjk[2], | ||
| ]; | ||
| setVolumeDimensions(originalDims); | ||
| setCurrentInPlaneDecimation(inPlane); | ||
| setCurrentKAxisDecimation(kAxis); | ||
| } | ||
| } catch { | ||
| setVolumeDimensions(null); | ||
| setCurrentInPlaneDecimation(1); | ||
| setCurrentKAxisDecimation(1); | ||
| } | ||
| }, [viewportId, cornerstoneViewportService]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find events emitted when a decimated volume is reloaded/swapped.
rg -nP "reloadVolumeWithDecimation|VIEWPORT_DATA_CHANGED" extensions/cornerstone/src/commandsModule.ts -A3 -B1Repository: OHIF/Viewers
Length of output: 1291
🏁 Script executed:
#!/bin/bash
# Read the implementation of reloadVolumeWithDecimation to verify if VIEWPORT_DATA_CHANGED is emitted there.
sed -n '1625,1700p' extensions/cornerstone/src/commandsModule.tsRepository: OHIF/Viewers
Length of output: 2649
🏁 Script executed:
#!/bin/bash
# Check if VolumeOptions.tsx subscribes to VIEWPORT_DATA_CHANGED or includes volumeId in deps.
rg -n "VIEWPORT_DATA_CHANGED\|SUBSCRIBE" extensions/cornerstone/src/components/VolumeOptionsMenu/VolumeOptions.tsx -A2 -B2Repository: OHIF/Viewers
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Check if the `cornerstoneViewportService` exposes the event emission mechanism used elsewhere to ensure we understand how to subscribe.
rg -n "eventsListenerService\|services.*events" extensions/cornerstone/src/components/VolumeOptionsMenu/VolumeOptions.tsx | head -20Repository: OHIF/Viewers
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Inspect imports and initialization in VolumeOptions.tsx to identify the available services and service manager
head -50 extensions/cornerstone/src/components/VolumeOptionsMenu/VolumeOptions.tsxRepository: OHIF/Viewers
Length of output: 2329
🏁 Script executed:
#!/bin/bash
# Verify if the component subscribes to events, specifically looking for useEventListener or similar patterns.
# Also check the specific lines around the useEffect to see the full context.
sed -n '50,110p' extensions/cornerstone/src/components/VolumeOptionsMenu/VolumeOptions.tsxRepository: OHIF/Viewers
Length of output: 2391
🏁 Script executed:
#!/bin/bash
# Check how other components interact with events (subscribe) to understand the pattern used in extension.
rg -n "subscribe\|VIEWPORT_DATA_CHANGED" extensions/cornerstone/src/components/ -A3 -B1 | head -60Repository: OHIF/Viewers
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Search for service subscription patterns in the cornerstone extension to see how events are typically handled.
rg -n "servicesManager.services" extensions/cornerstone/src/components/VolumeOptionsMenu/VolumeOptionsMenu.tsx extensions/cornerstone/src/components/VolumeOptionsMenu/VolumeOptions.tsx -A 10 -B 3Repository: OHIF/Viewers
Length of output: 1992
🏁 Script executed:
#!/bin/bash
# Verify how events are subscribed to in other parts of the extension, specifically looking for the event service or similar.
rg -n "subscribe" extensions/cornerstone/src/ --type ts -A 2 -B 1 | head -40Repository: OHIF/Viewers
Length of output: 3702
Effect does not update volume dimensions after a reload.
The useEffect dependency array [viewportId, cornerstoneViewportService] prevents the callback from re-running when the underlying volume is swapped or reloaded (e.g., via reloadVolumeWithDecimation). Since viewportId remains constant, the component continues to display the dimensions and decimation values calculated during the initial render, even though cs3DCache contains the updated volume data.
To fix this, subscribe to the VIEWPORT_DATA_CHANGED event from cornerstoneViewportService to trigger the calculation upon volume updates, or ensure the hook depends on the specific volumeId if it changes upon reload.
Current Effect Code
useEffect(() => {
try {
const viewport = cornerstoneViewportService.getCornerstoneViewport(viewportId);
if (!viewport) return;
// ... logic to get volume and set state
} catch {
// ... error handling
}
}, [viewportId, cornerstoneViewportService]);🤖 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 `@extensions/cornerstone/src/components/VolumeOptionsMenu/VolumeOptions.tsx`
around lines 55 - 104, The volume state in VolumeOptions.tsx is only
recalculated on viewportId/service changes, so it misses reloads when the
underlying volume is swapped in place. Update the VolumeOptions component’s
useEffect around getCornerstoneViewport and cs3DCache.getVolume to also react to
volume updates by subscribing to cornerstoneViewportService’s
VIEWPORT_DATA_CHANGED event, or otherwise depend on the current volumeId if it
changes on reload. Make sure the effect reruns the existing dimension/decimation
recalculation and cleanup is handled when the subscription is removed.
| // Volume loader schemes | ||
| export const VOLUME_LOADER_SCHEME = 'cornerstoneStreamingImageVolume'; | ||
| export const DYNAMIC_VOLUME_LOADER_SCHEME = 'cornerstoneStreamingDynamicImageVolume'; No newline at end of file | ||
| export const VOLUME_LOADER_SCHEME = 'decimatedVolumeLoader'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Confirm the loader is registered under this scheme
rg -nP "registerVolumeLoader\(\s*['\"]decimatedVolumeLoader" --type=ts -C2
# 2) Find all consumers of VOLUME_LOADER_SCHEME / DEFAULT_VOLUME_LOADER_SCHEME to assess fallback impact
rg -nP '\bVOLUME_LOADER_SCHEME\b' --type=ts -C2
rg -nP 'DEFAULT_VOLUME_LOADER_SCHEME\s*=' --type=ts -C1
# 3) Inspect where displaySet.volumeLoaderSchema is assigned (does it ever stay unset?)
rg -nP 'volumeLoaderSchema\s*[:=]' --type=ts --type=js -C1Repository: OHIF/Viewers
Length of output: 150
Fix the double-prefixed volume ID causing decimation to fail or load incorrect data.
Changing VOLUME_LOADER_SCHEME to 'decimatedVolumeLoader' creates a logic error:
CornerstoneCacheServicedefaults to this scheme ifvolumeLoaderSchemais unset, constructingbaseVolumeIdasdecimatedVolumeLoader:uid.- Logic generating the decimated ID blindly prepends
decimatedVolumeLoader:again, resulting indecimatedVolumeLoader:decimatedVolumeLoader:uid:.... - The
decimatedVolumeLoaderimplementation expects a single prefix; the extra prefix corrupts the parsed volume ID, preventing the loader from resolving data.
Use a standard scheme (e.g., cornerstoneStreamingImageVolume) for the default fallback or update the ID construction logic to handle the schema conditionally.
🤖 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 `@extensions/cornerstone/src/constants/index.ts` at line 2, The default volume
loader scheme is causing double-prefixed decimated IDs and breaking loader
resolution. Update the fallback in CornerstoneCacheService and the decimated ID
construction logic so the generated baseVolumeId and the decimatedVolumeLoader
ID are not prefixed twice; use a standard scheme for the default case or make
the prefixing conditional in the code paths around VOLUME_LOADER_SCHEME and the
decimated ID builder.
Comment on lines
+71
to
+96
| const volumeOptions = getVolumeOptions(); | ||
| if (!volumeOptions.gpuTestResults) { | ||
| try { | ||
| const isCypress = | ||
| typeof window !== 'undefined' && Boolean((window as any).Cypress); | ||
|
|
||
| if (isCypress || Boolean(appConfig.useCPURendering)) { | ||
| // Avoid WebGL/GPU probing in Cypress/CI runs, or when CPU rendering is forced. | ||
| setVolumeOptions({ | ||
| gpuTestResults: { | ||
| generalPerformanceScore: 100, | ||
| renderer: isCypress | ||
| ? 'Cypress (GPU test skipped)' | ||
| : 'CPU rendering forced (GPU test skipped)', | ||
| maxTextureSize: 0, | ||
| memoryUsedMB: 0, | ||
| memoryLimitMB: 0, | ||
| triangleRendering: 0, | ||
| textureUpload: 0, | ||
| bufferOperations: 0, | ||
| }, | ||
| rotateSampleDistanceFactor: 2, | ||
| sampleDistanceMultiplier: 1, | ||
| }); | ||
| } else { | ||
| await gpuPerformanceTest(appConfig); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t let skipped GPU probes suppress future real probes.
The guard only checks gpuTestResults, but the Cypress/CPU path stores a fake “skipped” result. If rendering mode changes later, startup will keep skipping the real benchmark and retain the high-score sampling defaults.
Proposed fix direction
const volumeOptions = getVolumeOptions();
- if (!volumeOptions.gpuTestResults) {
+ const isCypress =
+ typeof window !== 'undefined' && Boolean((window as any).Cypress);
+ const shouldSkipGpuTest = isCypress || Boolean(appConfig.useCPURendering);
+ const hasUsableGpuTestResults =
+ volumeOptions.gpuTestResults &&
+ (shouldSkipGpuTest ||
+ !String(volumeOptions.gpuTestResults.renderer).includes('GPU test skipped'));
+
+ if (!hasUsableGpuTestResults) {
try {
- const isCypress =
- typeof window !== 'undefined' && Boolean((window as any).Cypress);
-
- if (isCypress || Boolean(appConfig.useCPURendering)) {
+ if (shouldSkipGpuTest) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const volumeOptions = getVolumeOptions(); | |
| if (!volumeOptions.gpuTestResults) { | |
| try { | |
| const isCypress = | |
| typeof window !== 'undefined' && Boolean((window as any).Cypress); | |
| if (isCypress || Boolean(appConfig.useCPURendering)) { | |
| // Avoid WebGL/GPU probing in Cypress/CI runs, or when CPU rendering is forced. | |
| setVolumeOptions({ | |
| gpuTestResults: { | |
| generalPerformanceScore: 100, | |
| renderer: isCypress | |
| ? 'Cypress (GPU test skipped)' | |
| : 'CPU rendering forced (GPU test skipped)', | |
| maxTextureSize: 0, | |
| memoryUsedMB: 0, | |
| memoryLimitMB: 0, | |
| triangleRendering: 0, | |
| textureUpload: 0, | |
| bufferOperations: 0, | |
| }, | |
| rotateSampleDistanceFactor: 2, | |
| sampleDistanceMultiplier: 1, | |
| }); | |
| } else { | |
| await gpuPerformanceTest(appConfig); | |
| const volumeOptions = getVolumeOptions(); | |
| const isCypress = | |
| typeof window !== 'undefined' && Boolean((window as any).Cypress); | |
| const shouldSkipGpuTest = isCypress || Boolean(appConfig.useCPURendering); | |
| const hasUsableGpuTestResults = | |
| volumeOptions.gpuTestResults && | |
| (shouldSkipGpuTest || | |
| !String(volumeOptions.gpuTestResults.renderer).includes('GPU test skipped')); | |
| if (!hasUsableGpuTestResults) { | |
| try { | |
| if (shouldSkipGpuTest) { | |
| // Avoid WebGL/GPU probing in Cypress/CI runs, or when CPU rendering is forced. | |
| setVolumeOptions({ | |
| gpuTestResults: { | |
| generalPerformanceScore: 100, | |
| renderer: isCypress | |
| ? 'Cypress (GPU test skipped)' | |
| : 'CPU rendering forced (GPU test skipped)', | |
| maxTextureSize: 0, | |
| memoryUsedMB: 0, | |
| memoryLimitMB: 0, | |
| triangleRendering: 0, | |
| textureUpload: 0, | |
| bufferOperations: 0, | |
| }, | |
| rotateSampleDistanceFactor: 2, | |
| sampleDistanceMultiplier: 1, | |
| }); | |
| } else { | |
| await gpuPerformanceTest(appConfig); |
🤖 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 `@extensions/cornerstone/src/init.tsx` around lines 71 - 96, The startup guard
in init.tsx is treating the synthetic “skipped” gpuTestResults from the
Cypress/CPU path as a completed probe, which blocks later real benchmarking.
Update the logic around getVolumeOptions and setVolumeOptions so skipped results
are distinguishable from actual GPU test output, and only bypass
gpuPerformanceTest when a genuine benchmark has already been recorded; keep the
existing gpuPerformanceTest(appConfig) path available when rendering mode
changes.
Comment on lines
+42
to
+46
| export function setVolumeOptions(partial: Partial<VolumeOptions>): void { | ||
| const current = getVolumeOptions(); | ||
| const merged = { ...current, ...partial }; | ||
| localStorage.setItem(VOLUME_OPTIONS_KEY, JSON.stringify(merged)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard setVolumeOptions against localStorage failures.
getVolumeOptions is wrapped in try/catch, but setVolumeOptions is not. localStorage.setItem can throw (Safari private mode, disabled storage, quota exceeded). This setter is invoked during GPU-probe startup (init.tsx/gpuPerformanceTest.ts), so an exception here could surface as an unhandled startup error.
🛡️ Proposed fix
export function setVolumeOptions(partial: Partial<VolumeOptions>): void {
- const current = getVolumeOptions();
- const merged = { ...current, ...partial };
- localStorage.setItem(VOLUME_OPTIONS_KEY, JSON.stringify(merged));
+ try {
+ const current = getVolumeOptions();
+ const merged = { ...current, ...partial };
+ localStorage.setItem(VOLUME_OPTIONS_KEY, JSON.stringify(merged));
+ } catch {
+ // storage unavailable (private mode / quota); ignore
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function setVolumeOptions(partial: Partial<VolumeOptions>): void { | |
| const current = getVolumeOptions(); | |
| const merged = { ...current, ...partial }; | |
| localStorage.setItem(VOLUME_OPTIONS_KEY, JSON.stringify(merged)); | |
| } | |
| export function setVolumeOptions(partial: Partial<VolumeOptions>): void { | |
| try { | |
| const current = getVolumeOptions(); | |
| const merged = { ...current, ...partial }; | |
| localStorage.setItem(VOLUME_OPTIONS_KEY, JSON.stringify(merged)); | |
| } catch { | |
| // storage unavailable (private mode / quota); ignore | |
| } | |
| } |
🤖 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 `@platform/core/src/utils/volumeOptionsStorage.ts` around lines 42 - 46,
`setVolumeOptions` currently writes to `localStorage` without protection, so
startup flows that call it can fail if storage is unavailable or full. Update
`setVolumeOptions` in `volumeOptionsStorage` to mirror the defensive pattern
used by `getVolumeOptions`: wrap the merge-and-`localStorage.setItem` path in
try/catch, and safely no-op or log on failure so GPU probe startup from
`init.tsx`/`gpuPerformanceTest.ts` cannot throw an unhandled error. Keep the
behavior localized to `setVolumeOptions` and preserve the existing
`getVolumeOptions` fallback semantics.
| - `omitQuotationForMultipartRequest`: Some servers (e.g., .NET) require the `multipart/related` request to be sent without quotation marks. Defaults to `false`. If your server doesn't require this, then setting this flag to `true` might improve performance (by removing the need for preflight requests). Also note that | ||
| if auth headers are used, a preflight request is required. | ||
| - `maxNumRequests`: The maximum number of requests to allow in parallel. It is an object with keys of `interaction`, `thumbnail`, and `prefetch`. You can specify a specific number for each type. | ||
| - `volumeAutoDecimationThreshold`: (number, optional) If set, volumes with voxel count above this value are automatically decimated when loaded (e.g. for 3D viewports), reducing memory use. Default in `config/default.js` is `500_000_000` (500M voxels). When unset or undefined, auto-decimation is disabled. Users can still change decimation manually via the viewport Volume Downsizing menu. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether default.js defines volumeAutoDecimationThreshold and its value
fd -t f 'default.js' -p 'config' | xargs rg -nC3 'volumeAutoDecimationThreshold|customizationService' 2>/dev/nullRepository: OHIF/Viewers
Length of output: 50371
🏁 Script executed:
#!/bin/bash
# Find default.js in the typical config location
DEFAULT_JS=$(fd -t f 'default.js' 'platform/app/public/config' | head -n 1)
if [ -z "$DEFAULT_JS" ]; then
echo "File default.js not found in platform/app/public/config"
# Try broader search if needed, but usually it's here
fd -t f 'default.js' 'platform' | head -n 5
else
echo "Found file: $DEFAULT_JS"
echo "--- Content around volumeAutoDecimationThreshold ---"
rg -nC5 'volumeAutoDecimationThreshold' "$DEFAULT_JS"
fiRepository: OHIF/Viewers
Length of output: 253
🏁 Script executed:
#!/bin/bash
DEFAULT_JS="platform/app/public/config/default.js"
# Check if the file exists
if [ -f "$DEFAULT_JS" ]; then
echo "File exists. Lines count: $(wc -l < $DEFAULT_JS)"
# Search for the specific key with context
echo "--- Grep result for volumeAutoDecimationThreshold ---"
rg -n 'volumeAutoDecimationThreshold' "$DEFAULT_JS"
# If no result, search the file for just 'volume' or 'decimation' to see if it's named differently
if [ $? -ne 0 ]; then
echo "--- Search for related terms ---"
rg -ni 'volume|decimation' "$DEFAULT_JS" | head -n 20
fi
else
echo "Path not found: $DEFAULT_JS"
# List contents of the config directory
ls -la platform/app/public/config/ || echo "Directory not found"
fiRepository: OHIF/Viewers
Length of output: 311
Remove the incorrect reference to config/default.js regarding volumeAutoDecimationThreshold.
The file platform/app/public/config/default.js does not define volumeAutoDecimationThreshold. The documentation currently claims the default is 500_000_000 defined in that file, which is factually incorrect and misleading. Please either remove the reference to the specific file or update the documentation to reflect that this is not set by default in default.js (or specify where it is actually defaulted).
platform/docs/docs/configuration/configurationFiles.mdline 136
Snippet
``` - `volumeAutoDecimationThreshold`: (number, optional) If set, volumes with voxel count above this value are automatically decimated when loaded (e.g. for 3D viewports), reducing memory use. Default in `config/default.js` is `500_000_000` (500M voxels). When unset or undefined, auto-decimation is disabled. Users can still change decimation manually via the viewport Volume Downsizing menu. ```🤖 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 `@platform/docs/docs/configuration/configurationFiles.md` at line 136, The
documentation entry for volumeAutoDecimationThreshold incorrectly claims a
default is defined in config/default.js. Update the text in
configurationFiles.md to remove that file reference or replace it with the
correct source of truth, and make sure the description matches the actual
behavior exposed by volumeAutoDecimationThreshold rather than attributing a
default value to default.js.
| "Preview in viewport": "Test Preview in viewport", | ||
| "Grayscale": "Test Grayscale", | ||
| "Rendering Options": "Test Rendering Options", | ||
| "Volume Options": "Test Volume Options", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which WindowLevelActionMenu locale files contain the "Volume Options" key
fd 'WindowLevelActionMenu.json' platform/i18n/src/locales --exec sh -c 'printf "%s: " "$1"; grep -q "\"Volume Options\"" "$1" && echo present || echo MISSING' _ {}Repository: OHIF/Viewers
Length of output: 614
Add "Volume Options" to ru and ar locale files.
The key is already present in en-US, fr, nl, zh, and test-LNG, but is missing in ru and ar. Adding it here ensures consistent translation coverage across all supported locales.
🤖 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 `@platform/i18n/src/locales/test-LNG/WindowLevelActionMenu.json` at line 10,
The “Volume Options” localization key is missing from the ru and ar locale sets,
so add it to those locale JSON files to match the existing entries in en-US, fr,
nl, zh, and test-LNG. Use the same key name and add appropriate translated
values alongside the other WindowLevelActionMenu strings so the locale coverage
stays consistent across all supported languages.
Comment on lines
+178
to
+189
| useEffect(() => { | ||
| if (isLoading) { | ||
| wasEverLoading.current = true; | ||
| setHasSettled(false); | ||
| } else if (wasEverLoading.current) { | ||
| const timer = setTimeout(() => setHasSettled(true), SETTLE_DELAY); | ||
| return () => clearTimeout(timer); | ||
| } | ||
| }, [isLoading]); | ||
|
|
||
| const isExpanded = !hasSettled || isHovered || isDragging; | ||
| const effectiveWidth = isExpanded ? TRACK_WIDTH : RESTING_WIDTH; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
head -n 120 platform/ui-next/src/components/SmartScrollbar/SmartScrollbar.tsxRepository: OHIF/Viewers
Length of output: 3748
🏁 Script executed:
sed -n '120,200p' platform/ui-next/src/components/SmartScrollbar/SmartScrollbar.tsxRepository: OHIF/Viewers
Length of output: 3164
Scrollbar stays expanded for instantly loaded viewports.
If isLoading is false on the initial render (no previous loading state), wasEverLoading.current remains false and the useEffect fails to set hasSettled to true. This permanently locks isExpanded to true, forcing the track to stay at TRACK_WIDTH. Initialize hasSettled to true by default so the scrollbar contracts to RESTING_WIDTH when isLoading is never true.
🤖 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 `@platform/ui-next/src/components/SmartScrollbar/SmartScrollbar.tsx` around
lines 178 - 189, The SmartScrollbar state logic keeps the track expanded when
the view never enters loading, because `wasEverLoading.current` stays false and
`hasSettled` never flips back. Update the `SmartScrollbar` component so
`hasSettled` starts as true by default, while still letting the `useEffect`
reset it to false during loading and settle it after loading in the existing
`isLoading` flow. This will let `isExpanded` and `effectiveWidth` contract
correctly for instantly loaded viewports.
Comment on lines
+320
to
+342
| role="slider" | ||
| aria-valuenow={value} | ||
| aria-valuemin={0} | ||
| aria-valuemax={total - 1} | ||
| aria-orientation="vertical" | ||
| aria-label={ariaLabel} | ||
| tabIndex={0} | ||
| className={className} | ||
| style={{ | ||
| width: TRACK_WIDTH + hitZoneSideExtension * 2, | ||
| height: '100%', | ||
| position: 'relative', | ||
| marginLeft: -hitZoneSideExtension, | ||
| cursor: isDragging ? 'grabbing' : 'grab', | ||
| touchAction: 'none', | ||
| }} | ||
| onPointerEnter={() => setIsHovered(true)} | ||
| onPointerLeave={() => setIsHovered(false)} | ||
| onPointerDown={handlePointerDown} | ||
| onPointerMove={handlePointerMove} | ||
| onPointerUp={handlePointerUp} | ||
| onPointerCancel={handlePointerUp} | ||
| onKeyDown={enableKeyboardNavigation ? handleKeyDown : undefined} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Focusable role="slider" without keyboard operability when enableKeyboardNavigation is false.
tabIndex={0} is unconditional, so the slider is keyboard-focusable, but onKeyDown is only wired when enableKeyboardNavigation is true. The progress overlay (ViewportSliceProgressScrollbar) passes enableKeyboardNavigation={false}, leaving a focusable widget that can't be operated by keyboard (WCAG 2.1.1). Gate the tab stop on the same flag.
Proposed fix
- aria-label={ariaLabel}
- tabIndex={0}
+ aria-label={ariaLabel}
+ tabIndex={enableKeyboardNavigation ? 0 : -1}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| role="slider" | |
| aria-valuenow={value} | |
| aria-valuemin={0} | |
| aria-valuemax={total - 1} | |
| aria-orientation="vertical" | |
| aria-label={ariaLabel} | |
| tabIndex={0} | |
| className={className} | |
| style={{ | |
| width: TRACK_WIDTH + hitZoneSideExtension * 2, | |
| height: '100%', | |
| position: 'relative', | |
| marginLeft: -hitZoneSideExtension, | |
| cursor: isDragging ? 'grabbing' : 'grab', | |
| touchAction: 'none', | |
| }} | |
| onPointerEnter={() => setIsHovered(true)} | |
| onPointerLeave={() => setIsHovered(false)} | |
| onPointerDown={handlePointerDown} | |
| onPointerMove={handlePointerMove} | |
| onPointerUp={handlePointerUp} | |
| onPointerCancel={handlePointerUp} | |
| onKeyDown={enableKeyboardNavigation ? handleKeyDown : undefined} | |
| aria-label={ariaLabel} | |
| tabIndex={enableKeyboardNavigation ? 0 : -1} | |
| className={className} | |
| style={{ | |
| width: TRACK_WIDTH + hitZoneSideExtension * 2, | |
| height: '100%', | |
| position: 'relative', | |
| marginLeft: -hitZoneSideExtension, | |
| cursor: isDragging ? 'grabbing' : 'grab', | |
| touchAction: 'none', | |
| }} | |
| onPointerEnter={() => setIsHovered(true)} | |
| onPointerLeave={() => setIsHovered(false)} | |
| onPointerDown={handlePointerDown} | |
| onPointerMove={handlePointerMove} | |
| onPointerUp={handlePointerUp} | |
| onPointerCancel={handlePointerUp} | |
| onKeyDown={enableKeyboardNavigation ? handleKeyDown : undefined} |
🤖 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 `@platform/ui-next/src/components/SmartScrollbar/SmartScrollbar.tsx` around
lines 320 - 342, The SmartScrollbar slider is always focusable because
`tabIndex={0}` is unconditional, but `handleKeyDown` is only attached when
`enableKeyboardNavigation` is true, so `ViewportSliceProgressScrollbar` can
expose a non-operable keyboard target. Update the `SmartScrollbar` component so
the tab stop is gated by the same `enableKeyboardNavigation` flag used for
`onKeyDown`, keeping the slider focusable only when keyboard interaction is
actually supported.
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
1 participant
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.
Context
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Greptile Summary
This WIP PR introduces decimated volume loading for 3D rendering in OHIF: volumes exceeding a configurable voxel threshold are automatically downsampled via a new
decimatedVolumeLoader, a GPU benchmark runs at startup to calibrate sample-distance defaults, and a newSmartScrollbartracks per-slice load/viewed state with a visual progress fill.applyAutoDecimationIfNecessaryestimates voxel count from display-set metadata and selects an IJK decimation tuple;CornerstoneCacheServiceconstructs adecimatedVolumeLoader:…:I_J_Kvolume ID and passes it to Cornerstone's volume cache; anAutoDecimationOverlaybadge notifies the user when a volume has been reduced.gpuPerformanceTest): measures triangle throughput, texture upload, and buffer operations via a temporary off-screen WebGL canvas; stores ageneralPerformanceScoreinlocalStorageto setrotateSampleDistanceFactorandsampleDistanceMultiplieronce per browser session; Cypress/CPU-rendering paths skip the live test.useByteArraybatched updates; introducesViewedDataServiceto persist per-imageId viewed state across viewports.Confidence Score: 3/5
Not ready to merge — the PR is explicitly marked [WIP] and several unresolved issues from prior review rounds remain in the code.
Multiple issues flagged in earlier review rounds are still present: numImageFrames is overwritten for decimated volumes in volumeLoadedHandler, decimated volume cache entries are not purged during display-set invalidation, and the GPU result assigns level 'failed' outside the declared union type. New findings in this round are lower-impact but add to the overall debt.
init.tsx (volumeLoadedHandler still corrupts numImageFrames), CornerstoneCacheService.ts (stale decimated volumes survive invalidation, no-op delete calls), gpuPerformanceTest.ts (type-union mismatch on level, debug window assignment).
Important Files Changed
Comments Outside Diff (1)
extensions/cornerstone/src/services/CornerstoneCacheService/CornerstoneCacheService.ts, line 110-145 (link)invalidateVolumeiteratescandidateVolumeIds(baseVolumeId,…:volume3d,…:orthographic) but decimated volumes are stored under the keydecimatedVolumeLoader:${baseVolumeIdWithSuffix}:${i}_${j}_${k}. When a display set is invalidated (e.g. after a series reload), the stale decimated volume remains incs3DCacheand will be served on the next viewport render instead of the freshly-loaded data. A scan ofcs3DCache._volumeCachefor keys that start withdecimatedVolumeLoader:and contain the display set UID needs to be added to the purge loop.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "Add files to merge CornerstoneOverlays." | Re-trigger Greptile
Summary by CodeRabbit
New Features
Bug Fixes
Documentation