Skip to content

fix(MetadataProvider): Correctly assign imageId for multiframe images and remove unused frame information retrieval method#5965

Open
sedghi wants to merge 3 commits into
masterfrom
fix/multiframe
Open

fix(MetadataProvider): Correctly assign imageId for multiframe images and remove unused frame information retrieval method#5965
sedghi wants to merge 3 commits into
masterfrom
fix/multiframe

Conversation

@sedghi
Copy link
Copy Markdown
Member

@sedghi sedghi commented Apr 16, 2026

Context

add a console log after

const { PixelSpacing, type } = getPixelSpacingInformation(instance) || {};

in metadataprovider

see imageId in multiframe case is wrong even if you scroll always show the first one

try this case

https://viewer-dev.ohif.org/viewer?StudyInstanceUIDs=1.2.276.0.7230010.3.1.2.2723277605.10168.1676054414.178

Changes & Results

Testing

Checklist

PR

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • [] My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] OS:
  • [] Node version:
  • [] Browser:

Greptile Summary

This PR fixes a bug where all frames of a multiframe DICOM image always reported the first frame's imageId. The root cause is that DicomMetadataStore keeps only one instance per SOPInstanceUID; combineFrameInstance returns a new per-frame object but that object inherits imageId from the parent, which always pointed to frame 1. The fix explicitly stamps result.imageId = imageId after combineFrameInstance returns. It also removes the getFrameInformationFromURL helper (no external callers remain) and replaces URL-scraping with reading uids.frameNumber stored at ingestion time, while adding an explicit null-guard that was missing in the previous code.

Confidence Score: 5/5

Safe to merge — the fix is narrowly scoped, no regressions found across all existing data source call sites

No P0/P1 issues found. The imageId stamp is correct: for multiframe images combineFrameInstance returns a new per-frame Object.create() object so only that frame-specific cached instance is mutated, never the shared DicomMetadataStore instance. The new uids.frameNumber fallback is equivalent to the old URL-scraping for every current data source. The removed getFrameInformationFromURL has zero callers remaining in the repo.

No files require special attention

Important Files Changed

Filename Overview
platform/core/src/classes/MetadataProvider.ts Two targeted fixes: reassigns imageId on the combined frame instance to unblock per-frame metadata look-ups, and hardens getUIDsFromImageID with an explicit null guard while replacing URL-scraping with a stored uids.frameNumber read

Comments Outside Diff (1)

  1. platform/core/src/services/DicomMetadataStore/DicomMetadataStore.ts, line 323 (link)

    P0 security Accidental debug code exposes internal state globally

    window._model=_model is leftover debug code that leaks the internal _model object (which holds the full DICOM studies/series/instances tree) onto the global window object. This bypasses encapsulation, is a TypeScript type error (no semicolon, implicit any on window), and must not ship.

Reviews (3): Last reviewed commit: "fix(MetadataProvider): Add check for und..." | Re-trigger Greptile

… and remove unused frame information retrieval method
Copilot AI review requested due to automatic review settings April 16, 2026 20:32
@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 16, 2026

Deploy Preview for ohif-dev canceled.

Name Link
🔨 Latest commit 811ac60
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/69e148ddade2940008c587e8

Comment thread platform/core/src/classes/MetadataProvider.ts
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the core metadata plumbing to better handle multiframe DICOM imageIds and simplifies frame-number handling by removing a URL-based frame parsing helper.

Changes:

  • Reassign imageId on the returned instance from MetadataProvider._getInstance to avoid incorrect imageId on combined multiframe frame instances.
  • Remove getFrameInformationFromURL and switch frame derivation to rely on ingested UID mappings.
  • Expose the internal DICOM metadata store model on window (new global side effect).

Reviewed changes

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

File Description
platform/core/src/services/DicomMetadataStore/DicomMetadataStore.ts Adds a global window._model assignment (debug exposure) at module end.
platform/core/src/classes/MetadataProvider.ts Adjusts multiframe instance handling and changes frameNumber derivation / removes URL frame parsing method.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread platform/core/src/classes/MetadataProvider.ts
@cypress
Copy link
Copy Markdown

cypress Bot commented Apr 16, 2026

Viewers    Run #6191

Run Properties:  status check passed Passed #6191  •  git commit 811ac6020b: fix(MetadataProvider): Add check for undefined UIDs to prevent errors when proce...
Project Viewers
Branch Review fix/multiframe
Run status status check passed Passed #6191
Run duration 02m 35s
Commit git commit 811ac6020b: fix(MetadataProvider): Add check for undefined UIDs to prevent errors when proce...
Committer Alireza
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 37
View all changes introduced in this branch ↗︎

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.

2 participants