Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds lazy per-frame storage and background preload for DICOM images, and converts single-image loading to an asynchronous channel-based flow delivering a new internal Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(240,248,255,0.5)
actor UI as UI/App
participant BG as BackgroundThread
participant Loader as DICOM Loader
participant Cache as Frame Cache
participant Renderer as Renderer
end
UI->>BG: spawn load_path(path)
BG->>Loader: load_dicom(path) (decode frame 0)
Loader->>Loader: init frames (Eager or Lazy)
alt multiple frames -> lazy
Loader->>Cache: create LazyFrames & start preload workers
Cache->>Cache: preload workers populate frame cache
end
Loader-->>BG: return PendingLoad(path, image)
BG-->>UI: send Result<PendingLoad, String> via channel
UI->>UI: poll_single_load()
UI->>UI: apply_loaded_single(path, image)
UI->>Cache: frame_mono_pixels(frame_idx) / frame_rgb_pixels(frame_idx)
Cache-->>Renderer: Arc<[pixels]>
Renderer->>Renderer: create texture & render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/app.rs (2)
45-53:PendingSingleLoadis structurally identical toPendingMammoLoad— consider unifying.Both structs have exactly
path: PathBufandimage: DicomImage. A singlePendingLoadtype (or a type alias) could serve both channels.♻️ Proposed unification
-struct PendingMammoLoad { - path: PathBuf, - image: DicomImage, -} - -struct PendingSingleLoad { - path: PathBuf, - image: DicomImage, -} +struct PendingLoad { + path: PathBuf, + image: DicomImage, +} +type PendingMammoLoad = PendingLoad; +type PendingSingleLoad = PendingLoad;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 45 - 53, PendingMammoLoad and PendingSingleLoad are identical; replace them with a single unified type (e.g., PendingLoad) that contains path: PathBuf and image: DicomImage, and remove the duplicate structs; update all references/usages (channel types, send/recv sites, function signatures and pattern matches) that mention PendingMammoLoad or PendingSingleLoad to use PendingLoad instead so the code compiles and both channels share the same payload type.
1505-1519: Minor:self.image.clone()on Line 1512 can be avoided by cloning before the move.
imageis moved intoself.imageat Line 1505, thenself.image.clone()retrieves it again. CloningDicomImageis cheap (all frame data is behindArc), so this isn't a performance concern, but the intent is clearer if you keep a reference before the move:♻️ Optional cleanup
+ let history_image = image.clone(); self.image = Some(image); self.current_single_path = Some(path.clone()); self.mammo_group.clear(); self.mammo_selected_index = 0; self.reset_single_view_transform(); self.single_view_frame_scroll_accum = 0.0; self.rebuild_texture(ctx); - let history_image = self.image.clone(); let history_texture = self.texture.clone();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1505 - 1519, Before moving `image` into `self.image`, clone it into a local variable and use that clone when later building the history entry; specifically, capture a cloned `image` (e.g. `let history_image = image.clone()`) prior to `self.image = Some(image)`, then proceed with `self.image = Some(image)` and use that `history_image` when constructing the `HistorySingleData` passed to `push_single_history_entry` so you avoid calling `self.image.clone()` after the move.src/dicom.rs (2)
69-95: Significant duplication between mono and RGB lazy paths — consider a generic abstraction.
LazyMonoFrames/LazyRgbFrames,decode_mono_frame_from_path/decode_rgb_frame_from_path, andpreload_mono_frames_from_path/preload_rgb_frames_from_pathare structurally identical, differing only in pixel type (i32vsu8). The two preload functions alone account for ~150 lines of near-duplicate logic. Any fix applied to one path (e.g., theto_vec_frameconcern above) must be manually mirrored to the other.A lightweight approach is a single generic lazy struct and a single preload helper parameterized over the pixel type:
♻️ Sketch of a generic unification
-struct LazyMonoFrames { ... } -struct LazyRgbFrames { ... } -impl LazyMonoFrames { fn frame(...) ... fn ensure_background_preload(...) ... } -impl LazyRgbFrames { fn frame(...) ... fn ensure_background_preload(...) ... } +struct LazyFrames<T: Copy + Send + Sync + 'static> { + path: PathBuf, + cache: Arc<Mutex<Vec<Option<Arc<[T]>>>>>, + preload_started: Arc<AtomicBool>, + decode_fn: fn(&Path, usize) -> Result<Vec<T>>, +} +impl<T: Copy + Send + Sync + 'static> LazyFrames<T> { + fn frame(&self, frame_index: usize) -> Option<Arc<[T]>> { ... } + fn ensure_background_preload(&self) { ... } +}Also applies to: 123-193, 375-573
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dicom.rs` around lines 69 - 95, The Mono vs RGB lazy-loading code is duplicated; replace LazyMonoFrames/LazyRgbFrames and the paired functions decode_mono_frame_from_path/decode_rgb_frame_from_path and preload_mono_frames_from_path/preload_rgb_frames_from_path with a single generic LazyFrames<T> type and generic helper functions (e.g., decode_frame_from_path<T> and preload_frames_from_path<T>) parameterized on the pixel element type; keep the same fields (path, cache, preload_started) but make cache hold Arc<[T]> and use trait bounds required by your decoding logic (Send + Sync + 'static and any conversion traits you currently use), update MonoFrames/RgbFrames enum variants to use the generic LazyFrames<T> (or replace with a single enum parametrized by T), and move any duplicated preload/decode logic into the generic implementations so both mono (i32) and rgb (u8) code paths call the same generic functions.
375-418:to_vec_frame(0)afterdecode_pixel_data_frame(frame_index)is correct — but add a clarifying comment.The
PixelDecodertrait documentsdecode_pixel_data_frameas decoding "the pixel data of a single frame in this object." The returnedDecodedPixelDatacontains exactly one frame (the requested one), soto_vec_frame(0)correctly addresses it. This non-obvious invariant should be documented inline to prevent future readers from misreading it as a bug:♻️ Suggested explanatory comment
let decoded = obj .decode_pixel_data_frame(frame_index as u32) .context("Failed to decode PixelData frame for lazy load")?; + // decode_pixel_data_frame returns a single-frame DecodedPixelData for the + // requested frame, so to_vec_frame(0) correctly refers to that one frame. decoded .to_vec_frame(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dicom.rs` around lines 375 - 418, Add a short clarifying inline comment in both decode_mono_frame_from_path and decode_rgb_frame_from_path explaining that decode_pixel_data_frame(frame_index) returns a DecodedPixelData containing exactly the requested single frame, so calling to_vec_frame(0) is intentional and correct; reference the call sites decode_pixel_data_frame(frame_index as u32) and the subsequent to_vec_frame(0) invocations to make this invariant explicit for future readers and prevent mistaken refactors.
🤖 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`:
- Line 1511: CI is failing due to formatting; run rustfmt/cargo fmt and commit
the resulting changes so the call to self.rebuild_texture(ctx) (in App:: or impl
where rebuild_texture is defined in src/app.rs) matches the project's style.
Locate the invocation of self.rebuild_texture(ctx) and run cargo fmt (or
rustfmt) across the repo, then stage and push the formatted files to resolve the
CI formatting error.
In `@src/dicom.rs`:
- Around line 147-156: The background preload thread currently discards any
Result from preload_mono_frames_from_path (and preload_rgb_frames_from_path)
causing silent failures; change the thread closure in ensure_background_preload
to match on the Result (e.g., if let Err(e) =
preload_mono_frames_from_path(&path, &cache) {
log::error!("preload_mono_frames_from_path failed for {:?}: {:?}", path, e); })
so errors are logged (include the path and error), and apply the same pattern to
LazyRgbFrames::ensure_background_preload for preload_rgb_frames_from_path to
ensure failures are observable.
- Line 383: CI is failing due to Rust formatting violations; run `cargo fmt` (or
`rustfmt`) locally to reformat src/dicom.rs and commit the changes so the
pipeline passes; specifically reformat the expression/lines around
decoded.samples_per_pixel() (and nearby items in that module) to match rustfmt
rules, ensuring your repo has a rustfmt configuration if you need consistent
style.
---
Nitpick comments:
In `@src/app.rs`:
- Around line 45-53: PendingMammoLoad and PendingSingleLoad are identical;
replace them with a single unified type (e.g., PendingLoad) that contains path:
PathBuf and image: DicomImage, and remove the duplicate structs; update all
references/usages (channel types, send/recv sites, function signatures and
pattern matches) that mention PendingMammoLoad or PendingSingleLoad to use
PendingLoad instead so the code compiles and both channels share the same
payload type.
- Around line 1505-1519: Before moving `image` into `self.image`, clone it into
a local variable and use that clone when later building the history entry;
specifically, capture a cloned `image` (e.g. `let history_image =
image.clone()`) prior to `self.image = Some(image)`, then proceed with
`self.image = Some(image)` and use that `history_image` when constructing the
`HistorySingleData` passed to `push_single_history_entry` so you avoid calling
`self.image.clone()` after the move.
In `@src/dicom.rs`:
- Around line 69-95: The Mono vs RGB lazy-loading code is duplicated; replace
LazyMonoFrames/LazyRgbFrames and the paired functions
decode_mono_frame_from_path/decode_rgb_frame_from_path and
preload_mono_frames_from_path/preload_rgb_frames_from_path with a single generic
LazyFrames<T> type and generic helper functions (e.g., decode_frame_from_path<T>
and preload_frames_from_path<T>) parameterized on the pixel element type; keep
the same fields (path, cache, preload_started) but make cache hold Arc<[T]> and
use trait bounds required by your decoding logic (Send + Sync + 'static and any
conversion traits you currently use), update MonoFrames/RgbFrames enum variants
to use the generic LazyFrames<T> (or replace with a single enum parametrized by
T), and move any duplicated preload/decode logic into the generic
implementations so both mono (i32) and rgb (u8) code paths call the same generic
functions.
- Around line 375-418: Add a short clarifying inline comment in both
decode_mono_frame_from_path and decode_rgb_frame_from_path explaining that
decode_pixel_data_frame(frame_index) returns a DecodedPixelData containing
exactly the requested single frame, so calling to_vec_frame(0) is intentional
and correct; reference the call sites decode_pixel_data_frame(frame_index as
u32) and the subsequent to_vec_frame(0) invocations to make this invariant
explicit for future readers and prevent mistaken refactors.
timcogan
force-pushed
the
feat/faster-loading
branch
from
492156d to
54e57ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/app.rs (1)
1511-1511: Runcargo fmt— formatting at this line was previously flagged by CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` at line 1511, The CI flagged formatting at the call to self.rebuild_texture(ctx); in src/app.rs—run rustfmt (cargo fmt) to reformat the file so formatting matches project style, then re-stage the updated src/app.rs; ensure the call site (self.rebuild_texture(ctx) inside the relevant impl/block) remains semantically unchanged after formatting.src/dicom.rs (1)
150-165: Background preload errors are now logged — past concern resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dicom.rs` around lines 150 - 165, The ensure_background_preload function now spawns a thread that calls preload_mono_frames_from_path and logs any Err via eprintln!, which addresses the previous lack of error visibility; no code change required—verify that ensure_background_preload and the call to preload_mono_frames_from_path use the desired logging mechanism (consider replacing eprintln! with the project logger if needed) and keep the existing swap on preload_started to avoid duplicate spawns.
🧹 Nitpick comments (2)
src/dicom.rs (2)
409-444:decode_rgb_frame_from_pathlacks aSamplesPerPixelguard.
decode_mono_frame_from_pathearly-returns with an error whensamples_per_pixel != 1, but the RGB counterpart only validatesbits_allocated. Add a symmetric check at the top:♻️ Proposed fix
fn decode_rgb_frame_from_path(path: &Path, frame_index: usize) -> Result<Vec<u8>> { let obj = open_dicom_object(path)?; let decoded = obj .decode_pixel_data_frame(frame_index as u32) .context("Failed to decode PixelData frame for lazy load")?; + if decoded.samples_per_pixel() < 3 { + bail!( + "Lazy frame load expected color pixels, got SamplesPerPixel={}", + decoded.samples_per_pixel() + ); + } let bits_allocated = decoded.bits_allocated();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dicom.rs` around lines 409 - 444, The function decode_rgb_frame_from_path currently only checks bits_allocated but should also validate SamplesPerPixel like decode_mono_frame_from_path does; add a guard near the top of decode_rgb_frame_from_path that reads SamplesPerPixel from the opened DICOM object (use the same tag/lookup pattern used in decode_mono_frame_from_path) and early-return an error if samples_per_pixel != expected value for color images (e.g., != 3), including a clear error message referencing decode_rgb_frame_from_path and the actual samples_per_pixel value.
510-515: Early worker join error leaves sibling workers unjoined.When a worker returns
Err, the?operator exits the loop immediately; the remainingJoinHandles are dropped without being joined. The threads themselves will run to completion (theArc<Mutex>keeps the cache live), but any frames they decode are silently discarded, andpreload_startedis permanentlytrueso no retry occurs. Consider joining all workers before propagating any error, or at least logging the outcome of abandoned workers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dicom.rs` around lines 510 - 515, The loop that iterates over workers currently returns early on the first Err or Err result from result? which leaves remaining JoinHandle entries in workers unjoined and leaves preload_started set to true; change the logic in the block with workers and worker.join() so you first join all workers (call join() on each JoinHandle) and collect any errors (from join or from the returned Result) into a single error to propagate after the loop (or log individual failures) — for example, iterate through workers, record join failures and worker Result errors, ensure all handles are joined, and only then bail! or return Err if any error was observed; also ensure you log outcomes of abandoned workers instead of dropping them so preload_started can be retried if necessary.
🤖 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 1424-1432: The worker-disconnect branch currently leaves the UI
stuck because load_path pre-sets self.status_line to "Loading …" and the
disconnect handler only writes the error message when
self.status_line.is_empty(); remove that guard so the TryRecvError::Disconnected
arm always clears self.single_load_receiver and sets self.status_line to a clear
error like "Single-image load incomplete: worker exited before sending a
result." (update the match arm handling TryRecvError::Disconnected in the code
using self.single_load_receiver and self.status_line, ensuring
ctx.request_repaint() is still called).
In `@src/dicom.rs`:
- Around line 446-451: The code silences a poisoned mutex by using
cache.lock().ok() and treating Err as 0, which leaves preload_started true and
permanently disables preloading; update both preload_mono_frames_from_path and
preload_rgb_frames_from_path to handle cache.lock() failure explicitly instead
of mapping to 0 — either propagate the lock error (use ? on lock().map_err(...)
or expect with a descriptive message) or log the poison error and return early
(so preload_started is not set to true), and then proceed only when the lock is
successfully acquired; specifically replace the cache.lock().ok() usage with
explicit handling of the Result from cache.lock() and ensure that on Err you do
not mark preload as started and you return or log the error so the
decode-on-demand path can still retry.
- Around line 127-148: LazyMonoFrames::frame currently does a synchronous decode
by calling decode_mono_frame_from_path on a cache miss which blocks the UI
thread (called via render_image_frame → rebuild_texture /
advance_cine_if_needed); change frame so that on a cache miss it does NOT call
decode_mono_frame_from_path but simply triggers ensure_background_preload() (or
the existing preload kick) and returns None immediately, and then post a UI
repaint request so the UI will retry drawing when the background worker
populates the slot; apply the same change to the RGB counterpart and ensure
callers handle an Option::None by skipping the frame render instead of blocking.
---
Duplicate comments:
In `@src/app.rs`:
- Line 1511: The CI flagged formatting at the call to self.rebuild_texture(ctx);
in src/app.rs—run rustfmt (cargo fmt) to reformat the file so formatting matches
project style, then re-stage the updated src/app.rs; ensure the call site
(self.rebuild_texture(ctx) inside the relevant impl/block) remains semantically
unchanged after formatting.
In `@src/dicom.rs`:
- Around line 150-165: The ensure_background_preload function now spawns a
thread that calls preload_mono_frames_from_path and logs any Err via eprintln!,
which addresses the previous lack of error visibility; no code change
required—verify that ensure_background_preload and the call to
preload_mono_frames_from_path use the desired logging mechanism (consider
replacing eprintln! with the project logger if needed) and keep the existing
swap on preload_started to avoid duplicate spawns.
---
Nitpick comments:
In `@src/dicom.rs`:
- Around line 409-444: The function decode_rgb_frame_from_path currently only
checks bits_allocated but should also validate SamplesPerPixel like
decode_mono_frame_from_path does; add a guard near the top of
decode_rgb_frame_from_path that reads SamplesPerPixel from the opened DICOM
object (use the same tag/lookup pattern used in decode_mono_frame_from_path) and
early-return an error if samples_per_pixel != expected value for color images
(e.g., != 3), including a clear error message referencing
decode_rgb_frame_from_path and the actual samples_per_pixel value.
- Around line 510-515: The loop that iterates over workers currently returns
early on the first Err or Err result from result? which leaves remaining
JoinHandle entries in workers unjoined and leaves preload_started set to true;
change the logic in the block with workers and worker.join() so you first join
all workers (call join() on each JoinHandle) and collect any errors (from join
or from the returned Result) into a single error to propagate after the loop (or
log individual failures) — for example, iterate through workers, record join
failures and worker Result errors, ensure all handles are joined, and only then
bail! or return Err if any error was observed; also ensure you log outcomes of
abandoned workers instead of dropping them so preload_started can be retried if
necessary.
f1f9a61 to
f7cfb5a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/dicom.rs (1)
390-397:⚠️ Potential issue | 🟠 MajorDon’t silently ignore cache lock failures in preload workers.
If the cache mutex becomes poisoned during preloading, the current logic treats it as “already loaded,” which can leave frames missing with no error and no retry. Propagate the lock error so the worker returnsErrand the outerensure_background_preloadcan resetpreload_startedand log the failure.🛠️ Suggested fix (apply to both mono + RGB workers)
- let already_loaded = if let Ok(guard) = cache.lock() { - guard - .get(frame_index) - .and_then(|slot| slot.as_ref()) - .is_some() - } else { - true - }; + let already_loaded = { + let guard = cache + .lock() + .map_err(|err| anyhow::anyhow!("Background preload cache lock poisoned: {err}"))?; + guard + .get(frame_index) + .and_then(|slot| slot.as_ref()) + .is_some() + }; if already_loaded { continue; } @@ - if let Ok(mut guard) = cache.lock() { - if let Some(slot) = guard.get_mut(frame_index) { - if slot.is_none() { - *slot = Some(frame_pixels); - } - } - } + let mut guard = cache + .lock() + .map_err(|err| anyhow::anyhow!("Background preload cache lock poisoned: {err}"))?; + if let Some(slot) = guard.get_mut(frame_index) { + if slot.is_none() { + *slot = Some(frame_pixels); + } + }Also applies to: 424-430, 466-473, 517-523
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dicom.rs` around lines 390 - 397, The worker currently treats a poisoned cache mutex as "already loaded" by catching cache.lock() errors, which hides failures; change the logic in the preload worker(s) where already_loaded is computed (the cache.lock() checks in the mono and RGB preload blocks) to propagate the lock error instead of mapping it to true: return Err when cache.lock() fails so the worker bubbles up the error to ensure_background_preload, allowing ensure_background_preload to reset preload_started and log the failure; update the same pattern in the other occurrences flagged (the analogous cache.lock() checks around lines 424-430, 466-473, 517-523).
🤖 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/dicom.rs`:
- Around line 390-397: The worker currently treats a poisoned cache mutex as
"already loaded" by catching cache.lock() errors, which hides failures; change
the logic in the preload worker(s) where already_loaded is computed (the
cache.lock() checks in the mono and RGB preload blocks) to propagate the lock
error instead of mapping it to true: return Err when cache.lock() fails so the
worker bubbles up the error to ensure_background_preload, allowing
ensure_background_preload to reset preload_started and log the failure; update
the same pattern in the other occurrences flagged (the analogous cache.lock()
checks around lines 424-430, 466-473, 517-523).
timcogan
force-pushed
the
feat/faster-loading
branch
from
f7cfb5a to
ff47a2d
Compare
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
Improvements