feat(viewer): add GSPS overlay support with toggle, docs, and tests#23
Merged
feat(viewer): add GSPS overlay support with toggle, docs, and tests#23
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 GSPS (Grayscale Softcopy Presentation State) support: public GSPS types and parsing in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant App
participant DicomModule
participant FS as FileSystem
User->>UI: select files / press G
UI->>App: load_paths(paths) / toggle_gsps()
App->>FS: read file bytes
FS-->>App: file bytes
App->>DicomModule: classify_dicom_path(file)
alt GSPS file
App->>DicomModule: load_gsps_overlays(file)
DicomModule-->>App: map<SOP UID, GspsOverlay>
App->>App: merge/attach overlays to images by SOP UID
else Image file
App->>DicomModule: load_dicom(file)
DicomModule-->>App: DicomImage (with sop_instance_uid)
end
App->>UI: update state (pending overlays, active overlay)
UI->>App: toggle_gsps()
App->>UI: render(image + overlay if visible)
UI-->>User: display result
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. 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: 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/app.rs`:
- Around line 1137-1151: The loop currently discards GSPS by pushing only
prepared.image_paths into preload_groups; instead preserve GSPS by storing the
full prepared result (or both image_paths and gsps_paths) so overlays are
available when preloading non-active groups—update the preload_groups collection
to hold the prepared struct (from prepare_load_paths) and modify
preload_non_active_groups_into_history to accept and use the GSPS/overlay paths;
keep load_selected_paths usage for the active group unchanged but ensure
prepared (with GSPS) is passed into the history preload call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a490649f-5fe0-4c35-a9d8-a3bff9adca8a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlREADME.mdsrc/app.rssrc/dicom.rswebsite/content/docs/launch-options.md
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 1231-1232: When new streamed GSPS are merged into
pending_gsps_overlays (the code path that updates pending_gsps_overlays),
iterate the already-loaded images in the relevant mammo_group and call
Self::attach_matching_gsps_overlay(&mut image, &self.pending_gsps_overlays) for
each so late-arriving GSPS get attached to existing viewports; specifically, add
a loop that finds the same collection of images you use when inserting
pending.image and invoke attach_matching_gsps_overlay for each image instance
after merging GSPS into pending_gsps_overlays.
- Around line 1397-1399: The completion logic uses active_group_len (derived
from self.dicomweb_active_group_paths) but streamed_count only increments for
non-GSPS images, causing mismatches when groups contain GSPS; update the logic
where active_group_len is computed (used around active_group_len / lines
~1553-1560) to count only image paths that will be streamed (exclude GSPS) so it
matches the streamed_count behavior, and ensure the same filtered count is used
in any spawned thread that references dicomweb_active_group_paths (e.g., the
block started from mammo_load_sender thread spawn at dicomweb_active_group_paths
push) so completion/cleanup conditions compare like-for-like.
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/app.rs (1)
1561-1582:⚠️ Potential issue | 🟡 MinorNormalize
open_groupbefore preloading non-active groups.
active_group_lenis read safely via.get(open_group), but Line 1579 still passes rawopen_groupintopreload_non_active_groups_into_history. Ifopen_groupis out of bounds, non-active filtering can be wrong and may preload the active group unintentionally.💡 Proposed fix
DicomWebDownloadResult::Grouped { groups, open_group } => { let prepared_groups = groups .iter() .map(|group| Self::prepare_load_paths(group.clone())) .collect::<Vec<_>>(); - let active_group_len = prepared_groups - .get(open_group) + let active_group = open_group.min(prepared_groups.len().saturating_sub(1)); + let active_group_len = prepared_groups + .get(active_group) .map(|group| group.image_paths.len()) .unwrap_or(0); @@ - self.preload_non_active_groups_into_history( - &prepared_groups, - open_group, - ); + self.preload_non_active_groups_into_history( + &prepared_groups, + active_group, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1561 - 1582, The code reads active_group_len using prepared_groups.get(open_group) but then passes the raw open_group into preload_non_active_groups_into_history, which can be out-of-bounds and cause the active group to be treated as non-active; validate/normalize open_group against prepared_groups before calling preload_non_active_groups_into_history (e.g., compute let validated_open = if prepared_groups.get(open_group).is_some() { open_group } else { 0 } or similar) and use validated_open in the call to preload_non_active_groups_into_history, keeping the existing prepared_groups and Self::prepare_load_paths usage.
🤖 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/app.rs`:
- Around line 1561-1582: The code reads active_group_len using
prepared_groups.get(open_group) but then passes the raw open_group into
preload_non_active_groups_into_history, which can be out-of-bounds and cause the
active group to be treated as non-active; validate/normalize open_group against
prepared_groups before calling preload_non_active_groups_into_history (e.g.,
compute let validated_open = if prepared_groups.get(open_group).is_some() {
open_group } else { 0 } or similar) and use validated_open in the call to
preload_non_active_groups_into_history, keeping the existing prepared_groups and
Self::prepare_load_paths usage.
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 281-283: The match arm incorrectly treats Ok(DicomPathKind::Other)
as an image and pushes it into prepared.image_paths; update the match in the
code handling DicomPathKind so only Ok(DicomPathKind::Image) (and Err(_) if
intended) add to prepared.image_paths and ensure Ok(DicomPathKind::Other) is
excluded (e.g., handled separately or ignored) so non-image DICOM objects are
not fed into load_dicom; locate the match that references DicomPathKind and
prepared.image_paths and adjust the arms accordingly.
- Around line 1384-1396: Currently merge_gsps_overlays +
attach_matching_gsps_overlay only update live mammo_group images (via
pending_gsps_overlays), so cached HistoryGroupViewportData.image can miss late
GSPS overlays; update sync_current_state_to_history to, after merging
pending_gsps_overlays, iterate the active group's history entries and call
attach_matching_gsps_overlay(&mut history_entry.ViewportData.image,
&self.pending_gsps_overlays) for each HistoryGroupViewportData to keep cached
images in sync with live mammo_group; ensure you call merge_gsps_overlays before
applying overlays so pending_gsps_overlays contains the latest combined
overlays.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/app.rs (1)
1386-1436:⚠️ Potential issue | 🟠 MajorSkip non-image streamed objects before spawning image decode.
At Line 1386, only
Gspsis filtered; every other kind currently flows intoload_dicom. That includesDicomPathKind::Other, which can fail decode and abort an otherwise valid stream. Mirrorprepare_load_pathsbehavior here and only spawn decode forImage(optionallyErr(_)fallback if intentional), while ignoring/loggingOther.💡 Suggested fix
- if classify_dicom_path(&path).is_ok_and(|kind| kind == DicomPathKind::Gsps) { - match load_gsps_overlays(&path) { + match classify_dicom_path(&path) { + Ok(DicomPathKind::Gsps) => match load_gsps_overlays(&path) { Ok(overlays) => { Self::merge_gsps_overlays( &mut self.pending_gsps_overlays, overlays, ); for viewport in self.mammo_group.iter_mut().filter_map(Option::as_mut) { Self::attach_matching_gsps_overlay( &mut viewport.image, &self.pending_gsps_overlays, ); } self.sync_current_state_to_history(); } Err(err) => { eprintln!( "Could not parse streamed GSPS {}: {err:#}", path.display() ); } - } - } else { + }, + Ok(DicomPathKind::Image) | Err(_) => { self.dicomweb_active_group_paths.push(path.clone()); if let Some(sender) = self.mammo_load_sender.as_ref().cloned() { thread::spawn(move || { let result = match load_dicom(&path) { Ok(image) => Ok(PendingLoad { path, image }), Err(err) => Err(format!( "Error opening streamed DICOM {}: {err:#}", path.display() )), }; let _ = sender.send(result); }); } else { self.status_line = "Streaming multi-view load channel not available.".to_string(); self.mammo_group.clear(); self.mammo_load_receiver = None; self.mammo_load_sender = None; self.history_pushed_for_active_group = false; self.cine_mode = false; self.dicomweb_active_group_paths.clear(); self.dicomweb_active_pending_paths.clear(); self.dicomweb_active_group_expected = None; self.dicomweb_active_path_receiver = None; } - } + } + Ok(DicomPathKind::Other) => { + eprintln!("Skipping streamed non-image DICOM object {}", path.display()); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1386 - 1436, The code currently treats any non-GSPS path as an image and spawns load_dicom; update the else branch to re-check classify_dicom_path(&path) and only spawn the decode thread when the result is DicomPathKind::Image (optionally allow Err(_) if you want to attempt unknowns), and for DicomPathKind::Other skip spawning and log/ignore the path; modify the block handling self.mammo_load_sender (and the branch that currently calls load_dicom) to match on classify_dicom_path(&path) and only call load_dicom for Image, leaving GSPS handling unchanged and mirroring the prepare_load_paths behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/app.rs`:
- Around line 1386-1436: The code currently treats any non-GSPS path as an image
and spawns load_dicom; update the else branch to re-check
classify_dicom_path(&path) and only spawn the decode thread when the result is
DicomPathKind::Image (optionally allow Err(_) if you want to attempt unknowns),
and for DicomPathKind::Other skip spawning and log/ignore the path; modify the
block handling self.mammo_load_sender (and the branch that currently calls
load_dicom) to match on classify_dicom_path(&path) and only call load_dicom for
Image, leaving GSPS handling unchanged and mirroring the prepare_load_paths
behavior.
This was referenced Mar 7, 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
Chores