Skip to content

Draft: Add primary/secondary derived data#383

Open
igoroctaviano wants to merge 1 commit intomasterfrom
feat/multi-derived-data
Open

Draft: Add primary/secondary derived data#383
igoroctaviano wants to merge 1 commit intomasterfrom
feat/multi-derived-data

Conversation

@igoroctaviano
Copy link
Copy Markdown
Collaborator

feat: Fetch derived data from both primary and secondary DICOM stores when gcp= URL param is set

Summary

Closes #320.

When SLIM is opened with a ?gcp=… URL query parameter pointing at a secondary DICOM store, derived data (Comprehensive 3D SR, Microscopy Bulk Simple Annotation, Segmentation, Parametric Map, Presentation State) was only being fetched from that secondary store, ignoring whatever derived data already lived in the primary store. This PR makes SLIM merge derived data from both stores so:

  • annotations / segmentations already present in the primary store stay visible, and
  • additional derived series available only in the secondary gcp= store load on top of them.

This matches the behavior already implemented in OHIF and what was observed and flagged by @fedorov and @CPBridge in the issue thread.

Affected behavior

  • URL: /studies/{studyUID}/series/{seriesUID}?gcp={secondaryDicomWebUrl}
  • Before this PR: clients[<derivedClass>] was a DicomWebManager wrapping only the secondary store. Searches and retrievals for SR/ANN/SEG/PM/PR therefore never queried the primary store.
  • After this PR: clients[<derivedClass>] wraps the primary AND the secondary store. Searches run in parallel across both stores and the results are merged + de-duplicated; retrievals try the primary first and fall back to the secondary on failure (typical 404 for instances that only exist in one store).

Single-store deployments (no gcp=, no per-server storageClasses entries) are unchanged: every class still resolves to clientMapping.default, which still wraps just the configured primary.

Root cause

_createClientMapping in src/App.tsx builds a sopClassUID → DicomWebManager map. When a server config declares storageClasses (which is exactly what addGcpSecondaryAnnotationServer does for SR/SR3D/SEG/ANN/PM/ABPS/CSPS/GSPS/PSPS), those classes were assigned a manager wrapping only that server. The default (primary) was kept solely for whatever wasn't explicitly claimed (i.e., VL_WHOLE_SLIDE_MICROSCOPY_IMAGE). On top of that, DicomWebManager itself was hard-coded to only ever talk to stores[0] and explicitly errored with "Only one store is supported for now." when more than one store was passed in.

Changes

src/DicomWebManager.ts

Multi-store support added without changing the public API:

  • Searches (searchForStudies, searchForSeries, searchForInstances) now run in parallel against every readable store and the results are merged and de-duplicated by (StudyInstanceUID[/SeriesInstanceUID[/SOPInstanceUID]]). A failure on one store is logged in development but does not hide results from the other store.
  • Retrievals (retrieveStudyMetadata, retrieveSeriesMetadata, retrieveInstance, retrieveInstanceMetadata, retrieveInstanceFrames, retrieveInstanceRendered, retrieveInstanceFramesRendered, retrieveBulkData) try stores in order, fall through on failure (typically 404 when an instance only lives in one store), and re-throw the last error if every store fails so legitimate errors are still surfaced.
  • storeInstances now picks the first writable store (the secondary gcp= server is the writable one in the IDC config) instead of always using stores[0].
  • updateHeaders now propagates auth tokens to every wrapped store so the same Authorization header is set on the primary and secondary clients on sign-in / token refresh.
  • Removed the legacy "Only one store is supported for now." guard.
  • Two small helper functions searchAcrossStores and retrieveWithFallback encapsulate the cross-store dispatch so each public method stays a one-liner.

src/App.tsx

_createClientMapping now wraps both the default server and any specialty server(s) into the same DicomWebManager for each declared storageClass:

  • Tracks the default server(s) alongside the existing clientMapping.default.
  • Builds a Map<sopClassUID, ServerSettings[]> of every server (default + specialty) that should serve a given class.
  • Constructs one DicomWebManager per class that wraps […defaultServers, …specialtyServers].
  • Removed the previous "Only one configured server can specify a given storage class." warning since multiple specialty servers for the same class are now legitimately merged rather than silently overwriting each other.

src/__tests__/DicomWebManager.test.ts (new)

Nine unit tests covering the new behavior with stubbed underlying dwc.api.DICOMwebClients:

  • searchForSeries merges and de-duplicates results across stores by SeriesInstanceUID.
  • searchForInstances skips stores marked read: false.
  • A failing store does not hide results from a succeeding store.
  • retrieveInstanceFrames returns the primary result without falling back when primary succeeds.
  • retrieveInstanceFrames falls back to the secondary on primary rejection.
  • retrieveBulkData re-throws the last error when every store fails.
  • storeInstances routes to the first writable store (covers the gcp= STOW path).
  • storeInstances rejects when no store is writable.
  • updateHeaders propagates to every wrapped store (auth-token propagation regression guard).

Diff scope

 src/App.tsx                              |  48 ++++---
 src/DicomWebManager.ts                   | 242 +++++++++++++++++++++++++++----
 src/__tests__/DicomWebManager.test.ts    | 311 +++++++++++++++++++++++++++++++ (new)
 3 files changed, ~580 insertions(+), ~50 deletions(-)

Verification

  • npx tsc --noEmit — clean
  • npx biome check . — clean (95 files)
  • CI=true npx craco test --watchAll=false40/40 passing across 5 test suites (the new 9 plus all existing Worklist / segmentColors / formatDicomDate / logger tests)

Test plan

  • https://viewers-sandbox-gha-testing.web.app/studies/2.25.68803095896966276583382138924964839274/series/1.3.6.1.4.1.5962.99.1.1139028448.995765201.1637521600992.2.0?gcp=https://healthcare.googleapis.com/v1/projects/idc-dicom-review/locations/us-central1/datasets/gbm-parametric-map-test--heatmap-2025-08-06-dataset/dicomStores/gbm-parametric-map-test--heatmap-2025-08-06-dicom-store/dicomWeb shows derived data from both the IDC primary store (existing SEG/ANN) and the secondary parametric-map store at the same time (the exact regression reported in the issue).
  • The same URL without gcp= keeps showing the IDC primary store's SEG/ANN unchanged (single-store path).
  • A gcp= URL pointing at a store that 404s on derived series searches still loads derived data from the primary; conversely a primary that 404s still surfaces secondary derived data (cross-store resilience).
  • Worklist load is unchanged — clients.default still wraps only the primary, so QIDO study searches do not unexpectedly hit the secondary.
  • OIDC sign-in / silent token refresh: confirm both the primary and the secondary GCP healthcare endpoints receive the refreshed Authorization header (via the new updateHeaders propagation).
  • STOW (saving SR / ANN annotations) still targets the secondary gcp= store via the writable-store selection in storeInstances.
  • Lint / type-check / Jest pass (npx biome check ., npx tsc --noEmit, CI=true npx craco test --watchAll=false).

Risk / rollback

  • All public method signatures of DicomWebManager are unchanged; callers in SlideViewer.tsx, Worklist.tsx, CaseViewer.tsx, fetchImageMetadata.ts, etc. are untouched.
  • Single-store paths short-circuit through the same code (one readable store → one parallel search → one fallback step), so behavior for non-gcp= deployments is preserved.
  • The retrieval fallback only triggers when the first store throws, so a 200 OK from the primary still short-circuits the secondary call (no extra network traffic on the happy path).
  • Pure rollback is a revert of src/App.tsx and src/DicomWebManager.ts plus deletion of src/__tests__/DicomWebManager.test.ts.

Related

  • Closes #320
  • Aligns SLIM with OHIF's behavior of pulling derived data from primary AND secondary stores when a secondary is specified via URL.

@deepsource-io
Copy link
Copy Markdown

deepsource-io Bot commented Apr 30, 2026

DeepSource Code Review

We reviewed changes in f04d0d6...367321a on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade   Security  

Reliability  

Complexity  

Hygiene  

Code Review Summary

Analyzer Status Updated (UTC) Details
JavaScript Apr 30, 2026 5:31p.m. Review ↗

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

@sonarqubecloud
Copy link
Copy Markdown

❌ The last analysis has failed.

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support fetching of all data from both primary and secondary DICOM store in the URL

1 participant