Draft: Add primary/secondary derived data#383
Open
igoroctaviano wants to merge 1 commit intomasterfrom
Open
Conversation
|
|
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.
|
❌ The last analysis has failed. |
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
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.
feat: Fetch derived data from both primary and secondary DICOM stores when
gcp=URL param is setSummary
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: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
/studies/{studyUID}/series/{seriesUID}?gcp={secondaryDicomWebUrl}clients[<derivedClass>]was aDicomWebManagerwrapping only the secondary store. Searches and retrievals for SR/ANN/SEG/PM/PR therefore never queried the primary store.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-serverstorageClassesentries) are unchanged: every class still resolves toclientMapping.default, which still wraps just the configured primary.Root cause
_createClientMappinginsrc/App.tsxbuilds asopClassUID → DicomWebManagermap. When a server config declaresstorageClasses(which is exactly whataddGcpSecondaryAnnotationServerdoes 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,DicomWebManageritself was hard-coded to only ever talk tostores[0]and explicitly errored with"Only one store is supported for now."when more than one store was passed in.Changes
src/DicomWebManager.tsMulti-store support added without changing the public API:
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.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.storeInstancesnow picks the first writable store (the secondarygcp=server is the writable one in the IDC config) instead of always usingstores[0].updateHeadersnow propagates auth tokens to every wrapped store so the sameAuthorizationheader is set on the primary and secondary clients on sign-in / token refresh."Only one store is supported for now."guard.searchAcrossStoresandretrieveWithFallbackencapsulate the cross-store dispatch so each public method stays a one-liner.src/App.tsx_createClientMappingnow wraps both the default server and any specialty server(s) into the sameDicomWebManagerfor each declaredstorageClass:clientMapping.default.Map<sopClassUID, ServerSettings[]>of every server (default + specialty) that should serve a given class.DicomWebManagerper class that wraps[…defaultServers, …specialtyServers]."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:searchForSeriesmerges and de-duplicates results across stores bySeriesInstanceUID.searchForInstancesskips stores markedread: false.retrieveInstanceFramesreturns the primary result without falling back when primary succeeds.retrieveInstanceFramesfalls back to the secondary on primary rejection.retrieveBulkDatare-throws the last error when every store fails.storeInstancesroutes to the first writable store (covers thegcp=STOW path).storeInstancesrejects when no store is writable.updateHeaderspropagates to every wrapped store (auth-token propagation regression guard).Diff scope
Verification
npx tsc --noEmit— cleannpx biome check .— clean (95 files)CI=true npx craco test --watchAll=false— 40/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/dicomWebshows 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).gcp=keeps showing the IDC primary store's SEG/ANN unchanged (single-store path).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).clients.defaultstill wraps only the primary, so QIDO study searches do not unexpectedly hit the secondary.Authorizationheader (via the newupdateHeaderspropagation).gcp=store via the writable-store selection instoreInstances.npx biome check .,npx tsc --noEmit,CI=true npx craco test --watchAll=false).Risk / rollback
DicomWebManagerare unchanged; callers inSlideViewer.tsx,Worklist.tsx,CaseViewer.tsx,fetchImageMetadata.ts, etc. are untouched.gcp=deployments is preserved.src/App.tsxandsrc/DicomWebManager.tsplus deletion ofsrc/__tests__/DicomWebManager.test.ts.Related