Integrate ECG Tool, Smart Paint segmentation and flatfoot measurement tool #5915
Integrate ECG Tool, Smart Paint segmentation and flatfoot measurement tool #5915onkar76 wants to merge 3 commits intoOHIF:release/3.12from
Conversation
onkar76
changed the title
onkar76
changed the title
onkar76
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| {(() => { | ||
| const query = new URLSearchParams(); | ||
| if (filterValues.configUrl) { | ||
| query.append('configUrl', filterValues.configUrl); | ||
| } | ||
| query.append('StudyInstanceUIDs', studyInstanceUid); | ||
| preserveQueryParameters(query); | ||
| return ( | ||
| <Link | ||
| key="ecg-viewer" | ||
| to={`viewer${dataPath || ''}?${query.toString()}`} | ||
| > | ||
| <Button | ||
| type={ButtonEnums.type.primary} | ||
| size={ButtonEnums.size.smallTall} | ||
| startIcon={<Icons.LaunchArrow className="!h-[20px] !w-[20px] text-black" />} | ||
| onClick={() => {}} | ||
| dataCY={`mode-ecg-viewer-${studyInstanceUid}`} | ||
| > | ||
| ECG Viewer | ||
| </Button> | ||
| </Link> | ||
| ); | ||
| })()} |
There was a problem hiding this comment.
This IIFE renders an "ECG Viewer" <Button> inside StudyListExpandedRow unconditionally — it will appear for every study in the WorkList regardless of modality (CT, MR, US, etc.). The generated URL viewer?StudyInstanceUIDs=... also carries no mode identifier, so it simply opens the default viewer rather than any ECG-specific mode.
The existing per-mode buttons above this block (rendered by the modes.map(...)) already handle mode-based navigation. This addition appears to be a leftover debug/development shortcut that was not intended for production. It should be removed, or — if an ECG shortcut is truly needed — conditionally rendered when the study contains Waveform (ECG) series and routed to the correct mode.
| } | ||
|
|
||
| if (result.QT && result.RR) { | ||
| const qtSec = result.QT.ms / 1000; | ||
| const rrSec = result.RR.ms / 1000; | ||
| if (rrSec > 0) { | ||
| result.QTcB = { ms: (qtSec / Math.sqrt(rrSec)) * 1000 }; | ||
| result.QTcF = { ms: (qtSec / Math.cbrt(rrSec)) * 1000 }; | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| // ── Tool class ──────────────────────────────────────────────────────────────── | ||
|
|
||
| class ECGBidirectionalTool extends AngleTool { | ||
| static toolName = 'ECGBidirectional'; | ||
|
|
||
| constructor(toolProps = {}, defaultToolProps) { | ||
| super(toolProps, defaultToolProps); |
There was a problem hiding this comment.
computeStats converts the Euclidean world-coordinate distance directly to milliseconds via a hardcoded 25 mm/s ECG paper speed:
function mmToMs(mm) {
return (mm / ECG_PAPER_SPEED_MM_S) * 1000;
}This means the calculation is only accurate when:
- The DICOM image has correct pixel spacing metadata so that world coordinates are in true physical millimetres.
- The ECG was actually recorded and printed at exactly 25 mm/s — many systems also use 50 mm/s.
For scanned paper ECGs or screenshots there is typically no valid pixel spacing, so world distances bear no relation to physical millimetres and the QT/RR/QTc results will be silently wrong. Consider validating pixel spacing before presenting clinical values, and at minimum exposing a paper-speed selector (25/50 mm/s) so users can correct the assumption.
| function getIntervalStatus(key: string, ms: number) { | ||
| const def = ECG_INTERVAL_DEFS[key]; | ||
| if (!def || def.min === null) return null; | ||
| if (ms < def.min) return { label: '↓ Short', note: def.shortNote, color: 'text-yellow-400' }; | ||
| if (ms > def.max) return { label: '↑ Long', note: def.longNote, color: 'text-red-400' }; | ||
| return { label: '✓ Normal', note: `Normal range: ${def.min}–${def.max} ms`, color: 'text-green-400' }; | ||
| } | ||
|
|
||
| function qtcBazett(qtSec: number, rrSec: number): number | null { | ||
| if (!Number.isFinite(qtSec) || !Number.isFinite(rrSec) || rrSec <= 0) return null; |
There was a problem hiding this comment.
The standard HRV metric SDNN is defined as the sample standard deviation of NN intervals (divides by n − 1). The current code divides by n:
const sdnn = Math.sqrt(valid.reduce((s, v) => s + (v - meanRR) ** 2, 0) / n);This underestimates SDNN and will produce values that don't match established HRV reference ranges (e.g., Task Force of the European Society of Cardiology, 1996). Suggested fix:
| function getIntervalStatus(key: string, ms: number) { | |
| const def = ECG_INTERVAL_DEFS[key]; | |
| if (!def || def.min === null) return null; | |
| if (ms < def.min) return { label: '↓ Short', note: def.shortNote, color: 'text-yellow-400' }; | |
| if (ms > def.max) return { label: '↑ Long', note: def.longNote, color: 'text-red-400' }; | |
| return { label: '✓ Normal', note: `Normal range: ${def.min}–${def.max} ms`, color: 'text-green-400' }; | |
| } | |
| function qtcBazett(qtSec: number, rrSec: number): number | null { | |
| if (!Number.isFinite(qtSec) || !Number.isFinite(rrSec) || rrSec <= 0) return null; | |
| const sdnn = Math.sqrt(valid.reduce((s, v) => s + (v - meanRR) ** 2, 0) / (n - 1)); |
| female: { normal: 460, borderline: 470, prolonged: 470 }, | ||
| }; | ||
|
|
||
| const PROLONGED_CAUSES = [ | ||
| 'Amiodarone', 'Haloperidol', 'Sotalol', 'Azithromycin', |
There was a problem hiding this comment.
QTC_THRESHOLDS.borderline is dead code — its value is never read
The borderline field is declared in QTC_THRESHOLDS but qtcStatusLabel never references t.borderline; it only uses t.normal and t.prolonged:
if (ms > t.prolonged) // uses prolonged
...
if (ms > t.normal) // uses normalBecause borderline === prolonged for both genders the field has no effect on any output. Either the qtcStatusLabel function should be updated to use t.borderline as the upper boundary of the borderline zone (separating it from the fully prolonged zone), or the field should be removed. As written, male QTc 461–500 ms is labelled "PROLONGED" where clinically it should be "BORDERLINE".
| const isLabelMap = segmentationRepresentationTypes?.[0] === 'Labelmap'; | ||
| const addLabel = isLabelMap ? `Add ${t('Labelmap')} ${t('Segmentation')}` : t('Add segmentation'); |
There was a problem hiding this comment.
Constructing labels by concatenating two independently translated tokens in a fixed English word order:
const addLabel = isLabelMap ? `Add ${t('Labelmap')} ${t('Segmentation')}` : t('Add segmentation');won't translate correctly for languages with different noun/adjective order or compound-word rules. The same pattern is repeated in CustomDropdownMenuContent.tsx lines 53–58 (Create New ${t('Labelmap')} ${t('Segmentation')}, Manage Current …) and in SegmentationCollapsed.tsx line 80 (Select a ${t('Labelmap').toLowerCase()}).
All three cases should use a single translation key (e.g., 'Add Labelmap Segmentation') so translators can produce a grammatically correct phrase for each locale.
| "history": "5.3.0", | ||
| "i18next": "17.3.1", | ||
| "i18next-browser-languagedetector": "3.1.1", | ||
| "i18next-browser-languagedetector": "2.2.4", |
There was a problem hiding this comment.
i18next-browser-languagedetector downgraded from 3.1.1 → 2.2.4
This is a significant downgrade (4+ major patch versions). Version 2.2.4 predates several bug fixes and the same change is mirrored in platform/i18n/package.json. If the intention was to resolve a compatibility issue with the existing [email protected], please document why, as this may silently reintroduce old language-detection bugs. If it was accidental, revert to 3.1.1.
| // for all options read: https://www.i18next.com/overview/configuration-options | ||
| .init({ | ||
| fallbackLng: DEFAULT_LANGUAGE, | ||
| lngWhitelist: Object.keys(locales), |
There was a problem hiding this comment.
lngWhitelist is deprecated — use supportedLngs instead
lngWhitelist was deprecated in i18next v21 in favour of supportedLngs. While i18next 17.x still accepts it, aligning with the recommended API avoids a migration burden if i18next is ever upgraded. The same key is added on line 117.
| lngWhitelist: Object.keys(locales), | |
| supportedLngs: Object.keys(locales), |
|
is this correct base? you want it in 3.12? we don't usually add features after release |
2 participants
Context
Changes & Results
The following tools have been added to the Basic Viewer:
ECG QTc Tool — Enables QT interval measurement and QTc calculation directly on ECG images.
Smart Paint Suite — A comprehensive set of segmentation tools (Brush, Eraser, Threshold, AI-assisted segmentation, etc.) for labeling anatomical structures.
ABC Split Angle Tool (Flatfoot Measurement) — Provides geometric analysis for foot alignment assessment, including area and angle calculations.
Testing
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 measurement features to the Basic Viewer: an ECG QTc Tool (
ECGBidirectionalTool) for on-image QT/RR/QTc annotation, a Smart Paint Suite (Brush/Eraser/segmentation toolbox) for labelmap segmentation, and an ABC Split Angle Tool for flatfoot geometric analysis. It also introduces a sizeable newECGViewerPanel(973 lines) that provides off-image ECG interval, QTc, HRV, and QRS-axis calculations, wires the panel into the basic, longitudinal, and preclinical-4D modes, and extends the D3LineChartcomponent with point-click selection.Key concerns found during review:
ECGViewerPaneldivides byn(population SD) instead ofn − 1(sample SD); the standard Task Force SDNN definition requires sample SD, so reported values will underestimate the reference ranges.borderlinefield equalsprolongedfor both genders and is never read byqtcStatusLabel, so the BORDERLINE risk category is never returned correctly.i18next-browser-languagedetectoris downgraded from 3.1.1 → 2.2.4 without documented justification;lngWhitelist(deprecated) is used instead ofsupportedLngs; and multiple UI labels are constructed by splicing two translated tokens in a fixed English word order, breaking non-English locales.Confidence Score: 1/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant WorkList participant BasicMode participant ECGViewerPanel participant ECGBidirectionalTool participant LineChart User->>WorkList: Expand study row WorkList-->>User: Render mode buttons + (new) hardcoded ECG Viewer button User->>BasicMode: Open study via Basic mode BasicMode->>ECGViewerPanel: Mount in right panel BasicMode->>ECGBidirectionalTool: Register tool in tool group User->>LineChart: Click point on ECG chart LineChart-->>ECGViewerPanel: onPointClick(seriesIndex, pointIndex, x, y) ECGViewerPanel->>ECGViewerPanel: Add to selectedPoints (max 4) ECGViewerPanel->>ECGViewerPanel: buildInterval(A→B, B→C, C→D, B→D) ECGViewerPanel->>ECGViewerPanel: qtcBazett(qtSec, rrSec) ECGViewerPanel-->>User: Display QTc / HRV / QRS-axis results User->>ECGBidirectionalTool: Place 3 points (A, B, C) on ECG image ECGBidirectionalTool->>ECGBidirectionalTool: distance3(A,B) → mm → mmToMs → QT ms ECGBidirectionalTool->>ECGBidirectionalTool: distance3(A,C) → mm → mmToMs → RR ms ECGBidirectionalTool->>ECGBidirectionalTool: qtcBazett / qtcFridericia ECGBidirectionalTool-->>User: Overlay QT, RR, QTc labels on viewportReviews (1): Last reviewed commit: "Added clinical measurement changes" | Re-trigger Greptile
(4/5) You can add custom instructions or style guidelines for the agent here!