Merged
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds first-class Structured Report (SR) DICOM support: SR detection and parsing, new SR types/APIs, dedicated SR load/history/UI path, dicomweb displayable-content counting updates, docs/readme updates, and tests; load_dicom now rejects SR objects and SRs use a structured-report parser and single-document UI flow. Changes
Sequence DiagramsequenceDiagram
actor User
participant FileSystem
participant DicomClassifier
participant SRParser
participant AppState
participant UIRenderer
User->>FileSystem: Select DICOM path(s)
FileSystem-->>DicomClassifier: Provide path(s)
DicomClassifier->>DicomClassifier: Classify path(s) (Image / GSPS / StructuredReport)
alt StructuredReport detected
DicomClassifier-->>SRParser: Route SR path for parsing
SRParser->>SRParser: Parse StructuredReportDocument and metadata
SRParser-->>AppState: Return StructuredReportDocument
AppState->>AppState: Store report, create Report history entry, build thumbnail
AppState-->>UIRenderer: Trigger SR single-document view update
UIRenderer->>User: Display SR summary and nodes
else Images (with possible GSPS)
DicomClassifier-->>AppState: Route image/GSPS paths
AppState-->>UIRenderer: Render images in viewports (attach GSPS overlays)
UIRenderer->>User: Display images (and overlays)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app.rs (1)
2259-2284:⚠️ Potential issue | 🟡 MinorGeneralize the single-load error copy.
These messages still say “DICOM” even when the pending load is a
StructuredReport, so SR parse failures and worker disconnects will show the wrong object type to users.💬 Possible fix
Err(err) => { - self.set_load_error("Failed to load selected DICOM."); + self.set_load_error("Failed to load selected DICOM object."); log::error!("{err}"); } } self.single_load_receiver = None; ctx.request_repaint(); @@ Err(TryRecvError::Disconnected) => { self.single_load_receiver = None; - self.set_load_error("Selected DICOM load did not complete."); + self.set_load_error("Selected DICOM object load did not complete."); log::error!("Single-image load incomplete: worker exited before sending a result."); ctx.request_repaint(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 2259 - 2284, The error messages hardcode “DICOM” even though PendingSingleLoad can be Image or StructuredReport; update the error text used in the Err(err) and TryRecvError::Disconnected branches to a neutral phrase (e.g. “selected item” or “selected file”) so failures from both apply_loaded_single and apply_loaded_structured_report are described correctly, and update the corresponding log::error text accordingly; look for the match on result handling PendingSingleLoad, the Err(err) arm, and the TryRecvError::Disconnected arm in the same block to change self.set_load_error(...) and the log::error(...) strings to the generalized wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DESIGN.md`:
- Around line 28-36: Update the verification guidance section in DESIGN.md to
include SR-specific test cases: add a "SR-only open" case that verifies
Structured Reports are rejected by load_dicom for DicomPathKind::Other and
instead are parsed through the dedicated SR parser/single-document UI path
(reference load_dicom and the SR parser/single-document flow), and add a "mixed
image+SR selection" case that verifies mixed selections stage SR documents as
separate history entries (not image viewports), GSPS overlays attach only by SOP
Instance UID, GSPS visibility defaults off and is toggled by the `G` key, and
that streaming completion counts only images (not GSPS/SR paths); also ensure
instructions mention validating/clamping `open_group` and that UI state
mutations remain on the main thread while workers use channels.
In `@src/app.rs`:
- Around line 581-584: The current is_supported_prepared_group (and the similar
branch around the other occurrence) only allows report-only groups when
structured_report_paths.len() == 1, causing multi-SR groups to be rejected;
update the condition in is_supported_prepared_group (and the matching check in
the other block referenced) so that it accepts any non-empty
structured_report_paths when image_paths is empty (e.g., change the second
predicate to prepared.image_paths.is_empty() &&
!prepared.structured_report_paths.is_empty()), and ensure this aligns with
apply_prepared_load_paths handling of opening the first SR and staging the rest
into history.
- Around line 3048-3050: The CollapsingHeader instances created in
Self::show_structured_report_node are currently using the deprecated id_source()
(or relying on header text) which causes duplicate titles to share state; update
the function to call CollapsingHeader::new(...) then use .id_salt(path) where
`path` is a unique identifier per node (e.g., Vec or string built from node path
or index) instead of id_source(), or alternatively wrap the header creation in
ui.push_id(unique_id) / ui.pop_id to scope IDs; locate
show_structured_report_node and replace id_source usage (or add id_salt calls /
ui.push_id with node-specific id) so repeated labels no longer collide.
In `@src/dicom.rs`:
- Around line 352-363: load_dicom currently accepts Structured Report objects
and then fails with confusing pixel/rows errors; add the same upfront kind check
as in load_structured_report: after opening the object (open_dicom_object) call
classify_dicom_object(&obj) and if it equals DicomPathKind::StructuredReport
bail with a clear message (include the SOPClassUID via read_string(&obj,
"SOPClassUID").unwrap_or_else(|| "unknown".to_string())) that tells callers to
use load_structured_report() instead; update load_dicom to reference
classify_dicom_object, DicomPathKind::StructuredReport, and
load_structured_report in the error.
---
Outside diff comments:
In `@src/app.rs`:
- Around line 2259-2284: The error messages hardcode “DICOM” even though
PendingSingleLoad can be Image or StructuredReport; update the error text used
in the Err(err) and TryRecvError::Disconnected branches to a neutral phrase
(e.g. “selected item” or “selected file”) so failures from both
apply_loaded_single and apply_loaded_structured_report are described correctly,
and update the corresponding log::error text accordingly; look for the match on
result handling PendingSingleLoad, the Err(err) arm, and the
TryRecvError::Disconnected arm in the same block to change
self.set_load_error(...) and the log::error(...) strings to the generalized
wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bb2eb40-a5e9-4c11-b4bc-50f2f752a130
📒 Files selected for processing (4)
DESIGN.mdREADME.mdsrc/app.rssrc/dicom.rs
Comment on lines
+28
to
+36
| 3. Non-image DICOM objects (`DicomPathKind::Other`) and Structured Reports MUST NOT be passed to `load_dicom`. | ||
| 4. Structured Reports MUST load through the dedicated SR parser and single-document UI path. | ||
| 5. Mixed image+SR selections MUST stage SR documents as separate history entries, not image viewports. | ||
| 6. GSPS visibility MUST default to off and MUST be user-toggled (`G`). | ||
| 7. GSPS overlays MUST attach by SOP Instance UID match only. | ||
| 8. `open_group` MUST be validated/clamped before use. | ||
| 9. Streaming completion logic MUST compare image counts (not total paths including GSPS/SR). | ||
| 10. UI state mutations MUST stay on the main thread; workers MUST communicate through channels. | ||
| 11. Production diagnostics MUST use logging (`log` macros), not `println!/eprintln!`. |
There was a problem hiding this comment.
Add SR-specific checks to the verification guidance.
These invariants introduce a dedicated SR parser/UI path and mixed image+SR history behavior, but the verification guidance later in DESIGN.md still only calls out GSPS/history checks. Please add at least one SR-only open case and one mixed image+SR selection case so this document stays complete.
As per coding guidelines: "Keep DESIGN.md updated when architecture, module ownership, invariants, or core data flows change".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DESIGN.md` around lines 28 - 36, Update the verification guidance section in
DESIGN.md to include SR-specific test cases: add a "SR-only open" case that
verifies Structured Reports are rejected by load_dicom for DicomPathKind::Other
and instead are parsed through the dedicated SR parser/single-document UI path
(reference load_dicom and the SR parser/single-document flow), and add a "mixed
image+SR selection" case that verifies mixed selections stage SR documents as
separate history entries (not image viewports), GSPS overlays attach only by SOP
Instance UID, GSPS visibility defaults off and is toggled by the `G` key, and
that streaming completion counts only images (not GSPS/SR paths); also ensure
instructions mention validating/clamping `open_group` and that UI state
mutations remain on the main thread while workers use channels.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/dicom.rs (1)
365-378: Consider caching SOPClassUID to avoid duplicate reads.
read_string(obj, "SOPClassUID")is called twice (lines 366 and 369). While the overhead is minimal, caching the value once would be slightly more efficient.♻️ Suggested refactor
fn classify_dicom_object(obj: &DefaultDicomObject) -> DicomPathKind { - if read_string(obj, "SOPClassUID").is_some_and(|uid| is_gsps_sop_class_uid(&uid)) { + let sop_class_uid = read_string(obj, "SOPClassUID"); + if sop_class_uid.as_ref().is_some_and(|uid| is_gsps_sop_class_uid(uid)) { return DicomPathKind::Gsps; } - if read_string(obj, "SOPClassUID").is_some_and(|uid| is_structured_report_sop_class_uid(&uid)) + if sop_class_uid.is_some_and(|uid| is_structured_report_sop_class_uid(&uid)) || read_string(obj, "Modality").is_some_and(|value| value.eq_ignore_ascii_case("SR")) { return DicomPathKind::StructuredReport; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dicom.rs` around lines 365 - 378, In classify_dicom_object, avoid calling read_string(obj, "SOPClassUID") twice; read and cache the SOPClassUID once into a local Option<String> (e.g. let sop = read_string(obj, "SOPClassUID")), then reuse sop.as_deref() or is_some_and closures when checking is_gsps_sop_class_uid and is_structured_report_sop_class_uid; keep the existing fallback check for Modality and PixelData unchanged so only the SOPClassUID lookups are consolidated.
🤖 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 2427-2443: stage_structured_report_history_entries currently does
synchronous file I/O and SR parsing by calling load_structured_report on the
egui thread; instead, enqueue each report path into the existing background
preload pipeline used by apply_prepared_load_paths so parsing happens off the UI
thread, and only call push_report_history_entry from the UI thread when the
background task completes (e.g., send the parsed report back over the existing
preload/completion channel or schedule a UI callback via ctx). Replace direct
calls to load_structured_report and direct push_report_history_entry in
stage_structured_report_history_entries with logic that posts the PathBuf to the
preload worker and handles the worker's completion to perform
push_report_history_entry on the egui thread.
- Around line 1910-1914: The Structured Report arm currently logs and drops SRs
(match on DicomPathKind::StructuredReport) causing loss; change that branch to
stage the SR into history the same way Local/Prepared opens do by creating a
HistoryPreloadResult::Report and inserting it into the same history/preload
collection used elsewhere (so the preallocated mammo group slots and
selected_instances_by_group counts remain consistent). Update the streaming
active-group logic in src/app.rs to push the staged report into the history
container and adjust any counters/indices used by the streaming path (the code
that preallocates using selected_instances_by_group[open_group].len()) so SRs
occupy their reserved slot rather than being discarded.
---
Nitpick comments:
In `@src/dicom.rs`:
- Around line 365-378: In classify_dicom_object, avoid calling read_string(obj,
"SOPClassUID") twice; read and cache the SOPClassUID once into a local
Option<String> (e.g. let sop = read_string(obj, "SOPClassUID")), then reuse
sop.as_deref() or is_some_and closures when checking is_gsps_sop_class_uid and
is_structured_report_sop_class_uid; keep the existing fallback check for
Modality and PixelData unchanged so only the SOPClassUID lookups are
consolidated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d052b9bb-cfe7-49c2-af1a-12f71221db49
📒 Files selected for processing (3)
DESIGN.mdsrc/app.rssrc/dicom.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/app.rs (1)
1904-1908:⚠️ Potential issue | 🟠 MajorUse displayable image count for streamed active-group state.
This stages streamed SRs correctly, but the active-group UI is still sized before the paths are classified. A mixed
1 image + 1 SRstream will enter the multi-view path because of the raw instance count, never reachmammo_group_complete(), and never fall back to the single-image viewer even thoughpoll_dicomweb_download()later compares againstprepared.image_paths.len() == 1. Please key the provisional group off displayable-image count, or tear it down once classification shows the active group is single-image / SR-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1904 - 1908, The provisional active-group sizing uses raw instance counts causing mixed streams (image + SR) to pick multi-view; when enqueueing SRs in the DicomPathKind::StructuredReport branch (enqueue_history_preload_job with HistoryPreloadJob::StructuredReport) change logic so the provisional group decision uses the displayable-image count (not raw instance count) — e.g., update the staging/path-classification code path that sets up the active group to consult the displayable image count or immediately tear down/replace the provisional group once classification completes (code paths referenced: mammo_group_complete, poll_dicomweb_download which compares prepared.image_paths.len() == 1). Ensure after classification you recalc displayable images and either switch to single-image viewer or call the teardown flow so SR-only or single-image groups render correctly.
🤖 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 2559-2573: apply_loaded_structured_report currently doesn't clear
the active mammo group so opening a Structured Report leaves mammo_group
populated and the central panel still renders the grid; update
apply_loaded_structured_report to mirror open_history_entry(HistoryKind::Report)
by clearing the mammography group state before setting self.report—i.e., call
the same clearing helper (e.g., self.clear_mammo_group()) or explicitly set
self.mammo_group to None and reset any related mammo selection/flags so
has_mammo_group() becomes false, then proceed with push_report_history_entry,
setting self.report/self.current_single_path and repaint.
In `@src/dicomweb.rs`:
- Around line 155-168: The code currently computes displayable_image_count via
displayable_group_image_count(&selected_instances) and then permits SR-only
groups by checking has_displayable_group_content(&selected_instances) but still
emits ActiveGroupInstanceCount(0), which misrepresents SR-only progress; change
the emission logic so the ActiveGroupInstanceCount reflects all displayable
items (include structured reports/documents in the count) or else do not emit
ActiveGroupInstanceCount when the group contains only SR/document items and
instead emit a distinct signal (e.g., ActiveStructuredReportCount or skip
ActiveGroupInstanceCount) — update the code paths that call
displayable_group_image_count, has_displayable_group_content, and the
ActiveGroupInstanceCount emission to use a combined total_displayable_count
(images + SR/docs) or gate the ActiveGroupInstanceCount emission behind
displayable_image_count > 0.
---
Duplicate comments:
In `@src/app.rs`:
- Around line 1904-1908: The provisional active-group sizing uses raw instance
counts causing mixed streams (image + SR) to pick multi-view; when enqueueing
SRs in the DicomPathKind::StructuredReport branch (enqueue_history_preload_job
with HistoryPreloadJob::StructuredReport) change logic so the provisional group
decision uses the displayable-image count (not raw instance count) — e.g.,
update the staging/path-classification code path that sets up the active group
to consult the displayable image count or immediately tear down/replace the
provisional group once classification completes (code paths referenced:
mammo_group_complete, poll_dicomweb_download which compares
prepared.image_paths.len() == 1). Ensure after classification you recalc
displayable images and either switch to single-image viewer or call the teardown
flow so SR-only or single-image groups render correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ba97055-d6eb-400e-bf7d-7686670aaec6
📒 Files selected for processing (3)
src/app.rssrc/dicom.rssrc/dicomweb.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/app.rs`:
- Around line 1904-1913: The current flow lets load_selected_paths clear
history_preload_receiver/history_preload_queue when handling a streamed single
image, which can drop an already-enqueued SR (enqueued via
enqueue_history_preload_job/HistoryPreloadJob::StructuredReport) — instead,
introduce a separate staging queue for streamed SR sidecars (e.g.,
history_preload_sidecar_queue) and push StructuredReport jobs into that queue
via enqueue_history_preload_job; update load_selected_paths to avoid clearing
the main history_preload_receiver/history_preload_queue for single-image
arrivals and only clear/drain preload state when the active group
(dicomweb_active_group_paths) is fully drained or when SR sidecar queue has been
merged/applied, ensuring SR jobs are preserved regardless of arrival order.
In `@src/dicomweb.rs`:
- Around line 250-269: The function metadata_instance_kind currently defaults to
returning DicomPathKind::Image for anything not GSPS or StructuredReport; change
it to default to DicomPathKind::Other and only return DicomPathKind::Image when
the metadata explicitly signals an image (e.g., a known image SOP Class UID or a
modality value that positively indicates an image). Update
metadata_instance_kind to (1) keep the existing GSPS and StructuredReport
checks, (2) add an explicit positive image check using instance.sop_class_uid
and/or instance.modality (only when modality clearly indicates an image), and
(3) return DicomPathKind::Other as the final fallback instead of
DicomPathKind::Image.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a85caa0-a728-4465-8d80-1f2db5aa9cf0
📒 Files selected for processing (2)
src/app.rssrc/dicomweb.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/dicomweb.rs`:
- Around line 450-451: The sop_class_uid and modality fields are assigned raw
values from first_tag_string which preserves DICOM padding, causing values like
"SR " or "MG " to fail later equality checks; update the assignments (where
sop_class_uid and modality are set) to normalize these strings by trimming DICOM
padding/whitespace (e.g., call a normalization/trim helper or apply .trim()
semantics to first_tag_string's result) so SOPClassUID and Modality are compared
against normalized values throughout the code (ensure any downstream comparisons
use the trimmed/normalized sop_class_uid and modality).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2d30de5-5826-49b3-b070-886ad4ae97e8
📒 Files selected for processing (2)
src/app.rssrc/dicomweb.rs
This was referenced Mar 13, 2026
This was referenced Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests