Skip to content

Improve DICOM SR compatibility#76

Merged
timcogan merged 6 commits intomasterfrom
fix/dicom-sr
Apr 16, 2026
Merged

Improve DICOM SR compatibility#76
timcogan merged 6 commits intomasterfrom
fix/dicom-sr

Conversation

@timcogan
Copy link
Copy Markdown
Owner

@timcogan timcogan commented Apr 10, 2026

Summary by CodeRabbit

  • New Features

    • Overlays now render short text labels alongside visible vector marks and present both graphics and labels per image/frame.
    • Measurement labels use padded, rounded backgrounds for improved readability.
  • Documentation

    • CAD SR overlay rules clarified: supported only when vector (mark) data are present; non-geometric SR content appears only in the document view.
    • Help/keyboard text updated to reference “Mammography CAD SR marks”; navigation jumps to frames with GSPS/CAD/Parametric Map overlays.
  • Style

    • Overlay color updated to brand blue.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a1415079-f647-49bd-aa39-d53aeb2556db

📥 Commits

Reviewing files that changed from the base of the PR and between 97eaef0 and 96034b0.

📒 Files selected for processing (1)
  • src/dicom/sr.rs

📝 Walkthrough

Walkthrough

Adds extraction, storage, merging, and on-image rendering of Mammography CAD SR labels (anchor, multi-line text, units); updates measurement label padding/background and SR overlay color; tightens multi-int parsing and refines documentation, tests, and overlay merge logic to include labels.

Changes

Cohort / File(s) Summary
Documentation
DESIGN.md, README.md, website/content/docs/viewer-basics.md
Clarify Mammography CAD SR overlay rules: require vector marks for render/navigation; note short finding text rendered with visible geometry; update G-toggle and navigation wording.
SR model & parsing
src/dicom/sr.rs, src/dicom.rs
Add exported SrOverlayLabel; extend SrOverlay with labels: Vec<_> and frame query APIs; hydrate acquisition context (laterality/view); preserve/report graphic units; tighten multi-int parsing; label anchor/line extraction and deduplication; add related tests.
Overlay merge logic
src/app/overlay.rs
Merge paths now append both graphics and labels when combining overlays per SOP UID (pending & authoritative merge paths updated).
Rendering & app logic
src/app.rs
Render SR labels for each visible_labels_for_frame(...); add label layout/style constants, clamping to image bounds, multi-line layout, label rendering function, and switch overlay color to brand blue; update/refactor SR overlay tests.
Measurement label styling
src/app/measurement.rs
Introduce measurement label padding constants; render rounded semi-transparent background; draw text with padding; remove glyph-offset contrast trick; add unit test for padding/clamping.
Tests
src/app.rs (tests), src/dicom.rs (tests), src/dicom/sr.rs (tests)
Extend/refactor tests to cover label extraction/formatting/visibility, unit preservation, merge behavior, layout/clamping, and multi-int parsing edge cases.

Sequence Diagram

sequenceDiagram
    participant Loader as DICOM Loader
    participant Parser as SR Parser
    participant App as Viewer App
    participant Renderer as Graphics Renderer

    Loader->>Parser: load_mammography_cad_sr_overlays()
    Parser->>Parser: parse vector graphics, units, and label anchors/lines
    Parser-->>App: emit SrOverlay { graphics, labels }
    App->>App: merge_sr_overlays() append graphics + labels per SOP UID
    App->>Renderer: draw SR graphics (stroke: rgba(0,109,204,0.5))
    Renderer-->>App: graphics rendered
    App->>Renderer: draw_sr_overlay_label() for visible_labels_for_frame()
    Renderer-->>App: labels rendered (clamped to image bounds)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐇 I hopped through SR vectors bright,

I stitched a label, snug and light,
I padded, clamped, and set it true,
A little blue badge on your view,
A rabbit’s whisper: clearer sight.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request has no description content. The template requires a Summary section, Testing checklist, and optional Screenshots/Checklist sections to provide context and verification of changes. Add a comprehensive description following the template: include a Summary explaining the DICOM SR improvements (label rendering, overlay merging, multi-int parsing), document Testing steps (fmt-check, clippy, test, and manual testing), and Screenshots if UI changes are visible.
Docstring Coverage ⚠️ Warning Docstring coverage is 29.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Improve DICOM SR compatibility' accurately reflects the primary changes across multiple files related to DICOM Structured Report (SR) handling, including label extraction, rendering, and overlay merging logic.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dicom-sr

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (3)
src/app.rs (1)

1364-1370: SR overlay label anchors lack explicit coordinate space tracking.

Unlike GSPS graphics which read and respect GRAPHIC_ANNOTATION_UNITS, SR overlay label anchors are assumed to be pixel-space throughout the codebase but carry no units information. The SrOverlayLabel.anchor is a raw (f32, f32) tuple, and parse_spatial_coordinates never extracts GRAPHIC_ANNOTATION_UNITS (Tag 0x0070, 0x0005). This means if SR parsing is ever enhanced to support display-space coordinates (as GSPS does), label positioning will silently break unless the anchor struct gains a units field. Either add a units field to SrOverlayLabel or explicitly normalize and document that anchors are always pixel-space at construction time.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/app.rs` around lines 1364 - 1370, The SR overlay label anchor currently
stores raw coordinates without units, risking silent breakage if non-pixel units
are later parsed; update the SR parsing path so either (A) SrOverlayLabel gains
a units field (e.g., enum like GspsUnits) and parse_spatial_coordinates reads
Tag (0x0070,0x0005) GRAPHIC_ANNOTATION_UNITS to populate that field, and adapt
any uses of SrOverlayLabel.anchor (e.g., where passed to
Self::gsps_point_to_screen) to consult the units, or (B) explicitly normalize
coordinates to pixel-space at construction time and document that
SrOverlayLabel.anchor is always pixel-space; implement one of these two fixes
and update all call sites (including gsps_point_to_screen invocations) to rely
on the new contract.
src/dicom/sr.rs (2)

827-840: Consider using trim_end_matches for cleaner trailing character removal.

The while loop works correctly, but Rust provides a more idiomatic approach.

♻️ Suggested refactor
 fn format_sr_overlay_number(value: f32) -> String {
     if (value.fract()).abs() < 0.01 {
         format!("{value:.0}")
     } else {
-        let mut output = format!("{value:.1}");
-        while output.ends_with('0') {
-            output.pop();
-        }
-        if output.ends_with('.') {
-            output.pop();
-        }
-        output
+        format!("{value:.1}")
+            .trim_end_matches('0')
+            .trim_end_matches('.')
+            .to_string()
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/dicom/sr.rs` around lines 827 - 840, The formatting function
format_sr_overlay_number currently trims trailing zeros and a trailing decimal
point using a while loop; replace that manual loop with Rust's &str trims for
clarity: after creating output with format!("{value:.1}"), call
output.trim_end_matches('0') and then trim_end_matches('.') and convert back to
String (i.e., output =
output.trim_end_matches('0').trim_end_matches('.').to_string()) so trailing
zeros and a lone '.' are removed idiomatically while preserving the existing
branching logic in format_sr_overlay_number.

1161-1166: Observation: Test laterality/view values won't match abbreviation mappings.

The test uses "test-side-omega" and "test-view-sigma" which intentionally don't match any known laterality/view abbreviations in mammography_laterality_abbreviation and mammography_view_abbreviation. This results in the expected output having only the certainty percentage without laterality/view context (line 1286: "1234.5%").

This is fine if intentional—it tests the certainty-only case. Consider adding a separate test with real laterality/view values (e.g., "Left Breast", "Cranio-caudal") to verify the full context label generation.

Also applies to: 1284-1288

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/dicom/sr.rs` around lines 1161 - 1166, The test currently constructs an
image reference using image_library_reference_item with non-matching
laterality/view strings ("test-side-omega", "test-view-sigma"), which only
exercises the certainty-only branch; add an additional test case that uses real
mammography laterality and view values (for example "Left Breast" and
"Cranio-caudal") when calling image_library_reference_item (and any surrounding
test helper like referenced_frames) so the code paths in
mammography_laterality_abbreviation and mammography_view_abbreviation are
exercised and the full context label (laterality + view + certainty) is
asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/app.rs`:
- Around line 1377-1409: The label rectangle can exceed image_rect when
label_size is larger than the available image area; fix this by clamping the
label_size to the available space before constructing label_rect (use
image_rect.size() or image_rect.right()-image_rect.left(), bottom()-top() to
compute available_size), then compute a clamped_size =
label_size.min(available_size) and build the rect from the clamped size (e.g.
compute a final_min using the same clamped preferred position logic and create
label_rect from_min_size or from_min_max using final_min and final_min +
clamped_size); update subsequent text layout/painting to use the
clamped_size/rect (symbols: label_size, image_rect, max_x/max_y, preferred,
label_rect, egui::Rect::from_min_size).

In `@src/dicom.rs`:
- Around line 689-701: In read_item_multi_int: the current early-return from
element.to_str() can produce empty or partially parsed Vec<i32> (via filter_map)
and skip the typed fallback; change this to attempt a strict parse of every
split part (trim and try parse each) and only return Some(vec) if all parts
parsed successfully and vec is non-empty, otherwise fall back to
element.to_multi_int::<i32>() so you don't silently drop values or return
empty/partial results; reference element.to_str(), the split('\\') parsing
branch, and element.to_multi_int::<i32>() when implementing the fix.

In `@website/content/docs/viewer-basics.md`:
- Around line 44-47: Update the overlay behavior text so it includes Parametric
Map overlays alongside GSPS and Mammography CAD SR: mention that the 'G' key
toggles the active image overlay when a matching GSPS, Mammography CAD SR, or
Parametric Map object with vector marks is available, and that the 'N' key jumps
to the next image/frame containing any of those overlay types; also add a
sentence stating Parametric Map overlays are supplementary (do not count as
display slots) and follow the same visibility rules as the other SR overlays.

---

Nitpick comments:
In `@src/app.rs`:
- Around line 1364-1370: The SR overlay label anchor currently stores raw
coordinates without units, risking silent breakage if non-pixel units are later
parsed; update the SR parsing path so either (A) SrOverlayLabel gains a units
field (e.g., enum like GspsUnits) and parse_spatial_coordinates reads Tag
(0x0070,0x0005) GRAPHIC_ANNOTATION_UNITS to populate that field, and adapt any
uses of SrOverlayLabel.anchor (e.g., where passed to Self::gsps_point_to_screen)
to consult the units, or (B) explicitly normalize coordinates to pixel-space at
construction time and document that SrOverlayLabel.anchor is always pixel-space;
implement one of these two fixes and update all call sites (including
gsps_point_to_screen invocations) to rely on the new contract.

In `@src/dicom/sr.rs`:
- Around line 827-840: The formatting function format_sr_overlay_number
currently trims trailing zeros and a trailing decimal point using a while loop;
replace that manual loop with Rust's &str trims for clarity: after creating
output with format!("{value:.1}"), call output.trim_end_matches('0') and then
trim_end_matches('.') and convert back to String (i.e., output =
output.trim_end_matches('0').trim_end_matches('.').to_string()) so trailing
zeros and a lone '.' are removed idiomatically while preserving the existing
branching logic in format_sr_overlay_number.
- Around line 1161-1166: The test currently constructs an image reference using
image_library_reference_item with non-matching laterality/view strings
("test-side-omega", "test-view-sigma"), which only exercises the certainty-only
branch; add an additional test case that uses real mammography laterality and
view values (for example "Left Breast" and "Cranio-caudal") when calling
image_library_reference_item (and any surrounding test helper like
referenced_frames) so the code paths in mammography_laterality_abbreviation and
mammography_view_abbreviation are exercised and the full context label
(laterality + view + certainty) is asserted.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0422078c-3ca0-420b-8901-55acc2fb1088

📥 Commits

Reviewing files that changed from the base of the PR and between 749e6d3 and 7060898.

📒 Files selected for processing (8)
  • DESIGN.md
  • README.md
  • src/app.rs
  • src/app/measurement.rs
  • src/app/overlay.rs
  • src/dicom.rs
  • src/dicom/sr.rs
  • website/content/docs/viewer-basics.md

Comment thread src/app.rs
Comment thread src/dicom.rs
Comment thread website/content/docs/viewer-basics.md Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/dicom/sr.rs (1)

447-455: ⚠️ Potential issue | 🟠 Major

Enrich inline IMAGE targets too

These lines only hydrate laterality/view on the copy inserted into image_index. When selected_image_target() resolves a direct child IMAGE item instead of a ReferencedContentItemIdentifier, it still returns the raw ReferencedImageTarget with both fields unset, so those overlays lose the mammography context label and can also slip past the per-target de-dupe once equality includes laterality/view. Please move this enrichment into a shared helper and use it for both indexed and inline image references.

Possible direction
+fn enriched_image_reference(node: &SrIndexedNode) -> Option<ReferencedImageTarget> {
+    let mut reference = node.image_reference.clone()?;
+    reference.laterality = reference
+        .laterality
+        .or_else(|| node.acquisition_context_value("Image Laterality"));
+    reference.view = reference
+        .view
+        .or_else(|| node.acquisition_context_value("Image View"));
+    Some(reference)
+}
+
 fn collect_image_references(
     nodes: &[SrIndexedNode],
     image_index: &mut HashMap<Vec<usize>, ReferencedImageTarget>,
 ) {
     for node in nodes {
-        if let Some(mut reference) = node.image_reference.clone() {
-            reference.laterality = node.acquisition_context_value("Image Laterality");
-            reference.view = node.acquisition_context_value("Image View");
+        if let Some(reference) = enriched_image_reference(node) {
             image_index.insert(node.path.clone(), reference);
         }
         collect_image_references(&node.children, image_index);
     }
 }

Then use the same helper inside selected_image_target() when returning a direct child IMAGE.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/dicom/sr.rs` around lines 447 - 455, The code only sets laterality and
view when inserting a cloned ReferencedImageTarget into the image_index inside
collect_image_references, leaving inline IMAGE targets returned by
selected_image_target() un-enriched; create a small helper (e.g.,
hydrate_image_context(target: &mut ReferencedImageTarget, node: &SrIndexedNode)
or similar) that reads node.acquisition_context_value("Image Laterality") and
("Image View") and sets target.laterality and target.view, call that helper from
collect_image_references before inserting into image_index and also call it in
selected_image_target() when returning a direct child IMAGE
ReferencedImageTarget so both indexed and inline targets have the same enriched
fields.
🧹 Nitpick comments (1)
src/app/measurement.rs (1)

182-193: Preserve padding when labels are clamped near viewport edges.

Right now, clamping is done on the text rect and padding is added afterward, so background padding can get cut off at edges. Compute layout with padded size first, then place text inside it.

Proposed refactor
-        let label_rect = measurement_label_rect(start, end, galley.size(), painter.clip_rect());
-        painter.rect_filled(
-            egui::Rect::from_min_max(
-                label_rect.min
-                    - egui::vec2(MEASUREMENT_LABEL_PADDING_X, MEASUREMENT_LABEL_PADDING_Y),
-                label_rect.max
-                    + egui::vec2(MEASUREMENT_LABEL_PADDING_X, MEASUREMENT_LABEL_PADDING_Y),
-            ),
-            4.0,
-            egui::Color32::from_black_alpha(176),
-        );
-        painter.galley(label_rect.min, galley, MEASUREMENT_COLOR);
+        let padding = egui::vec2(MEASUREMENT_LABEL_PADDING_X, MEASUREMENT_LABEL_PADDING_Y);
+        let padded_size = galley.size() + egui::vec2(padding.x * 2.0, padding.y * 2.0);
+        let bg_rect = measurement_label_rect(start, end, padded_size, painter.clip_rect());
+        painter.rect_filled(bg_rect, 4.0, egui::Color32::from_black_alpha(176));
+        painter.galley(bg_rect.min + padding, galley, MEASUREMENT_COLOR);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/app/measurement.rs` around lines 182 - 193, The background padding gets
cut off because clamping is applied to the text rect then padding added; change
to compute layout using the padded size first: compute padded_size =
galley.size() + egui::vec2(2.0*MEASUREMENT_LABEL_PADDING_X,
2.0*MEASUREMENT_LABEL_PADDING_Y), call measurement_label_rect(start, end,
padded_size, painter.clip_rect()) to get the clamped background rect, draw the
background with painter.rect_filled using that rect, then position the text
inside by calling painter.galley(label_rect.min +
egui::vec2(MEASUREMENT_LABEL_PADDING_X, MEASUREMENT_LABEL_PADDING_Y), galley,
MEASUREMENT_COLOR) so the padding is preserved when near viewport edges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/dicom/sr.rs`:
- Around line 447-455: The code only sets laterality and view when inserting a
cloned ReferencedImageTarget into the image_index inside
collect_image_references, leaving inline IMAGE targets returned by
selected_image_target() un-enriched; create a small helper (e.g.,
hydrate_image_context(target: &mut ReferencedImageTarget, node: &SrIndexedNode)
or similar) that reads node.acquisition_context_value("Image Laterality") and
("Image View") and sets target.laterality and target.view, call that helper from
collect_image_references before inserting into image_index and also call it in
selected_image_target() when returning a direct child IMAGE
ReferencedImageTarget so both indexed and inline targets have the same enriched
fields.

---

Nitpick comments:
In `@src/app/measurement.rs`:
- Around line 182-193: The background padding gets cut off because clamping is
applied to the text rect then padding added; change to compute layout using the
padded size first: compute padded_size = galley.size() +
egui::vec2(2.0*MEASUREMENT_LABEL_PADDING_X, 2.0*MEASUREMENT_LABEL_PADDING_Y),
call measurement_label_rect(start, end, padded_size, painter.clip_rect()) to get
the clamped background rect, draw the background with painter.rect_filled using
that rect, then position the text inside by calling
painter.galley(label_rect.min + egui::vec2(MEASUREMENT_LABEL_PADDING_X,
MEASUREMENT_LABEL_PADDING_Y), galley, MEASUREMENT_COLOR) so the padding is
preserved when near viewport edges.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: facec890-8587-4e82-9730-3e6a7889089d

📥 Commits

Reviewing files that changed from the base of the PR and between 7060898 and efc78e0.

📒 Files selected for processing (8)
  • DESIGN.md
  • README.md
  • src/app.rs
  • src/app/measurement.rs
  • src/app/overlay.rs
  • src/dicom.rs
  • src/dicom/sr.rs
  • website/content/docs/viewer-basics.md
✅ Files skipped from review due to trivial changes (1)
  • README.md
🚧 Files skipped from review as they are similar to previous changes (5)
  • website/content/docs/viewer-basics.md
  • DESIGN.md
  • src/app/overlay.rs
  • src/dicom.rs
  • src/app.rs

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/dicom/sr.rs (1)

485-532: ⚠️ Potential issue | 🟠 Major

Compare label targets by image/frame identity, not full ReferencedImageTarget.

ReferencedImageTarget equality now includes laterality and view, but these are metadata, not identity. In this PR, both label dedupe (labeled_targets.contains(&reference)) and anchor lookup (target == *reference) rely on full equality, so the same SOP/frame can stop matching when one geometry resolves through the image library and another resolves inline with different hydration. That can produce duplicate labels or miss the Center anchor and fall back to the wrong position.

Suggested fix
+fn same_overlay_target(a: &ReferencedImageTarget, b: &ReferencedImageTarget) -> bool {
+    a.sop_instance_uid == b.sop_instance_uid
+        && a.referenced_frames == b.referenced_frames
+}
+
 fn preferred_sr_overlay_label_anchor(
     geometry_nodes: &[&SrIndexedNode],
     image_index: &HashMap<Vec<usize>, ReferencedImageTarget>,
     reference: &ReferencedImageTarget,
 ) -> Option<((f32, f32), GspsUnits)> {
     geometry_nodes
         .iter()
         .copied()
         .filter(|node| {
             node.selected_image_target(image_index)
-                .is_some_and(|target| target == *reference)
+                .is_some_and(|target| same_overlay_target(&target, reference))
         })
         .find(|node| node.concept_name.matches_meaning("Center"))
         .and_then(sr_overlay_label_anchor)
         .or_else(|| {
             geometry_nodes
                 .iter()
                 .copied()
                 .filter(|node| {
                     node.selected_image_target(image_index)
-                        .is_some_and(|target| target == *reference)
+                        .is_some_and(|target| same_overlay_target(&target, reference))
                 })
                 .find_map(sr_overlay_label_anchor)
         })
 }
-            let mut labeled_targets = Vec::<ReferencedImageTarget>::new();
+            let mut labeled_targets = Vec::<ReferencedImageTarget>::new();

             for geometry_node in geometry_nodes.iter().copied() {
                 let Some(reference) = geometry_node.selected_image_target(image_index) else {
                     continue;
                 };

-                if labeled_targets.contains(&reference) {
+                if labeled_targets
+                    .iter()
+                    .any(|target| same_overlay_target(target, &reference))
+                {
                     continue;
                 }

                 // ...
-                labeled_targets.push(reference);
+                labeled_targets.push(reference);
             }

Also applies to: 764-787

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/dicom/sr.rs` around lines 485 - 532, The code currently deduplicates and
matches ReferencedImageTarget using full struct equality (used in
labeled_targets.contains(&reference) and inside
preferred_sr_overlay_label_anchor), but laterality/view are metadata and must
not affect identity; implement comparison by image/frame identity instead: add a
small helper (e.g. same_image_frame(a: &ReferencedImageTarget, b:
&ReferencedImageTarget) -> bool) that compares the SOP Instance UID and the
referenced frame identifiers (or referenced_frames contents), then use that
helper to check membership (replace labeled_targets.contains(&reference) with
labeled_targets.iter().any(|t| same_image_frame(t, &reference))) and update
preferred_sr_overlay_label_anchor to use the same helper when comparing targets
(replace target == *reference comparisons with same_image_frame(target,
&reference)); keep rest of logic (graphics/labels push) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/dicom/sr.rs`:
- Around line 485-532: The code currently deduplicates and matches
ReferencedImageTarget using full struct equality (used in
labeled_targets.contains(&reference) and inside
preferred_sr_overlay_label_anchor), but laterality/view are metadata and must
not affect identity; implement comparison by image/frame identity instead: add a
small helper (e.g. same_image_frame(a: &ReferencedImageTarget, b:
&ReferencedImageTarget) -> bool) that compares the SOP Instance UID and the
referenced frame identifiers (or referenced_frames contents), then use that
helper to check membership (replace labeled_targets.contains(&reference) with
labeled_targets.iter().any(|t| same_image_frame(t, &reference))) and update
preferred_sr_overlay_label_anchor to use the same helper when comparing targets
(replace target == *reference comparisons with same_image_frame(target,
&reference)); keep rest of logic (graphics/labels push) unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7eaefa38-7ac8-401c-b2aa-de44ad0eb6a7

📥 Commits

Reviewing files that changed from the base of the PR and between efc78e0 and 7781915.

📒 Files selected for processing (2)
  • src/app/measurement.rs
  • src/dicom/sr.rs
✅ Files skipped from review due to trivial changes (1)
  • src/app/measurement.rs

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/dicom/sr.rs`:
- Around line 522-527: preferred_sr_overlay_label_anchor may select a different
geometry node than the loop-local reference, but sr_overlay_label_lines(node,
&reference) still uses the original reference causing mismatched label text vs
anchor; update preferred_sr_overlay_label_anchor to also return the selected
target (or make the anchor include a reference to its source node) and then call
sr_overlay_label_lines(node, &selected_target) (i.e., feed the chosen target
returned with the anchor into sr_overlay_label_lines) so the label text is
generated from the same geometry node that supplied the anchor.
- Around line 461-464: The helper hydrate_image_context currently overwrites
ReferencedImageTarget.laterality and .view unconditionally by calling
SrIndexedNode.acquisition_context_value, which clears previously resolved values
when the node lacks one of those ACQ CONTEXT items; change hydrate_image_context
so it only assigns target.laterality if node.acquisition_context_value("Image
Laterality") returns Some(value) and only assigns target.view if
node.acquisition_context_value("Image View") returns Some(value), leaving
existing target fields untouched when the node omits them.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3bc54e94-a901-41ad-9de6-a7e30ee0ccca

📥 Commits

Reviewing files that changed from the base of the PR and between 7781915 and 97eaef0.

📒 Files selected for processing (1)
  • src/dicom/sr.rs

Comment thread src/dicom/sr.rs
Comment thread src/dicom/sr.rs Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/dicom/sr.rs`:
- Around line 225-228: When resolving referenced_content_item_identifier in the
block that finds reference = image_index.get(reference_path), ensure you hydrate
the image context on the target before returning so acquisition-context flags
(e.g., HAS ACQ CONTEXT) are applied; clone the referenced entry into a mutable
value, call hydrate_image_context(...) on that cloned/mutable target (using the
same helper and any required context/index), then return the hydrated clone
instead of returning reference.clone() directly. This change touches the
referenced_content_item_identifier lookup path and uses symbols: child,
referenced_content_item_identifier, image_index, hydrate_image_context, and the
returned reference.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: feb41414-fe49-4184-bd58-41194b35229f

📥 Commits

Reviewing files that changed from the base of the PR and between 97eaef0 and 55e3580.

📒 Files selected for processing (1)
  • src/dicom/sr.rs

Comment thread src/dicom/sr.rs
@timcogan timcogan merged commit 9b53106 into master Apr 16, 2026
7 checks passed
@timcogan timcogan deleted the fix/dicom-sr branch April 16, 2026 02:34
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.

1 participant