Skip to content

fix(defaultRouteInit): defer prior fetches and guard study browser thumbnails#5971

Open
nutzlastfan wants to merge 4 commits into
OHIF:masterfrom
nutzlastfan:fix/defer-prior-fetches-and-guard-study-browser-thumbnails
Open

fix(defaultRouteInit): defer prior fetches and guard study browser thumbnails#5971
nutzlastfan wants to merge 4 commits into
OHIF:masterfrom
nutzlastfan:fix/defer-prior-fetches-and-guard-study-browser-thumbnails

Conversation

@nutzlastfan
Copy link
Copy Markdown

@nutzlastfan nutzlastfan commented Apr 20, 2026

Context

This ports a downstream loading fix that improves the first hanging-protocol application while still pulling prior studies in the background.

The downstream issue had two parts:

  • active and prior study retrievals were mixed too early during route initialization, which made the first HP run less predictable
  • the Study Browser could throw when a display set had no usable thumbnail image ids

Changes

  • fetch and store patient studies up front, but keep the initial metadata retrieval focused on the active study set
  • start prior-study series retrieval only after the first required series have loaded and the initial hanging protocol has been applied
  • upsert study metadata in DicomMetadataStore instead of assuming a fresh entry every time
  • guard Study Browser thumbnail selection for display sets without image ids or dynamic timepoints

Result

  • more stable initial study loading
  • priors still get prefetched, but they no longer interfere with the first layout decision
  • no Study Browser crash when SR/SEG/RT or other non-thumbnail-ready display sets are present

Notes

  • Opened as a draft because defaultRouteInit has been evolving quickly on upstream and I expect architecture-level feedback on the prefetch timing.

Greptile Summary

This PR restructures defaultRouteInit to separate active-study metadata retrieval from patient-history/prior-study fetching, deferring prior retrieves until after the first HP application, and adds defensive null guards to getImageIdForThumbnail for display sets that have no image IDs (SR/SEG/RT). The Study Browser guard is clean. The defaultRouteInit refactor is directionally correct but has two P2 concerns around prior-study filter leakage and missing cleanup on navigation.

Confidence Score: 4/5

Safe to merge with awareness of two P2 concerns; active-study loading and initial HP application are not regressed.

No P0/P1 findings. Two P2 issues: active-study seriesInstanceUID filter leaking into prior-study retrieval (silent suppression of prior series when a series filter is set), and a fire-and-forget startPriorFetches that lacks a cancellation path on route change.

platform/app/src/routes/Mode/defaultRouteInit.ts — specifically startPriorFetches and the filters forwarded to prior retrieves.

Important Files Changed

Filename Overview
platform/app/src/routes/Mode/defaultRouteInit.ts Introduces fetchAndStorePatientStudies, upsertStudyMetadata, collectSeriesPromises, and startPriorFetches to defer prior-study fetching after initial HP application; fire-and-forget prior fetches lack cancellation and the active-study filters are forwarded to prior-study retrieval without stripping seriesInstanceUID.
extensions/default/src/Panels/StudyBrowser/PanelStudyBrowser.tsx Adds null/empty guards for imageIds, timePoints, and middleTimePointImageIds in getImageIdForThumbnail; both call sites already handle an undefined return value, so the guards are safe and correct.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
platform/app/src/routes/Mode/defaultRouteInit.ts:267-274
**`filters` applied to prior-study retrieval may suppress all prior series**

`startPriorFetches` passes the same `filters` object as the active-study retrieval. If `filters.seriesInstanceUID` is set (e.g., `?SeriesInstanceUID=1.2.3`), the data source will only try to retrieve that specific series UID from every prior study — UIDs that won't exist in those studies — so each prior study returns zero series. The `collectSeriesPromises` loop then finds nothing, `requiredSeriesPromises` stays empty, and the prior studies silently load no display sets.

Since `seriesInstanceUID` filtering is meaningful only for the active study, passing a separate `filters` object (or stripping `seriesInstanceUID`) for prior retrieves would avoid this silent suppression.

```typescript
const priorFilters = filters
  ? { ...filters, seriesInstanceUID: undefined }
  : filters;

const priorRetrieves = priorStudyUIDs.map(StudyInstanceUID =>
  dataSource.retrieve.series.metadata({
    StudyInstanceUID,
    filters: priorFilters,
    returnPromises: true,
    sortCriteria: customizationService.getCustomization('sortingCriteria'),
  })
);
```

### Issue 2 of 2
platform/app/src/routes/Mode/defaultRouteInit.ts:303-305
**`startPriorFetches` has no cancellation / cleanup on route change**

`startPriorFetches` is fired as a fire-and-forget and is never added to `unsubscriptions`. If the user navigates to a new study before it completes, it continues running in the old closure and will call `applyHangingProtocol()` (line 287) against the shared `hangingProtocolService`/`displaySetService` singletons, potentially re-applying the prior route's HP ID over the new study's layout, and may add stale study metadata to `DicomMetadataStore`.

A cancellation token or an `AbortController`-style flag checked before each `applyHangingProtocol()` call and before each metadata upsert would prevent this cross-route interference.

Reviews (4): Last reviewed commit: "fix(app): limit URL display-set loading ..." | Re-trigger Greptile

@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 20, 2026

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 65c763d
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/69f363a418637900086eca37
😎 Deploy Preview https://deploy-preview-5971--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@nutzlastfan nutzlastfan marked this pull request as ready for review April 20, 2026 06:41
Comment thread platform/app/src/routes/Mode/defaultRouteInit.ts
Comment thread platform/app/src/routes/Mode/defaultRouteInit.ts Outdated
Comment thread platform/app/src/routes/Mode/defaultRouteInit.ts Outdated
Comment thread platform/app/src/routes/Mode/defaultRouteInit.ts
Comment thread platform/app/src/routes/Mode/defaultRouteInit.ts Outdated
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.

1 participant