Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. ✨ Finishing Touches
🧪 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 |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app.rs (2)
1773-1779:⚠️ Potential issue | 🟠 MajorAvoid logging raw local DICOM paths.
Line 1850 and Line 1901 log
path.display()directly, and Line 1778 forwards worker errors that already embed the path. For a medical viewer, local paths can contain patient identifiers, so this change moves potentially sensitive data from transient UI state into durable logs.Also applies to: 1850-1850, 1901-1901
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1773 - 1779, The code logs raw local DICOM paths (including in the receiver.try_recv() error branch and other places that call path.display()), which may expose patient-identifying data; update the error logging in the receiver.try_recv() match (the Err(err) arm) and any direct uses of path.display() to avoid writing the full path to durable logs: instead log a generic, non-identifying message (e.g., "failed to load image" or "path redacted"), or redact the path (keep only a hashed/obfuscated identifier) before including it; specifically modify the match around receiver.try_recv(), the call site that invokes apply_loaded_single(pending.path, ...), and the two other log sites that directly use path.display() so they no longer emit raw filesystem paths.
1773-1791:⚠️ Potential issue | 🟠 MajorKeep a user-visible error path for failed opens and invalid selections.
These branches now only write to the logger. In the GUI, a bad file / unsupported selection then falls back to the same generic empty state, so the user gets no on-screen explanation of what failed or how to recover.
Also applies to: 1818-1841
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1773 - 1791, Update the error-handling branches of the receiver.try_recv() match to set a user-visible error state instead of only logging: when result is Err(_) or when Err(TryRecvError::Disconnected) assign a descriptive message to a new or existing field (e.g. self.single_load_error: Option<String>) and call ctx.request_repaint(); ensure the success path in Ok(Ok(pending)) clears that field (e.g. self.single_load_error = None) after calling self.apply_loaded_single(pending.path, pending.image, ctx). Apply the same change to the other analogous receiver block referenced (lines ~1818-1841) so all failed opens/invalid selections populate the UI-visible error state rather than only writing to log.
🤖 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/main.rs`:
- Around line 13-18: The code currently swallows parse_launch_request_from_args
errors (in the initial_request match) and continues startup; change this to fail
fast or surface the error: when
launch::parse_launch_request_from_args(&cli_args) returns Err(err) do not return
None silently—either call process::exit with a non-zero status or propagate the
error so the caller can abort before window creation, or invoke your UI error
display routine to show the failure to the user; update the handling of
initial_request so invalid launches do not result in a blank window.
---
Outside diff comments:
In `@src/app.rs`:
- Around line 1773-1779: The code logs raw local DICOM paths (including in the
receiver.try_recv() error branch and other places that call path.display()),
which may expose patient-identifying data; update the error logging in the
receiver.try_recv() match (the Err(err) arm) and any direct uses of
path.display() to avoid writing the full path to durable logs: instead log a
generic, non-identifying message (e.g., "failed to load image" or "path
redacted"), or redact the path (keep only a hashed/obfuscated identifier) before
including it; specifically modify the match around receiver.try_recv(), the call
site that invokes apply_loaded_single(pending.path, ...), and the two other log
sites that directly use path.display() so they no longer emit raw filesystem
paths.
- Around line 1773-1791: Update the error-handling branches of the
receiver.try_recv() match to set a user-visible error state instead of only
logging: when result is Err(_) or when Err(TryRecvError::Disconnected) assign a
descriptive message to a new or existing field (e.g. self.single_load_error:
Option<String>) and call ctx.request_repaint(); ensure the success path in
Ok(Ok(pending)) clears that field (e.g. self.single_load_error = None) after
calling self.apply_loaded_single(pending.path, pending.image, ctx). Apply the
same change to the other analogous receiver block referenced (lines ~1818-1841)
so all failed opens/invalid selections populate the UI-visible error state
rather than only writing to log.
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 (2)
src/app.rs (2)
1039-1065:⚠️ Potential issue | 🟡 MinorClear stale load errors after successful history restores.
A previous
load_error_messagesurvives both cache-hit paths here, so the error banner can stay visible even after the user opens a valid study from history.Suggested fix
self.rebuild_texture(ctx); + self.clear_load_error(); log::info!("Loaded study from memory cache."); ctx.request_repaint(); @@ self.mammo_selected_index = selected_index .unwrap_or(group.selected_index) .min(self.mammo_group.len().saturating_sub(1)); + self.clear_load_error(); log::info!("Loaded grouped study from memory cache."); ctx.request_repaint();Also applies to: 1097-1105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1039 - 1065, The history-restore handlers (e.g., the HistoryKind::Single branch and the other history branch around lines 1097-1105) don't clear self.load_error_message after a successful restore, leaving stale error banners visible; update both restore-success paths to set self.load_error_message = None once the image/series has been loaded and view state reset (before calling self.rebuild_texture and ctx.request_repaint) so that any prior load error is cleared; refer to symbols HistoryKind::Single, load_error_message, rebuild_texture, ctx.request_repaint, reset_single_view_transform (and the analogous reset for the other branch) when making the change.
1588-1646:⚠️ Potential issue | 🟠 MajorDon't mark grouped DICOMweb loads successful before the active study is actually ready.
This branch runs when the grouped download worker finishes, but the active group may still be decoding, may have failed validation in
load_local_groups, or may already have set a user-visible error from the streaming path. The unconditionalclear_load_error()and success log here can therefore erase a real failure and emit a false success signal.Suggested fix
- self.clear_load_error(); - log::info!("Loaded grouped study from DICOMweb.");Move the success clear/log to the branches that actually finish loading the active study.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1588 - 1646, The code unconditionally calls clear_load_error() and logs success at the end of the DicomWebDownloadResult::Grouped handling, which can erase real failures; instead, remove the unconditional clear_load_error() and log::info! call and move them into the concrete success paths: after a successful local load returned by load_local_groups (i.e., the branch where you call self.load_local_groups(...)) and into the branch(s) where the active group is known-ready (the path where streamed_active_complete is true or where you finish preparing the active group via preload_non_active_groups_into_history, mammo_group_complete(), reorder_complete_mammo_group(), push_group_history_entry(), and move_current_history_to_front()). Ensure clear_load_error() and the log::info!("Loaded grouped study from DICOMweb.") are only invoked when the active study is actually ready and not in the general end-of-branch cleanup.
🤖 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 1039-1065: The history-restore handlers (e.g., the
HistoryKind::Single branch and the other history branch around lines 1097-1105)
don't clear self.load_error_message after a successful restore, leaving stale
error banners visible; update both restore-success paths to set
self.load_error_message = None once the image/series has been loaded and view
state reset (before calling self.rebuild_texture and ctx.request_repaint) so
that any prior load error is cleared; refer to symbols HistoryKind::Single,
load_error_message, rebuild_texture, ctx.request_repaint,
reset_single_view_transform (and the analogous reset for the other branch) when
making the change.
- Around line 1588-1646: The code unconditionally calls clear_load_error() and
logs success at the end of the DicomWebDownloadResult::Grouped handling, which
can erase real failures; instead, remove the unconditional clear_load_error()
and log::info! call and move them into the concrete success paths: after a
successful local load returned by load_local_groups (i.e., the branch where you
call self.load_local_groups(...)) and into the branch(s) where the active group
is known-ready (the path where streamed_active_complete is true or where you
finish preparing the active group via preload_non_active_groups_into_history,
mammo_group_complete(), reorder_complete_mammo_group(),
push_group_history_entry(), and move_current_history_to_front()). Ensure
clear_load_error() and the log::info!("Loaded grouped study from DICOMweb.") are
only invoked when the active study is actually ready and not in the general
end-of-branch cleanup.
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1579-1589:⚠️ Potential issue | 🟠 MajorDon't clear the error state before the single DICOMweb load actually succeeds.
load_selected_pathscan still fail synchronously here. The unconditionalclear_load_error()then erases that failure beforepoll_single_loadhas produced a real success.💡 Suggested change
DicomWebDownloadResult::Single(paths) => { self.load_selected_paths(paths, ctx); self.dicomweb_active_group_expected = None; self.dicomweb_active_group_paths.clear(); self.dicomweb_active_pending_paths.clear(); self.dicomweb_active_path_receiver = None; self.mammo_load_sender = None; self.history_pushed_for_active_group = false; - self.clear_load_error(); - log::info!("Loaded study from DICOMweb."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1579 - 1589, The code unconditionally calls clear_load_error() after invoking load_selected_paths() inside the DicomWebDownloadResult::Single branch, but load_selected_paths may fail synchronously; change this so you only clear the error when the single DICOMweb load truly succeeds: have the DicomWebDownloadResult::Single branch inspect the result/return value of load_selected_paths (or propagate its Result), and on Err leave the existing error state and avoid resetting dicomweb_active_* flags; only call clear_load_error() (and reset dicomweb_active_group_expected, dicomweb_active_group_paths, dicomweb_active_pending_paths, dicomweb_active_path_receiver, mammo_load_sender, history_pushed_for_active_group) after poll_single_load confirms success (or when load_selected_paths returns Ok). Ensure you update error handling paths in load_selected_paths, poll_single_load, and the Single branch so failures are preserved.
🤖 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 1858-1864: When paths.is_empty() currently only handles the GSPS
case, add a branch to also report when the user's selection consisted solely of
DicomPathKind::Other items so it doesn't silently return: detect this by
checking prepared (e.g. a prepared.other_files_found or by inspecting the
original selection for DicomPathKind::Other), and in that case call
self.set_load_error(...) with a user-facing message, log::warn!(...) with the
same context, and call ctx.request_repaint(); keep the existing GSPS handling
intact (use the same symbols: paths, prepared, DicomPathKind::Other,
set_load_error, log::warn, ctx.request_repaint).
---
Outside diff comments:
In `@src/app.rs`:
- Around line 1579-1589: The code unconditionally calls clear_load_error() after
invoking load_selected_paths() inside the DicomWebDownloadResult::Single branch,
but load_selected_paths may fail synchronously; change this so you only clear
the error when the single DICOMweb load truly succeeds: have the
DicomWebDownloadResult::Single branch inspect the result/return value of
load_selected_paths (or propagate its Result), and on Err leave the existing
error state and avoid resetting dicomweb_active_* flags; only call
clear_load_error() (and reset dicomweb_active_group_expected,
dicomweb_active_group_paths, dicomweb_active_pending_paths,
dicomweb_active_path_receiver, mammo_load_sender,
history_pushed_for_active_group) after poll_single_load confirms success (or
when load_selected_paths returns Ok). Ensure you update error handling paths in
load_selected_paths, poll_single_load, and the Single branch so failures are
preserved.
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