ECG Viewe || Smart Paint || Flatfoot Analysis#5913
ECG Viewe || Smart Paint || Flatfoot Analysis#5913onkar76 wants to merge 7 commits intoOHIF:release/3.12from
Conversation
onkar76
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ...panelService.addActivatePanelTriggers( | ||
| cornerstone.labelMapSegmentationPanel, | ||
| [ | ||
| { |
There was a problem hiding this comment.
true corrupts subscriptions array
The literal true appended to _activatePanelTriggersSubscriptions will cause a runtime error when the mode exits and iterates over the subscriptions to call .unsubscribe() on each entry. true is not an object and has no .unsubscribe() method.
| { | |
| true |
should be removed entirely:
...panelService.addActivatePanelTriggers(
tracked.measurements,
[
{
sourcePubSubService: measurementService,
sourceEvents: [
measurementService.EVENTS.MEASUREMENT_ADDED,
measurementService.EVENTS.RAW_MEASUREMENT_ADDED,
],
},
],
true
),
];| @@ -63,6 +66,69 @@ export const modeInstance = { | |||
| longitudinalRoute | |||
| ], | |||
| extensions: extensionDependencies, | |||
There was a problem hiding this comment.
onModeEnter silently drops base-mode initialization
The new onModeEnter added to modeInstance overrides any onModeEnter that exists on basicModeInstance (if the base object contains one) without delegating to it. Any initialization logic defined there — toolbar registration, tool-group setup, panel triggers — will be silently skipped for the longitudinal mode.
If basicModeInstance does not have an onModeEnter, this is harmless; but it should be made explicit. If it does, call it first:
onModeEnter(args) {
// delegate to base implementation if present
basicModeInstance.onModeEnter?.(args);
const { servicesManager, extensionManager, commandsManager, panelService, segmentationService } = args;
// ... rest of the new logic
},| { | ||
| path: 'dicomlocal', | ||
| layoutTemplate: () => { |
There was a problem hiding this comment.
mods
mods is computed but never read. The function always returns { valid: true } regardless of the modality split, so the variable is dead code.
| { | |
| path: 'dicomlocal', | |
| layoutTemplate: () => { | |
| isValidMode({ modalities }) { | |
| // ECG mode is valid for ECG modality and all non-DICOM image uploads | |
| return { valid: true, description: 'ECG & Clinical Tools' }; | |
| }, |
| }, | ||
| { | ||
| "packageName": "@custom/extension-ecg-tools" |
There was a problem hiding this comment.
version field for @custom/extension-ecg-tools
Every other entry in this file includes a "version" field. The plugin resolution at build time may silently fail or pick an unexpected version without it. The three other custom extensions added in this PR (extension-smart-paint, extension-flatfoot) do not appear here at all either, which may mean they rely on a different registration path — but for any entry that is present, the version should be specified.
| }, | |
| { | |
| "packageName": "@custom/extension-ecg-tools" | |
| { | |
| "packageName": "@custom/extension-ecg-tools", | |
| "version": "1.0.0" | |
| } |
| rrVariance, | ||
| type EcgCalibration, | ||
| } from '../utils/ecgCalculations'; | ||
| import SegmentLabelPanel, { type Segment } from '../../../../platform/app/src/components/SegmentLabelPanel'; |
There was a problem hiding this comment.
platform/app — architectural violation
PanelEcgViewer (and similarly PanelSmartPaint at extensions/smart-paint/src/panels/PanelSmartPaint.tsx:9 and PanelFlatfoot at extensions/flatfoot/src/panels/PanelFlatfoot.tsx:21) reaches into platform/app/src/components/SegmentLabelPanel via a relative path that crosses the extension–app boundary.
In the OHIF architecture, extensions must not import from the application platform layer. Doing so:
- Creates a hard circular-ish dependency (extensions are supposed to be consumed by the app, not depend on it).
- Makes the extensions impossible to use in any context other than this specific app build.
- Breaks tree-shaking and may produce duplicate module instances.
SegmentLabelPanel (or the Segment type it exports) should be moved to a shared @ohif/ui or @ohif/core package, or at minimum into a shared extensions utility package, so extensions can import it without crossing the app boundary.
| @@ -1,4 +1,6 @@ | |||
| import React, { useEffect, useState, useCallback } from 'react'; | |||
|
|
|||
| const EMBED_VIEWER_EVENT = 'ohif:embedViewer'; | |||
There was a problem hiding this comment.
const declaration placed between import statements
EMBED_VIEWER_EVENT is declared between two import lines. While technically valid in ES modules (the const is in module scope and imports are hoisted), it is a non-standard pattern that confuses linters, formatters, and reviewers. Move it below all imports.
| const EMBED_VIEWER_EVENT = 'ohif:embedViewer'; |
(Remove this line from between the imports and place it after the last import statement.)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| props: { | ||
| leftPanels: [ohif.thumbnailList], | ||
| leftPanelResizable: true, | ||
| rightPanels: [cornerstone.segmentation, cornerstone.measurements], | ||
| rightPanelClosed: true, | ||
| rightPanels: [cornerstone.labelMapSegmentationPanel, cornerstone.measurements], | ||
| rightPanelClosed: false, |
There was a problem hiding this comment.
Two silent behavioural changes are introduced here:
rightPanelClosed: true→false— the right panel will now open by default in the basic viewer for all users.enableSegmentationEdit: false→true— segmentation editing tools are now enabled by default.
Both are user-visible regressions for anyone using the basic viewer who does not expect segmentation or an open right panel. If these changes are intentional (e.g. they were requested as part of this feature), please add a comment explaining the rationale and update the relevant documentation/changelog entry.
| ) { | ||
| const { mask, width, height } = state; | ||
| const imageData = ctx.createImageData(width, height); | ||
| const data = imageData.data; | ||
|
|
||
| // Parse color | ||
| const tmp = document.createElement('canvas'); | ||
| tmp.width = tmp.height = 1; | ||
| const tmpCtx = tmp.getContext('2d')!; | ||
| tmpCtx.fillStyle = color; |
There was a problem hiding this comment.
A new <canvas> element is created and immediately thrown away on every call to renderMaskOverlay solely to parse the color string into RGB components. This is called during interactive painting (potentially dozens of times per second) and will create significant GC pressure.
Cache the parsed color values, or use a CSS-color parsing utility:
// Simple inline parser for rgba(r,g,b,a) and hex strings
function parseColor(color: string): [number, number, number] {
const m = color.match(/rgba?\((\d+),\s*(\d+),\s*(\d+)/);
if (m) return [+m[1], +m[2], +m[3]];
// hex
const hex = color.replace('#', '');
return [
parseInt(hex.slice(0, 2), 16),
parseInt(hex.slice(2, 4), 16),
parseInt(hex.slice(4, 6), 16),
];
}1 participant
Context
Changes & Results
Testing
ECG Viewer — Analyze heart ECG strips, measure QT intervals, calculate heart rate and cardiac axis
Smart Paint — Paint/highlight regions on any medical image, measure painted area
Flatfoot Analysis — Measure foot arch angles on X-rays to detect flat foot or high arch
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Greptile Summary
This PR adds three new clinical analysis extensions (ECG Viewer, Smart Paint ROI, and Flatfoot Analysis) to the OHIF viewer, along with a new ECG mode, a Clinical Hub aggregator route, and shortcut buttons on the worklist. It also modifies the longitudinal and basic modes to enable segmentation editing by default, and adds an embedded-iframe overlay capability to
ViewerLayout.Key issues found:
trueliteral is appended to_activatePanelTriggersSubscriptionsinmodes/longitudinal/src/index.ts. When the mode exits and iterates over subscriptions to call.unsubscribe(), it will throw aTypeError: true.unsubscribe is not a function.onModeEnteroverrides any base-modeonModeEnterfrombasicModeInstancewithout delegating to it, potentially skipping essential toolbar and tool-group initialization.PanelEcgViewer,PanelSmartPaint,PanelFlatfoot) importSegmentLabelPanelvia a relative path fromplatform/app/src/components. Extensions must not depend on the application layer; the shared component should live in@ohif/uior a dedicated shared package.rightPanelClosedis flipped fromtruetofalseandenableSegmentationEditfromfalsetotrue, silently changing UX defaults for all users of the basic viewer.versioninpluginConfig.json: The new@custom/extension-ecg-toolsentry lacks the"version"field required by every other plugin entry;extension-smart-paintandextension-flatfootare not registered at all.modsvariable inmodes/ecg/src/index.tsx:isValidMode.EMBED_VIEWER_EVENTconstant is placed between import statements inViewerLayout/index.tsx.Confidence Score: 2/5
truein the subscriptions array is a guaranteedTypeErroron every longitudinal mode exit. The silent default changes to the basic mode (open right panel + segmentation edit enabled) are visible UX regressions for existing users. The extension–app import boundary violation will create long-term maintainability problems. These three issues together bring the score to 2.Important Files Changed
trueappended to the subscriptions array) and a P1 concern around the base onModeEnter not being delegated to.modsvariable in isValidMode that is computed but never used.Sequence Diagram
sequenceDiagram participant WL as WorkList participant Router as App Router participant Hub as ViewerHubRoute participant ECG as PanelEcgViewer participant SP as PanelSmartPaint participant FF as PanelFlatfoot participant VL as ViewerLayout participant DOM as window CustomEvent WL->>Router: navigate /viewer-hub or /ecg-viewer Router->>Hub: render ViewerHubRoute Hub->>ECG: React.lazy load PanelEcgViewer Hub->>SP: React.lazy load PanelSmartPaint Hub->>FF: React.lazy load PanelFlatfoot note over ECG,FF: All three panels import SegmentLabelPanel from platform/app (arch violation) DOM->>VL: CustomEvent ohif:embedViewer with url VL->>VL: setEmbeddedViewerUrl(url) note over VL: Replace ResizablePanelGroup with iframe VL->>DOM: closeEmbedded dispatches ohif:embedViewer url nullLast reviewed commit: "Merge branch 'releas..."
(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!