OHIF 同步與資料源設定調整：回傳 series 選擇並改走 3001 proxy#5902
Conversation
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| emitSelection(); | ||
| // Poll as fallback for navigation paths that do not trigger history events. | ||
| setInterval(emitSelection, 1200); |
There was a problem hiding this comment.
setInterval never cleared — resource leak
The interval started here has no corresponding clearInterval. Because setupParentSelectionBridge runs once at app bootstrap and the return value of setInterval is discarded, this interval fires every 1200 ms for the entire lifetime of the page with no way to stop it. Even though the emitSelection guard (lastPayload) keeps it cheap when nothing changes, it still represents a permanent timer that cannot be cleaned up.
| setInterval(emitSelection, 1200); | |
| const intervalId = setInterval(emitSelection, 1200); | |
| // Return a cleanup function so callers can stop the interval if needed. | |
| return () => { | |
| clearInterval(intervalId); | |
| window.removeEventListener('popstate', emitSelection); | |
| window.removeEventListener('hashchange', emitSelection); | |
| }; |
| } | ||
|
|
||
| lastPayload = payload; | ||
| window.parent.postMessage(JSON.parse(payload), '*'); |
There was a problem hiding this comment.
JSON.stringify → JSON.parse round-trip
payload is already serialised to a string so it can be compared cheaply with lastPayload. On line 46 it is immediately deserialised back to an object before being passed to postMessage. postMessage performs its own structured-clone serialisation, so the double round-trip is redundant and slightly wasteful.
| window.parent.postMessage(JSON.parse(payload), '*'); | |
| window.parent.postMessage( | |
| { type: 'OHIF_SELECTION', studyInstanceUIDs, href: window.location.href }, | |
| '*' | |
| ); |
| type: 'OHIF_SELECTION', | ||
| studyInstanceUIDs, | ||
| href: window.location.href, | ||
| }); | ||
|
|
||
| if (payload === lastPayload) { | ||
| return; | ||
| } | ||
|
|
||
| lastPayload = payload; | ||
| window.parent.postMessage(JSON.parse(payload), '*'); |
There was a problem hiding this comment.
postMessage target origin is '*' — information disclosure risk
Using '*' as the target origin means this message (containing studyInstanceUIDs and the full href) will be delivered to any page that embeds the viewer in an iframe, regardless of its origin. An attacker who social-engineers a user into visiting a malicious page embedding this viewer would receive DICOM study UIDs silently.
Best practice is to accept a configurable allowedParentOrigin from window.config and pass it as the target origin:
window.parent.postMessage(payload, window.config?.allowedParentOrigin ?? '*');The same concern applies to Mode.tsx line 241.
| function setupParentSelectionBridge() { | ||
| // Bridge OHIF selection state to embedding parent app (iframe host). | ||
| if (!window.parent || window.parent === window) { | ||
| return; | ||
| } | ||
|
|
||
| let lastPayload = ''; | ||
|
|
||
| const emitSelection = () => { | ||
| try { | ||
| const params = new URLSearchParams(window.location.search || ''); | ||
| const studyInstanceUIDs = params.get('StudyInstanceUIDs') || ''; | ||
| const payload = JSON.stringify({ | ||
| type: 'OHIF_SELECTION', | ||
| studyInstanceUIDs, | ||
| href: window.location.href, | ||
| }); | ||
|
|
||
| if (payload === lastPayload) { | ||
| return; | ||
| } | ||
|
|
||
| lastPayload = payload; | ||
| window.parent.postMessage(JSON.parse(payload), '*'); | ||
| } catch (error) { | ||
| // Ignore bridge errors to avoid impacting viewer rendering. | ||
| } | ||
| }; | ||
|
|
||
| const wrapHistoryMethod = methodName => { | ||
| const original = window.history[methodName]; | ||
| window.history[methodName] = function (...args) { | ||
| const result = original.apply(this, args); | ||
| setTimeout(emitSelection, 0); | ||
| return result; | ||
| }; | ||
| }; | ||
|
|
||
| wrapHistoryMethod('pushState'); | ||
| wrapHistoryMethod('replaceState'); | ||
|
|
||
| window.addEventListener('popstate', emitSelection); | ||
| window.addEventListener('hashchange', emitSelection); | ||
|
|
||
| emitSelection(); | ||
| // Poll as fallback for navigation paths that do not trigger history events. | ||
| setInterval(emitSelection, 1200); | ||
| } |
There was a problem hiding this comment.
OHIF_SELECTION messages from two independent senders
setupParentSelectionBridge() in index.js (URL-only, no series info) and the new useEffect in Mode.tsx (rich viewport selection with series UID) both post messages with type: 'OHIF_SELECTION' to the parent window. Any given navigation event will likely trigger both handlers, causing the parent app to receive two messages in quick succession — the first with only studyInstanceUIDs, then a second (possibly more accurate) one overwriting it with seriesInstanceUIDs as well.
If Mode.tsx's richer handler is the intended canonical source of truth, the URL-based index.js bridge probably no longer needs to emit OHIF_SELECTION. Consider giving the index.js bridge a distinct type (e.g. OHIF_NAVIGATION) or removing its emission entirely now that Mode.tsx covers the same use-case with richer data.
| window.parent.postMessage( | ||
| { | ||
| type: 'OHIF_SELECTION', | ||
| studyInstanceUIDs: studyInstanceUID, | ||
| seriesInstanceUIDs: seriesInstanceUID, | ||
| href: window.location.href, | ||
| }, | ||
| '*' | ||
| ); |
There was a problem hiding this comment.
The message field studyInstanceUIDs holds the value of studyInstanceUID (a single string), and seriesInstanceUIDs holds a single series UID string. Using plural names for singular values creates an ambiguous API contract for the parent app consumer, who may reasonably expect an array.
Consider either renaming to the singular form or always wrapping the values in an array to keep the contract consistent with what index.js sends:
| window.parent.postMessage( | |
| { | |
| type: 'OHIF_SELECTION', | |
| studyInstanceUIDs: studyInstanceUID, | |
| seriesInstanceUIDs: seriesInstanceUID, | |
| href: window.location.href, | |
| }, | |
| '*' | |
| ); | |
| window.parent.postMessage( | |
| { | |
| type: 'OHIF_SELECTION', | |
| studyInstanceUID: studyInstanceUID, | |
| seriesInstanceUID: seriesInstanceUID, | |
| href: window.location.href, | |
| }, | |
| '*' | |
| ); |
| wadoUriRoot: 'http://127.0.0.1:3001/api/orthanc/wado', | ||
| qidoRoot: 'http://127.0.0.1:3001/api/orthanc/dicom-web', | ||
| wadoRoot: 'http://127.0.0.1:3001/api/orthanc/dicom-web', |
There was a problem hiding this comment.
local_orthanc.js is the shared, out-of-the-box local development configuration used by every contributor who runs OHIF against a local Orthanc instance. Replacing the standard http://localhost/dicom-web with http://127.0.0.1:3001/api/orthanc/... hard-wires this to a custom liverct-server proxy that only exists in one developer's environment.
Anyone cloning the repo and running the default local setup will immediately hit connection failures with no Orthanc data loading. This change should either:
- Be kept in a local, gitignored override file (e.g.
local_orthanc.local.js), or - Document and upstream the proxy layer as a required dependency alongside the usual Orthanc setup instructions.
1 participant
Context
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Greptile Summary
This PR adds an iframe-to-parent communication bridge that forwards the active OHIF selection (study UID and series UID) to an embedding parent application, and redirects local DICOMWeb traffic through a custom proxy on port 3001. While the core idea of syncing selection state to a parent frame is sound, the implementation has several notable issues that need addressing before merging.
Key changes:
local_orthanc.js: Replaces standardhttp://localhost/dicom-webOrthanc endpoints with a hardcoded personal proxy athttp://127.0.0.1:3001/api/orthanc/...— this will break local Orthanc setup for all other contributorsindex.js: AddssetupParentSelectionBridge()which monkey-patcheswindow.historyand polls every 1.2 s to sendOHIF_SELECTIONmessages to a parent iframe; thesetIntervalis never clearedMode.tsx: Adds auseEffectthat subscribes to viewport grid/display-set events and posts richerOHIF_SELECTIONmessages (includingseriesInstanceUID) — but creates a duplicate message stream conflicting with the one inindex.jsIssues found:
local_orthanc.jshardcodes a non-standard proxy that is specific to one developer's environment and will break other contributors' setupsindex.jsandMode.tsxindependently emittype: 'OHIF_SELECTION'messages, so the parent app receives duplicate and potentially conflicting events on every navigationsetIntervalfallback inindex.jsis never cleared — a permanent resource leakpostMessageuses'*'as the target origin in both files, which can leak DICOM study UIDs to any malicious host page embedding the viewerMode.tsxuses plural field names (studyInstanceUIDs,seriesInstanceUIDs) but sets them to single strings, creating an inconsistent API contract for the parent consumerConfidence Score: 1/5
local_orthanc.jscommits a personal proxy URL that will break other contributors' environments;index.jsintroduces a permanent uncleared interval and duplicates the message type already emitted byMode.tsx; and both JavaScript files usepostMessage('*')which leaks DICOM metadata to any embedding origin. None of the checklist items in the PR template are ticked, and no context, screenshots, or testing instructions are provided.platform/app/public/config/local_orthanc.js(breaks shared config),platform/app/src/index.js(resource leak + duplicate messages + security),platform/app/src/routes/Mode/Mode.tsx(security + naming inconsistency)Important Files Changed
setupParentSelectionBridge()to forward navigation events to an iframe parent viapostMessage. Three issues: thesetIntervalfallback is never cleared (resource leak), the target origin is'*'(information disclosure), and it duplicates theOHIF_SELECTIONmessage type thatMode.tsxnow also emits with richer data.useEffectthat subscribes to viewport/display-set changes and posts the active study+series selection to a parent iframe viapostMessage. The subscription cleanup is correct. Main concerns are'*'target origin and confusingly plural field names holding single string values.http://localhost/dicom-webDICOMWeb endpoints with a personal proxy athttp://127.0.0.1:3001/api/orthanc/.... This breaks the shared local dev configuration for all other contributors who do not have this proxy running.Sequence Diagram
sequenceDiagram participant Parent as Parent App (iframe host) participant IndexJS as index.js bridge<br/>(setupParentSelectionBridge) participant ModeRoute as Mode.tsx useEffect<br/>(viewport subscription) participant VGS as viewportGridService participant DSS as displaySetService Note over IndexJS: Runs once at bootstrap.<br/>Patches pushState/replaceState.<br/>Starts setInterval(1200ms) — never cleared. Parent->>IndexJS: (embeds OHIF in iframe) IndexJS-->>Parent: postMessage OHIF_SELECTION<br/>{studyInstanceUIDs, href} target='*' Note over ModeRoute: Runs after ExtensionDependenciesLoaded<br/>+ studyInstanceUIDs are set. ModeRoute->>VGS: subscribe(ACTIVE_VIEWPORT_ID_CHANGED) ModeRoute->>VGS: subscribe(GRID_STATE_CHANGED) ModeRoute->>DSS: subscribe(DISPLAY_SETS_CHANGED) ModeRoute->>VGS: getState() → activeViewport VGS-->>ModeRoute: gridState ModeRoute->>DSS: getDisplaySetByUID(displaySetUID) DSS-->>ModeRoute: displaySet {StudyInstanceUID, SeriesInstanceUID} ModeRoute-->>Parent: postMessage OHIF_SELECTION<br/>{studyInstanceUIDs (single string),<br/>seriesInstanceUIDs (single string), href}<br/>target='*' Note over Parent: Receives TWO OHIF_SELECTION messages<br/>per navigation — first from index.js,<br/>then from Mode.tsx.Last reviewed commit: b911efb
(2/5) Greptile learns from your feedback when you react with thumbs up/down!