Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds 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
Sequence DiagramsequenceDiagram
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)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
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. TheSrOverlayLabel.anchoris a raw(f32, f32)tuple, andparse_spatial_coordinatesnever extractsGRAPHIC_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 toSrOverlayLabelor 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 usingtrim_end_matchesfor 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 inmammography_laterality_abbreviationandmammography_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
📒 Files selected for processing (8)
DESIGN.mdREADME.mdsrc/app.rssrc/app/measurement.rssrc/app/overlay.rssrc/dicom.rssrc/dicom/sr.rswebsite/content/docs/viewer-basics.md
There was a problem hiding this comment.
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 | 🟠 MajorEnrich inline
IMAGEtargets tooThese lines only hydrate
laterality/viewon the copy inserted intoimage_index. Whenselected_image_target()resolves a direct childIMAGEitem instead of aReferencedContentItemIdentifier, it still returns the rawReferencedImageTargetwith both fields unset, so those overlays lose the mammography context label and can also slip past the per-target de-dupe once equality includeslaterality/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 childIMAGE.🤖 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
📒 Files selected for processing (8)
DESIGN.mdREADME.mdsrc/app.rssrc/app/measurement.rssrc/app/overlay.rssrc/dicom.rssrc/dicom/sr.rswebsite/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
There was a problem hiding this comment.
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 | 🟠 MajorCompare label targets by image/frame identity, not full
ReferencedImageTarget.
ReferencedImageTargetequality now includeslateralityandview, 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 theCenteranchor 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
📒 Files selected for processing (2)
src/app/measurement.rssrc/dicom/sr.rs
✅ Files skipped from review due to trivial changes (1)
- src/app/measurement.rs
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
src/dicom/sr.rs
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
src/dicom/sr.rs
Summary by CodeRabbit
New Features
Documentation
Style