Add webcam QR scanning and desktop share import/export#230
Open
wksantiago wants to merge 10 commits intomainfrom
Open
Add webcam QR scanning and desktop share import/export#230wksantiago wants to merge 10 commits intomainfrom
wksantiago wants to merge 10 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a desktop camera-backed QR scanner and UI: new scanner screen, CameraEvent flow, background camera thread (nokhwa) with rqrr QR detection and multi-frame assembly, app message routing/lifecycle for scanner start/stop/poll, import-screen button to open scanner, GTK init and Cargo dependency updates. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as Import Screen
participant App as App State
participant Scanner as ScannerScreen
participant CameraThread as Camera Thread
participant QRLib as rqrr
User->>UI: Click "Scan QR Code"
UI->>App: Message::ScannerOpen
App->>Scanner: create ScannerScreen
App->>CameraThread: start_camera(active, tx)
CameraThread->>App: CameraEvent::Ready
loop Poll (~33ms)
CameraThread->>CameraThread: capture frame, convert to RGBA
CameraThread->>QRLib: detect QR
QRLib-->>CameraThread: decoded? (opt)
CameraThread->>App: CameraEvent::Frame(rgba, w, h, decoded)
App->>Scanner: apply_scanner_event(frame)
Scanner->>UI: update preview/status
end
alt QR decoded (single or assembled)
Scanner->>App: emit decoded data
App->>App: transition to Import with bound data
App->>CameraThread: stop (active=false)
else Error / Retry
User->>Scanner: click Retry
Scanner->>CameraThread: restart capture
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
keep-desktop/src/app.rs (1)
1270-1293:⚠️ Potential issue | 🔴 Critical
do_lock()doesn't stop the scanner — camera thread leaks.If the auto-lock timer fires or the user manually locks while on the scanner screen,
do_lock()replacesself.screenwithout callingstop_scanner(). The background camera thread holds a clone ofcamera_activeand will continue running indefinitely because no one sets it tofalse. Thescanner_rxchannel also remains inself.scanner_rx(though it becomes effectively orphaned after the screen changes).🐛 Proposed fix
fn do_lock(&mut self) -> Task<Message> { self.handle_disconnect_relay(); self.stop_bunker(); + self.stop_scanner(); let mut guard = lock_keep(&self.keep);🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@keep-desktop/src/app.rs` around lines 1270 - 1293, In do_lock(), ensure the scanner background thread and its channel are shut down before replacing self.screen: call the existing stop_scanner() (which clears camera_active and drops scanner_rx) early in the function — e.g., right after stop_bunker() and before changing self.screen — so the camera_active flag is set to false and scanner_rx is taken/dropped to prevent the leaked camera thread and orphaned receiver.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@keep-desktop/src/app.rs`:
- Around line 1080-1089: When constructing the Import screen after a successful
QR decode (in the block that calls ImportScreen::new(),
ImportScreen::detect_mode(result.trim()), sets import.mode and import.data) also
compute and set import.npub_preview the same way the ImportDataChanged handler
does for nsec keys: detect the nsec mode, derive the corresponding npub preview
from the imported nsec value, and assign it to import.npub_preview so the
public-key preview is shown. Use the same helper/logic invoked in the
ImportDataChanged flow to produce the preview.
- Around line 1029-1034: stop_scanner currently only calls
Screen::Scanner.stop_camera() when self.screen is Scanner but always clears
scanner_rx, which can leave a running camera if the screen was changed first;
add a camera_active (or a dedicated camera_handle) field to App alongside
scanner_rx, set it true when starting the camera and false when stopped, and
change stop_scanner to check that field and unconditionally signal the camera
via the stored handle (or call stop_camera on the stored handle) before clearing
scanner_rx and setting camera_active = false; also update wherever the camera is
started/stopped to set the new field so stop_scanner no longer depends on
matching self.screen or on Screen::Scanner.
- Around line 1270-1293: In do_lock(), ensure the scanner background thread and
its channel are shut down before replacing self.screen: call the existing
stop_scanner() (which clears camera_active and drops scanner_rx) early in the
function — e.g., right after stop_bunker() and before changing self.screen — so
the camera_active flag is set to false and scanner_rx is taken/dropped to
prevent the leaked camera thread and orphaned receiver.
In `@keep-desktop/src/screen/scanner.rs`:
- Around line 56-89: The code currently overwrites self.total_expected when
parsing each frame (setting it unconditionally) which allows frames from
different animated sequences to be mixed; modify the frame-handling in the block
that parses f/t/d so that you first compare the parsed total (t) against
self.total_expected: if self.total_expected is None, set it to t as now; if
Some(existing) and existing != t then either reject the incoming frame (do
nothing) or clear self.collected_frames, set self.total_expected = Some(t) and
reset status to ScannerStatus::CollectingFrames { got: 0, total: t } before
inserting the new frame; ensure collected_frames.len() and the completion check
use the validated total and update ScannerStatus accordingly (affecting symbols:
self.total_expected, self.collected_frames, ScannerStatus::CollectingFrames).
- Around line 169-236: The camera thread sends full-frame CameraEvent::Frame
messages over an unbounded MPSC which can accumulate large images and OOM if the
UI falls behind; change the channel in open_scanner from unbounded_channel() to
a bounded channel (e.g., tokio::sync::mpsc::channel(2)) and update start_camera
to use try_send (or handle SendError) so frames are dropped when the buffer is
full, and adjust drain_scanner_events to use try_recv/try_recv loop semantics
for the bounded receiver; alternatively (or additionally) constrain resolution
in start_camera by changing the RequestedFormat::new call
(RequestedFormatType::AbsoluteHighestFrameRate) to a RequestedFormat that sets a
reasonable width/height to limit frame size.
- Around line 209-223: The code is doing a redundant DynamicImage round-trip:
rgb_image is converted to DynamicImage then to_luma8() for QR decoding and later
into_rgb8() to recreate RGB bytes for RGBA; instead, avoid creating DynamicImage
twice by deriving the grayscale buffer directly from the original rgb_image
bytes and by building the rgba Vec<u8> directly from rgb_image pixel data, then
call rqrr::PreparedImage::prepare on the produced grayscale image (keep using
PreparedImage::prepare, detect_grids and decode as before) so you remove the
into_rgb8() call and the extra DynamicImage conversions.
- Around line 134-150: The view is cloning the entire RGBA buffer on every
render (self.frame_rgba -> rgba.clone()) when calling
iced_image::Handle::from_rgba; change the design to avoid heavy clones by either
storing the pixel buffer as an Arc<Vec<u8>> (make self.frame_rgba: Arc<Vec<u8>>
and pass Arc::clone into Handle::from_rgba) or cache the created
iced_image::Handle on the struct (e.g., self.frame_handle) and only recreate the
Handle when the frame data or dimensions change (update in the code path that
mutates frame_rgba/frame_width/frame_height), then use the cached Handle in
view_content instead of rebuilding from_rgba each render.
🧹 Nitpick comments (4)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@keep-desktop/src/app.rs`: - Around line 1029-1034: stop_scanner currently only calls Screen::Scanner.stop_camera() when self.screen is Scanner but always clears scanner_rx, which can leave a running camera if the screen was changed first; add a camera_active (or a dedicated camera_handle) field to App alongside scanner_rx, set it true when starting the camera and false when stopped, and change stop_scanner to check that field and unconditionally signal the camera via the stored handle (or call stop_camera on the stored handle) before clearing scanner_rx and setting camera_active = false; also update wherever the camera is started/stopped to set the new field so stop_scanner no longer depends on matching self.screen or on Screen::Scanner. In `@keep-desktop/src/screen/scanner.rs`: - Around line 169-236: The camera thread sends full-frame CameraEvent::Frame messages over an unbounded MPSC which can accumulate large images and OOM if the UI falls behind; change the channel in open_scanner from unbounded_channel() to a bounded channel (e.g., tokio::sync::mpsc::channel(2)) and update start_camera to use try_send (or handle SendError) so frames are dropped when the buffer is full, and adjust drain_scanner_events to use try_recv/try_recv loop semantics for the bounded receiver; alternatively (or additionally) constrain resolution in start_camera by changing the RequestedFormat::new call (RequestedFormatType::AbsoluteHighestFrameRate) to a RequestedFormat that sets a reasonable width/height to limit frame size. - Around line 209-223: The code is doing a redundant DynamicImage round-trip: rgb_image is converted to DynamicImage then to_luma8() for QR decoding and later into_rgb8() to recreate RGB bytes for RGBA; instead, avoid creating DynamicImage twice by deriving the grayscale buffer directly from the original rgb_image bytes and by building the rgba Vec<u8> directly from rgb_image pixel data, then call rqrr::PreparedImage::prepare on the produced grayscale image (keep using PreparedImage::prepare, detect_grids and decode as before) so you remove the into_rgb8() call and the extra DynamicImage conversions. - Around line 134-150: The view is cloning the entire RGBA buffer on every render (self.frame_rgba -> rgba.clone()) when calling iced_image::Handle::from_rgba; change the design to avoid heavy clones by either storing the pixel buffer as an Arc<Vec<u8>> (make self.frame_rgba: Arc<Vec<u8>> and pass Arc::clone into Handle::from_rgba) or cache the created iced_image::Handle on the struct (e.g., self.frame_handle) and only recreate the Handle when the frame data or dimensions change (update in the code path that mutates frame_rgba/frame_width/frame_height), then use the cached Handle in view_content instead of rebuilding from_rgba each render.keep-desktop/src/screen/scanner.rs (3)
169-236: Unbounded channel can accumulate large frames without backpressure.Each
CameraEvent::Framecontains a full RGBA buffer (e.g., ~2.7 MB at 1920×360, potentially much larger at high resolutions sinceAbsoluteHighestFrameRateis requested without constraining resolution). The unbounded channel has no backpressure, so if the UI polling falls behind, frames queue up in memory. Whiledrain_scanner_eventskeeps only the last frame, the channel itself grows unboundedly between polls.Consider either:
- Using a bounded channel (capacity 2–3) with
try_sendto drop frames when full.- Or constraining the requested camera resolution in addition to frame rate (line 179).
♻️ Use a bounded channel to apply backpressure
-pub fn start_camera( - active: Arc<AtomicBool>, - tx: tokio::sync::mpsc::UnboundedSender<CameraEvent>, -) { +pub fn start_camera( + active: Arc<AtomicBool>, + tx: tokio::sync::mpsc::Sender<CameraEvent>, +) { std::thread::spawn(move || { // ... - let _ = tx.send(CameraEvent::Frame { + let _ = tx.try_send(CameraEvent::Frame { rgba, width: w, height: h, decoded, }); // ... }); }This requires changing the channel creation in
open_scanner(inapp.rs) fromunbounded_channel()tochannel(2)and updatingdrain_scanner_eventsto usetry_recv()on the bounded receiver.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@keep-desktop/src/screen/scanner.rs` around lines 169 - 236, The camera thread sends full-frame CameraEvent::Frame messages over an unbounded MPSC which can accumulate large images and OOM if the UI falls behind; change the channel in open_scanner from unbounded_channel() to a bounded channel (e.g., tokio::sync::mpsc::channel(2)) and update start_camera to use try_send (or handle SendError) so frames are dropped when the buffer is full, and adjust drain_scanner_events to use try_recv/try_recv loop semantics for the bounded receiver; alternatively (or additionally) constrain resolution in start_camera by changing the RequestedFormat::new call (RequestedFormatType::AbsoluteHighestFrameRate) to a RequestedFormat that sets a reasonable width/height to limit frame size.
209-223: Redundant image conversion in the hot path.The frame is converted:
rgb_image→DynamicImage→to_luma8()(for QR) →into_rgb8()→ manual RGBA. Theinto_rgb8()call on line 220 re-derives the RGB data that was already available in the originalrgb_image. Since this runs ~30 times/sec, consider converting the original RGB buffer to RGBA directly and creating the grayscale image separately.♻️ Avoid redundant RGB round-trip
let (w, h) = (rgb_image.width(), rgb_image.height()); - let dynamic = ::image::DynamicImage::from(rgb_image); - let gray = dynamic.to_luma8(); + let gray = ::image::DynamicImage::from(rgb_image.clone()).to_luma8(); let mut prepared = rqrr::PreparedImage::prepare(gray); let grids = prepared.detect_grids(); let decoded = grids .into_iter() .find_map(|g| g.decode().ok().map(|(_, content)| content)); - let rgba: Vec<u8> = dynamic - .into_rgb8() - .pixels() + let rgba: Vec<u8> = rgb_image + .pixels() .flat_map(|p| [p[0], p[1], p[2], 255]) .collect();🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@keep-desktop/src/screen/scanner.rs` around lines 209 - 223, The code is doing a redundant DynamicImage round-trip: rgb_image is converted to DynamicImage then to_luma8() for QR decoding and later into_rgb8() to recreate RGB bytes for RGBA; instead, avoid creating DynamicImage twice by deriving the grayscale buffer directly from the original rgb_image bytes and by building the rgba Vec<u8> directly from rgb_image pixel data, then call rqrr::PreparedImage::prepare on the produced grayscale image (keep using PreparedImage::prepare, detect_grids and decode as before) so you remove the into_rgb8() call and the extra DynamicImage conversions.
134-150: Full RGBA buffer cloned on every render.Line 139 clones the entire frame buffer (potentially megabytes) every time
view_contentis called. SinceHandle::from_rgbatakes ownership, this clone is necessary with the current design. Consider storing the frame as anArc<Vec<u8>>or caching theHandleto avoid repeated large allocations.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@keep-desktop/src/screen/scanner.rs` around lines 134 - 150, The view is cloning the entire RGBA buffer on every render (self.frame_rgba -> rgba.clone()) when calling iced_image::Handle::from_rgba; change the design to avoid heavy clones by either storing the pixel buffer as an Arc<Vec<u8>> (make self.frame_rgba: Arc<Vec<u8>> and pass Arc::clone into Handle::from_rgba) or cache the created iced_image::Handle on the struct (e.g., self.frame_handle) and only recreate the Handle when the frame data or dimensions change (update in the code path that mutates frame_rgba/frame_width/frame_height), then use the cached Handle in view_content instead of rebuilding from_rgba each render.keep-desktop/src/app.rs (1)
1029-1034:stop_scanneronly signals the camera ifScreen::Scanneris the current screen.The method checks
self.screenbefore callingstop_camera(), but always clearsscanner_rx. This is currently safe because all callers invokestop_scanner()while the screen is stillScanner(or when it's already a different screen and no camera is running). However, this coupling is fragile — if a future caller changes the screen before callingstop_scanner(), the camera thread won't be signaled.Consider storing the
camera_activeflag inAppdirectly (or alongsidescanner_rx) rather than reaching into the screen variant, sostop_scannerworks regardless of current screen state.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@keep-desktop/src/app.rs` around lines 1029 - 1034, stop_scanner currently only calls Screen::Scanner.stop_camera() when self.screen is Scanner but always clears scanner_rx, which can leave a running camera if the screen was changed first; add a camera_active (or a dedicated camera_handle) field to App alongside scanner_rx, set it true when starting the camera and false when stopped, and change stop_scanner to check that field and unconditionally signal the camera via the stored handle (or call stop_camera on the stored handle) before clearing scanner_rx and setting camera_active = false; also update wherever the camera is started/stopped to set the new field so stop_scanner no longer depends on matching self.screen or on Screen::Scanner.
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)
keep-desktop/src/app.rs (1)
1278-1301:⚠️ Potential issue | 🟠 MajorCamera thread leaks when vault locks while scanner is active.
do_lockreplaces the screen and clears most state, but never callsstop_scanner(). If the scanner is open when auto-lock fires (or the user manually locks), the camera thread keeps running indefinitely — theAtomicBoolis never set tofalse,scanner_rxis never cleared, and the unbounded channel accumulates frames with no consumer.The same issue applies to
handle_window_close(line 2408) andhandle_tray_quit(line 2443).🐛 Proposed fix
fn do_lock(&mut self) -> Task<Message> { self.handle_disconnect_relay(); self.stop_bunker(); + self.stop_scanner(); let mut guard = lock_keep(&self.keep);And similarly for the exit paths:
fn handle_window_close(&mut self, id: iced::window::Id) -> Task<Message> { if self.settings.minimize_to_tray && self.has_tray { self.window_visible = false; iced::window::set_mode(id, iced::window::Mode::Hidden) } else { self.handle_disconnect_relay(); self.stop_bunker(); + self.stop_scanner(); iced::exit() } }fn handle_tray_quit(&mut self) -> Task<Message> { self.handle_disconnect_relay(); self.stop_bunker(); + self.stop_scanner(); iced::exit() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/app.rs` around lines 1278 - 1301, The do_lock function (and the other exit/lock paths handle_window_close and handle_tray_quit) fails to stop the camera scanner, leaking the scanner thread and its channel; before clearing state and swapping screens in do_lock (and likewise in handle_window_close/handle_tray_quit) call the existing stop_scanner() helper to set the AtomicBool false, drain/clear scanner_rx and ensure the scanner consumer is dropped, then proceed with the rest of the state resets (clearing identities, toast, screen swap, etc.) so the camera thread and unbounded channel are properly terminated.
🧹 Nitpick comments (1)
keep-desktop/src/screen/scanner.rs (1)
142-158: RGBA frame buffer cloned on every render (~1 MB/frame at 30 fps).
rgba.clone()allocates and copies the full camera frame every timeview_contentis called. For a 640×480 camera, that's ~1.2 MB per clone at ~30 fps. Consider caching theiced_image::Handlein the struct (updated only when a new frame arrives inapply_scanner_event) so the view can reuse it without re-cloning the raw buffer each render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/screen/scanner.rs` around lines 142 - 158, The view currently calls iced_image::Handle::from_rgba(self.frame_width, self.frame_height, rgba.clone()) inside view_content (using self.frame_rgba, frame_width, frame_height), causing a full buffer clone every render; instead add a cached field (e.g., frame_handle: Option<iced_image::Handle>) to the scanner struct, create/update that Handle only when a new frame arrives in apply_scanner_event (where you already receive/replace self.frame_rgba), and have view_content use the cached self.frame_handle (and skip calling from_rgba/clone there) so rendering reuses the Handle and avoids per-frame buffer allocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keep-desktop/src/screen/scanner.rs`:
- Around line 62-97: The code currently inserts frames into collected_frames
without checking that the parsed index (idx) is within the expected range
(total), which allows out‑of‑range frames to grow the map; before inserting at
collected_frames.insert(idx, ...) validate that idx < total and skip (or return
None) if not—i.e., add a guard right after computing idx and total to ignore
frames where idx >= total, keeping the existing total_expected and ScannerStatus
handling otherwise so only valid indices are stored and assembly via
ShareExport::from_animated_frames sees a complete, bounded sequence.
---
Outside diff comments:
In `@keep-desktop/src/app.rs`:
- Around line 1278-1301: The do_lock function (and the other exit/lock paths
handle_window_close and handle_tray_quit) fails to stop the camera scanner,
leaking the scanner thread and its channel; before clearing state and swapping
screens in do_lock (and likewise in handle_window_close/handle_tray_quit) call
the existing stop_scanner() helper to set the AtomicBool false, drain/clear
scanner_rx and ensure the scanner consumer is dropped, then proceed with the
rest of the state resets (clearing identities, toast, screen swap, etc.) so the
camera thread and unbounded channel are properly terminated.
---
Duplicate comments:
In `@keep-desktop/src/screen/scanner.rs`:
- Around line 64-72: The code previously allowed mixed animated sequences;
update ensures when a new frame arrives with a different total the scanner
resets collected_frames, updates total_expected, and sets status to
ScannerStatus::CollectingFrames {{got: 0, total}}; keep this change and add a
unit test covering the Scanner struct: simulate receiving a frame with one total
then a frame with a different total and assert collected_frames is cleared,
total_expected equals the new total, and status is CollectingFrames with got ==
0 to prevent mixing sequences (refer to total_expected, collected_frames, and
ScannerStatus::CollectingFrames in your test).
---
Nitpick comments:
In `@keep-desktop/src/screen/scanner.rs`:
- Around line 142-158: The view currently calls
iced_image::Handle::from_rgba(self.frame_width, self.frame_height, rgba.clone())
inside view_content (using self.frame_rgba, frame_width, frame_height), causing
a full buffer clone every render; instead add a cached field (e.g.,
frame_handle: Option<iced_image::Handle>) to the scanner struct, create/update
that Handle only when a new frame arrives in apply_scanner_event (where you
already receive/replace self.frame_rgba), and have view_content use the cached
self.frame_handle (and skip calling from_rgba/clone there) so rendering reuses
the Handle and avoids per-frame buffer allocations.
wksantiago
changed the title
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)
keep-desktop/src/app.rs (1)
1276-1299:⚠️ Potential issue | 🟠 Major
do_lock()does not stop the scanner — camera thread and hardware leak.If auto-lock fires (or the user manually locks) while on the Scanner screen, the screen is replaced with
Unlockbutstop_scanner()is never called. The camera thread holds its ownArc<AtomicBool>clone and will keep running indefinitely (and holding the camera device open), sincecamera_activeis never set tofalse.🐛 Proposed fix
fn do_lock(&mut self) -> Task<Message> { + self.stop_scanner(); self.handle_disconnect_relay(); self.stop_bunker();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/app.rs` around lines 1276 - 1299, do_lock() currently replaces the UI with Unlock but never stops the scanner, leaking the camera thread; update do_lock to ensure the scanner is stopped and camera activity flag cleared: call the existing stop_scanner() method (or otherwise set camera_active = false / clear the Arc<AtomicBool> used by the camera thread) before switching screens, ensuring the camera thread is signalled to exit when the current screen is Scanner (match or check for Screen::Scanner) or unconditionally if safe; reference do_lock, stop_scanner, camera_active, and Screen::Scanner to locate where to add this call.
🧹 Nitpick comments (4)
keep-desktop/src/screen/scanner.rs (3)
143-158:rgba.clone()on everyview()call copies the full frame buffer (~1 MB+ at 640×480).
iced_image::Handle::from_rgbatakes ownership, so a clone is necessary here, butview()can be called multiple times per frame by the iced runtime. Consider caching theHandlein the struct (updating it only when a new frame arrives inapply_scanner_event) instead of rebuilding it from the raw buffer on every render.♻️ Sketch: cache the Handle
Store a pre-built handle in
ScannerScreen:pub struct ScannerScreen { - pub frame_rgba: Option<Vec<u8>>, - pub frame_width: u32, - pub frame_height: u32, + pub frame_handle: Option<iced::widget::image::Handle>, ... }Update it once when a new frame arrives (in
app.rs::apply_scanner_event):- s.frame_rgba = Some(rgba); - s.frame_width = width; - s.frame_height = height; + s.frame_handle = Some( + iced_image::Handle::from_rgba(width, height, rgba), + );Then in
view_content, use it directly without cloning:- if let Some(rgba) = &self.frame_rgba { - if self.frame_width > 0 && self.frame_height > 0 { - let handle = iced_image::Handle::from_rgba( - self.frame_width, - self.frame_height, - rgba.clone(), - ); - let img = iced_image::Image::new(handle) + if let Some(handle) = &self.frame_handle { + let img = iced_image::Image::new(handle.clone())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/screen/scanner.rs` around lines 143 - 158, The view currently clones the full frame buffer each render by calling iced_image::Handle::from_rgba with self.frame_rgba, which is expensive; to fix, add a cached Option<iced_image::Handle> field on ScannerScreen (e.g. frame_handle) and update that handle only when a new frame arrives inside apply_scanner_event (build the Handle from the owned buffer there and clear or replace frame_rgba if needed), then change view/view_content to use the cached frame_handle (and avoid rgba.clone()/recreating the Handle) while still checking frame_width/frame_height before constructing the Image.
178-242: Camera thread has no mechanism to report when it has fully stopped.
stop_camera()sets the atomic flag, but the caller (e.g.,stop_scanner) dropsscanner_rximmediately. If the channel is dropped while the thread is mid-send,tx.send()returnsErrwhich is already ignored (let _ =), so there's no functional bug. However, the camera hardware resource (camera.stop_stream()on line 241) is released asynchronously — if the user quickly retries (ScannerRetry), a newCamera::newcall may race with the old thread still holding the device.This is an edge case but worth noting for camera-exclusive platforms. A
JoinHandleor a one-shot "stopped" signal would allowopen_scannerto wait for the old thread to finish before opening a new camera.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/screen/scanner.rs` around lines 178 - 242, The camera thread started by start_camera lacks a way to signal when it has fully stopped, causing a race where a new Camera::new can contend with the old thread still holding the device; fix by changing start_camera to provide a deterministic "stopped" signal (e.g., return a JoinHandle or accept a oneshot::Sender/Receiver pair) and ensure stop_camera (or caller) awaits that signal before attempting to reopen the camera; specifically, modify start_camera to return a handle or send a message on a provided stopped_sender when the thread completes (after camera.stop_stream()), and update the caller (stop_camera/stop_scanner) to await the stopped_receiver before proceeding to reopen the scanner.
215-229: Considerinto_rgba8()instead of manual RGB→RGBA expansion.The
imagecrate'sDynamicImage::into_rgba8()does the same conversion but avoids the intermediateinto_rgb8()step and the manualflat_mapallocation.♻️ Proposed simplification
- let dynamic = ::image::DynamicImage::from(rgb_image); - let gray = dynamic.to_luma8(); + let dynamic = ::image::DynamicImage::from(rgb_image); + let gray = dynamic.to_luma8(); let mut prepared = rqrr::PreparedImage::prepare(gray); let grids = prepared.detect_grids(); let decoded = grids .into_iter() .find_map(|g| g.decode().ok().map(|(_, content)| content)); - let rgba: Vec<u8> = dynamic - .into_rgb8() - .pixels() - .flat_map(|p| [p[0], p[1], p[2], 255]) - .collect(); + let rgba: Vec<u8> = dynamic.into_rgba8().into_raw();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/screen/scanner.rs` around lines 215 - 229, Replace the manual RGB→RGBA expansion that constructs the rgba Vec<u8> from dynamic.into_rgb8().pixels().flat_map(...) with the image crate's built-in conversion: call DynamicImage::into_rgba8() on the dynamic image and obtain the u8 buffer from that (e.g. via into_vec()/to_vec()), assigning it to the same rgba variable; this removes the intermediate into_rgb8() allocation and the manual flat_map while preserving alpha=255.keep-desktop/src/app.rs (1)
1029-1034:stop_scanneronly signals the camera if the current screen isScanner.After a successful QR decode,
apply_scanner_eventtransitions the screen toImport(line 1096) and setsscanner_rx = None(line 1083), but the screen is no longerScanner. Ifstop_scanner()is called later (e.g., viaGoBackfrom the Import screen, line 759), theif let Screen::Scanner(s)guard won't match andstop_camera()won't be called. In this specific flowapply_scanner_eventalready callss.stop_camera()on line 1082, so it's fine.However, if
stop_scanneris called in any other context where the screen has already been swapped (e.g., thedo_lockfix above), the camera won't be stopped. Consider also storing thecamera_activeArc directly onAppfor a more robust stop mechanism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/app.rs` around lines 1029 - 1034, stop_scanner only calls s.stop_camera when self.screen is Screen::Scanner, so if the screen was swapped earlier the camera can remain running; to fix, add a persistent camera handle on App (e.g., camera_active Arc or a ScannerHandle field) and use it in stop_scanner instead of probing self.screen: store the Arc/handle when starting the scanner (where scanner_rx is created), update apply_scanner_event to set that field to None/stop the camera when finishing, and change stop_scanner to unconditionally stop the camera via that stored handle (or clear scanner_rx and call stop on the contained scanner if you choose a ScannerHandle wrapper). Ensure you update start/stop sites (start scanner, apply_scanner_event, do_lock/GoBack flows) to set/clear the new camera handle so s.stop_camera is always invoked regardless of current Screen.
🤖 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 `@keep-desktop/src/app.rs`:
- Around line 1276-1299: do_lock() currently replaces the UI with Unlock but
never stops the scanner, leaking the camera thread; update do_lock to ensure the
scanner is stopped and camera activity flag cleared: call the existing
stop_scanner() method (or otherwise set camera_active = false / clear the
Arc<AtomicBool> used by the camera thread) before switching screens, ensuring
the camera thread is signalled to exit when the current screen is Scanner (match
or check for Screen::Scanner) or unconditionally if safe; reference do_lock,
stop_scanner, camera_active, and Screen::Scanner to locate where to add this
call.
---
Duplicate comments:
In `@keep-desktop/src/app.rs`:
- Around line 1080-1097: This change correctly computes npub_preview for nsec QR
imports to match the ImportDataChanged handler; no functional change required —
ensure the code uses the trimmed QR result passed to ImportScreen::detect_mode
and keep_core::keys::NostrKeypair::from_nsec(trimmed).ok().map(|kp|
kp.to_npub()) to set import.npub_preview when mode == ImportMode::Nsec, leaves
it None otherwise, stops the camera via s.stop_camera(), clears scanner_rx,
assigns import.data = Zeroizing::new(result) and sets self.screen =
Screen::Import(import) so behavior mirrors the ImportDataChanged flow.
In `@keep-desktop/src/screen/scanner.rs`:
- Around line 49-107: The review contains a duplicate approval comment; remove
the redundant "[duplicate_comment]"/duplicate approval so there's only a single
approval note for clarity. No code changes needed in process_qr_content, but
keep the existing guards (idx >= total) and total_expected logic intact in the
function process_qr_content and related ScannerStatus handling while removing
the duplicate review annotation.
---
Nitpick comments:
In `@keep-desktop/src/app.rs`:
- Around line 1029-1034: stop_scanner only calls s.stop_camera when self.screen
is Screen::Scanner, so if the screen was swapped earlier the camera can remain
running; to fix, add a persistent camera handle on App (e.g., camera_active Arc
or a ScannerHandle field) and use it in stop_scanner instead of probing
self.screen: store the Arc/handle when starting the scanner (where scanner_rx is
created), update apply_scanner_event to set that field to None/stop the camera
when finishing, and change stop_scanner to unconditionally stop the camera via
that stored handle (or clear scanner_rx and call stop on the contained scanner
if you choose a ScannerHandle wrapper). Ensure you update start/stop sites
(start scanner, apply_scanner_event, do_lock/GoBack flows) to set/clear the new
camera handle so s.stop_camera is always invoked regardless of current Screen.
In `@keep-desktop/src/screen/scanner.rs`:
- Around line 143-158: The view currently clones the full frame buffer each
render by calling iced_image::Handle::from_rgba with self.frame_rgba, which is
expensive; to fix, add a cached Option<iced_image::Handle> field on
ScannerScreen (e.g. frame_handle) and update that handle only when a new frame
arrives inside apply_scanner_event (build the Handle from the owned buffer there
and clear or replace frame_rgba if needed), then change view/view_content to use
the cached frame_handle (and avoid rgba.clone()/recreating the Handle) while
still checking frame_width/frame_height before constructing the Image.
- Around line 178-242: The camera thread started by start_camera lacks a way to
signal when it has fully stopped, causing a race where a new Camera::new can
contend with the old thread still holding the device; fix by changing
start_camera to provide a deterministic "stopped" signal (e.g., return a
JoinHandle or accept a oneshot::Sender/Receiver pair) and ensure stop_camera (or
caller) awaits that signal before attempting to reopen the camera; specifically,
modify start_camera to return a handle or send a message on a provided
stopped_sender when the thread completes (after camera.stop_stream()), and
update the caller (stop_camera/stop_scanner) to await the stopped_receiver
before proceeding to reopen the scanner.
- Around line 215-229: Replace the manual RGB→RGBA expansion that constructs the
rgba Vec<u8> from dynamic.into_rgb8().pixels().flat_map(...) with the image
crate's built-in conversion: call DynamicImage::into_rgba8() on the dynamic
image and obtain the u8 buffer from that (e.g. via into_vec()/to_vec()),
assigning it to the same rgba variable; this removes the intermediate
into_rgb8() allocation and the manual flat_map while preserving alpha=255.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deny.toml (1)
8-8: Consider adding atimeoutto this ignore entry.cargo-deny supports an optional
timeoutdate onadvisories.ignoreentries. Without one, this suppression persists indefinitely. Adding a timeout (e.g., one year out) ensures the team periodically re-evaluates whether thetray-iconecosystem has gained a gtk4-backed path.♻️ Suggested addition
- { id = "RUSTSEC-2024-0415", reason = "gtk3 bindings unmaintained; no migration path for tray-icon ecosystem" }, + { id = "RUSTSEC-2024-0415", reason = "gtk3 bindings unmaintained; no migration path for tray-icon ecosystem", timeout = "2027-02-01" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deny.toml` at line 8, The ignore entry for advisory id "RUSTSEC-2024-0415" lacks a timeout and will suppress the advisory indefinitely; update the advisories.ignore entry for id "RUSTSEC-2024-0415" to include a timeout field set to a sensible re-evaluation date (e.g., one year from now) so the suppression expires automatically and the advisory is rechecked later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deny.toml`:
- Line 31: The project currently lists "IJG" in deny.toml but does not include
the required IJG attribution in distributed documentation; update the app's
about screen and third-party credits file (e.g.,
CreditsDialog/AboutWindow/README) to include the exact acknowledgement: "this
software is based in part on the work of the Independent JPEG Group", and ensure
you do NOT use any IJG author or company names in advertising or publicity; tie
this change to the existing "IJG" entry so reviewers can verify the
acknowledgement is present before release.
---
Nitpick comments:
In `@deny.toml`:
- Line 8: The ignore entry for advisory id "RUSTSEC-2024-0415" lacks a timeout
and will suppress the advisory indefinitely; update the advisories.ignore entry
for id "RUSTSEC-2024-0415" to include a timeout field set to a sensible
re-evaluation date (e.g., one year from now) so the suppression expires
automatically and the advisory is rechecked later.
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
Tested Features (Desktop App, 2-PC local verification)
Passing
Known Behavior
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Chores