Skip to content

fix: Enhance multiframe instance handling and improve instance valida… - #5956

Merged
sedghi merged 1 commit into
masterfrom
ptmeta
Apr 13, 2026
Merged

fix: Enhance multiframe instance handling and improve instance valida…#5956
sedghi merged 1 commit into
masterfrom
ptmeta

Conversation

@sedghi

@sedghi sedghi commented Apr 10, 2026

Copy link
Copy Markdown
Member

…tion

  • Updated getDisplaySetInfo to ensure local file frame imageIds resolve correctly, falling back to the source multiframe instance when necessary.
  • Improved instance filtering in isDisplaySetReconstructable to handle undefined instances more robustly.
  • Modified App prop types to make routerBasename optional and added new props for loading indicators and data sources.
  • Refactored CinePlayer component to replace Button with a div for better accessibility and interaction.

Context

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 adds null-safety fixes for multiframe instance handling (filtering undefined instances, guarding NumberOfFrames), relaxes App prop requirements, and refactors the CinePlayer FPS trigger from a <Button> to a <div role=\"button\">.

  • The <div role=\"button\"> in CinePlayer is missing an onKeyDown handler for Enter/Space. Because <div> elements don't fire click events on keydown natively and Radix's PopoverTrigger relies on that native behavior, keyboard users cannot open the FPS popover — a concrete accessibility regression versus the <Button> it replaces.

Confidence Score: 4/5

Safe to merge after addressing the missing keyboard handler on the FPS popover trigger in CinePlayer.

The null-safety and prop-type changes are straightforward and correct. The CinePlayer change introduces a P1 accessibility regression by removing native keyboard activation without providing a replacement handler.

platform/ui-next/src/components/CinePlayer/CinePlayer.tsx — missing onKeyDown for keyboard activation of the FPS popover trigger.

Important Files Changed

Filename Overview
platform/ui-next/src/components/CinePlayer/CinePlayer.tsx Replaces <Button> with <div role="button" tabIndex={0}> as PopoverTrigger child, but omits onKeyDown handler — keyboard users cannot activate the FPS popover via Enter/Space.
extensions/default/src/getSopClassHandlerModule.js Adds .filter(Boolean) guards for local-file frame imageIds that don't resolve to instance objects, with a fallback to the source multiframe instance when all entries are filtered out.
platform/core/src/utils/isDisplaySetReconstructable.js Adds null-safety via `instances?.filter(Boolean)
platform/app/src/App.tsx Makes routerBasename and the top-level config prop optional in PropTypes (existing default values already cover the missing-prop case), and adds PropTypes entries for showLoadingIndicator, showStudyList, modes, and dataSources.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: platform/ui-next/src/components/CinePlayer/CinePlayer.tsx
Line: 106-133

Comment:
**Missing keyboard activation on `role="button"` div**

Replacing `<Button>` with `<div role="button">` removes native keyboard activation. A `<button>` fires a `click` event on both Enter and Space by default; a `<div>` does not, even with `role="button"` and `tabIndex={0}`. Radix UI's `PopoverTrigger` with `asChild` merges an `onClick` handler onto the child element, but that handler is only invoked on pointer clicks, not keydown events. Keyboard users who tab to this element and press Enter or Space will get no response and cannot open the FPS popover.

An `onKeyDown` handler that calls `setPopoverOpen` when Enter or Space is pressed is needed to restore keyboard accessibility.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix: Enhance multiframe instance handlin..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

(5/5) You can turn off certain types of comments like style here!

…tion

- Updated `getDisplaySetInfo` to ensure local file frame imageIds resolve correctly, falling back to the source multiframe instance when necessary.
- Improved instance filtering in `isDisplaySetReconstructable` to handle undefined instances more robustly.
- Modified `App` prop types to make `routerBasename` optional and added new props for loading indicators and data sources.
- Refactored `CinePlayer` component to replace Button with a div for better accessibility and interaction.
@netlify

netlify Bot commented Apr 10, 2026

Copy link
Copy Markdown

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 96c40a5
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/69d94f129d2b3d00089e65a5
😎 Deploy Preview https://deploy-preview-5956--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Comment on lines 106 to +133
@@ -129,7 +130,7 @@ const CinePlayer: React.FC<CinePlayerProps> = ({
</div>
</Numeric.NumberStepper>
</Numeric.Container>
</Button>
</div>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Missing keyboard activation on role="button" div

Replacing <Button> with <div role="button"> removes native keyboard activation. A <button> fires a click event on both Enter and Space by default; a <div> does not, even with role="button" and tabIndex={0}. Radix UI's PopoverTrigger with asChild merges an onClick handler onto the child element, but that handler is only invoked on pointer clicks, not keydown events. Keyboard users who tab to this element and press Enter or Space will get no response and cannot open the FPS popover.

An onKeyDown handler that calls setPopoverOpen when Enter or Space is pressed is needed to restore keyboard accessibility.

Prompt To Fix With AI
This is a comment left during a code review.
Path: platform/ui-next/src/components/CinePlayer/CinePlayer.tsx
Line: 106-133

Comment:
**Missing keyboard activation on `role="button"` div**

Replacing `<Button>` with `<div role="button">` removes native keyboard activation. A `<button>` fires a `click` event on both Enter and Space by default; a `<div>` does not, even with `role="button"` and `tabIndex={0}`. Radix UI's `PopoverTrigger` with `asChild` merges an `onClick` handler onto the child element, but that handler is only invoked on pointer clicks, not keydown events. Keyboard users who tab to this element and press Enter or Space will get no response and cannot open the FPS popover.

An `onKeyDown` handler that calls `setPopoverOpen` when Enter or Space is pressed is needed to restore keyboard accessibility.

How can I resolve this? If you propose a fix, please make it concise.

@cypress

cypress Bot commented Apr 10, 2026

Copy link
Copy Markdown

Viewers    Run #6168

Run Properties:  status check passed Passed #6168  •  git commit 96c40a5a7d: fix: Enhance multiframe instance handling and improve instance validation
Project Viewers
Branch Review ptmeta
Run status status check passed Passed #6168
Run duration 02m 18s
Commit git commit 96c40a5a7d: fix: Enhance multiframe instance handling and improve instance validation
Committer sedghi
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 ↗︎

@sedghi
sedghi merged commit f5a66b9 into master Apr 13, 2026
13 checks passed
@sedghi
sedghi deleted the ptmeta branch April 13, 2026 13:31
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