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 system tray module, tray-driven controls and desktop notifications, new tray-related settings (minimize_to_tray, start_minimized), startup/window-close behavior changes, CI/Docker apt installs for GTK/XDo, and new desktop dependencies ( Changes
Sequence DiagramsequenceDiagram
participant User
participant Tray as "System Tray"
participant App
participant Bunker as "BunkerService"
participant Notify as "Notification System"
User->>Tray: click menu (Show / Toggle Bunker / Quit)
Tray->>App: TrayEvent (ShowWindow / ToggleBunker / Quit)
App->>App: handle_tray_event (show/hide/toggle/quit)
App->>Bunker: request start/stop/toggle
Bunker->>App: emit events (Connected / Approval / Log)
App->>Notify: send_sign_request_notification / send_bunker_approval_notification
Notify->>User: display notification
alt Window close & minimize_to_tray enabled and tray present
User->>App: WindowCloseRequested
App->>App: hide window (keep running)
else
User->>App: WindowCloseRequested
App->>App: exit application
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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
🤖 Fix all issues with AI agents
In `@keep-desktop/src/app.rs`:
- Around line 234-237: The new serde default for minimize_to_tray (serde(default
= "default_true")) will enable minimize-to-tray for existing users; change this
to opt-in by switching the serde default to false (e.g., use a default_false
helper or serde(default) with a false-returning function) for the
minimize_to_tray field and keep start_minimized as-is, and add a one-time
migration/notification path in the settings load code (where you deserialize
settings) to either preserve prior behavior or show a notice explaining the new
minimize-to-tray option so existing users are not surprised; update references
to minimize_to_tray and default_true accordingly.
- Around line 1514-1526: The early return inside
Message::SettingsMinimizeToTrayToggled currently updates
self.settings.minimize_to_tray and returns before the code that syncs the
Settings screen fields, causing the UI to show stale state when re-displaying
the window; to fix, perform the same screen-field synchronization (the code that
updates the Settings screen's fields) immediately after setting
self.settings.minimize_to_tray and calling save_settings(&self.keep_path,
&self.settings) and only then run the window-visibility logic that sets
self.window_visible, calls iced::window::oldest() and Task::batch with
iced::window::set_mode and iced::window::gain_focus; ensure the settings-screen
sync runs in both the early-return branch and the normal path so the UI always
reflects the new minimize_to_tray value.
🧹 Nitpick comments (5)
keep-desktop/src/app.rs (3)
1612-1622:notify_sign_requestignores the_reqparameter — is this intentional?The
_req: &PendingSignRequestparameter is unused. If the intent is to include request details in the notification later, consider adding aTODOcomment. If it's intentional to keep notifications generic, the parameter could be removed to avoid confusion.
1672-1686: Test name is misleading — it actually verifies that close does not hide when tray is absent.
window_close_with_minimize_to_tray_hides_windowuseshas_tray: false, sohandle_window_closetakes the exit path. The assertion confirmswindow_visiblestaystrue. Consider renaming to something likewindow_close_minimize_to_tray_without_tray_exits.
506-517: Tray events are not processed whenhandle_tickreturns early.Multiple early returns (auto-lock at line 482, clipboard clear at 487, reconnect at 502) skip tray event polling. This means tray actions (Show, Quit, etc.) may be delayed by up to one tick (1 second). This is likely acceptable for UX, but worth being aware of — a queued
Quitevent won't be processed until after a reconnect completes, for example.keep-desktop/src/tray.rs (2)
88-106: Background thread for menu event dispatch relies on globalMenuEvent::receiver().
MenuEvent::receiver()returns a global static&Receiver. IfTrayState::new()were called more than once (e.g., in tests or after a retry), multiple threads would race on the same receiver. This is safe in production sinceApp::initcreates exactly oneTrayState, but it would be a footgun if tray re-initialization is ever added.Consider adding a doc comment on
TrayState::newnoting the singleton constraint.
210-274: Icon pixel tests duplicatebuild_iconlogic instead of calling it.These tests reimplement the icon buffer generation rather than calling
build_icon()and inspecting the result. Ifbuild_iconis refactored (e.g., different radius or color), these tests will still pass with stale values, giving false confidence.Consider extracting the raw RGBA buffer generation into a helper (e.g.,
build_icon_rgba) that bothbuild_iconand the tests can use:Proposed refactor
+fn build_icon_rgba(connected: bool) -> Vec<u8> { + let mut rgba = vec![0u8; (ICON_SIZE * ICON_SIZE * 4) as usize]; + let color: [u8; 4] = if connected { + [77, 166, 115, 255] + } else { + [140, 140, 148, 255] + }; + let center = ICON_SIZE as f32 / 2.0; + let r = center - 2.0; + let r_sq = r * r; + for y in 0..ICON_SIZE { + for x in 0..ICON_SIZE { + let dx = x as f32 - center; + let dy = y as f32 - center; + if dx * dx + dy * dy <= r_sq { + let idx = ((y * ICON_SIZE + x) * 4) as usize; + rgba[idx..idx + 4].copy_from_slice(&color); + } + } + } + rgba +} + fn build_icon(connected: bool) -> Result<Icon, String> { - let mut rgba = vec![0u8; (ICON_SIZE * ICON_SIZE * 4) as usize]; - ... - Icon::from_rgba(rgba, ICON_SIZE, ICON_SIZE).map_err(|e| format!("icon build failed: {e}")) + let rgba = build_icon_rgba(connected); + Icon::from_rgba(rgba, ICON_SIZE, ICON_SIZE).map_err(|e| format!("icon build failed: {e}")) }Then tests can call
build_icon_rgba(true)directly and assert on the returned buffer.
wksantiago
force-pushed
the
System-tray
branch
from
5aab7a8 to
95b165c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
keep-desktop/src/tray.rs (3)
198-208: Test names suggest color verification but only check construction.
build_icon_connected_uses_greenandbuild_icon_disconnected_uses_grayimply the tests verify the icon color, but they only assert thatbuild_iconsucceeds. Consider either renaming (e.g.,build_icon_connected_succeeds) or adding an assertion that inspects the RGBA data.
223-229: Test uses byte length butsanitize_notification_fieldcounts chars.Line 227 uses
result.len()(byte length) which happens to matchMAX_NOTIFICATION_FIELD_LEN + 3for ASCII input. For consistency with the function's char-based logic, preferresult.chars().count()— as you already do in the Unicode test on line 260.Proposed fix
- assert_eq!(result.len(), MAX_NOTIFICATION_FIELD_LEN + 3); + assert_eq!(result.chars().count(), MAX_NOTIFICATION_FIELD_LEN + 3);
88-106: Spawned thread has no name and no logging on shutdown.Consider naming the thread (e.g.,
.name("tray-events")) for easier debugging, and optionally logging when the event loop exits.keep-desktop/src/app.rs (3)
282-296: Double-parsing of settings JSON for migration detection.The settings file is deserialized twice: once into
Settings(line 287) and once intoserde_json::Value(line 288) just to check ifminimize_to_traywas absent. This is minor, but you could parse toValuefirst, check the field, then deserialize from the sameValue.Suggested optimization
- let settings: Settings = serde_json::from_str(&contents).unwrap_or_default(); - let migrated = serde_json::from_str::<serde_json::Value>(&contents) - .ok() - .map(|v| v.get("minimize_to_tray").is_none()) - .unwrap_or(false); + let value: serde_json::Value = match serde_json::from_str(&contents) { + Ok(v) => v, + Err(_) => return (Settings::default(), false), + }; + let migrated = value.get("minimize_to_tray").is_none(); + let settings: Settings = serde_json::from_value(value).unwrap_or_default();
537-548: Tray events processed duringhandle_tickmay accumulate multiple actions.
poll_tray_eventscollects all pending events and processes them in a single tick. If multiple events queue up (e.g., rapid menu clicks), they'll all execute. TheQuitevent in particular could race with other actions. Sincehandle_tray_quitcallsiced::exit(), subsequent tasks in the batch may not matter, but it's worth being aware that earlier events in the batch could mutate state before the quit.
1750-1758: Re-entrantself.update()call for bunker toggle.
handle_tray_toggle_bunkercallsself.update(Message::BunkerStop)andself.update(Message::BunkerStart), which re-enter the top-level update dispatch. This works but makes the control flow less obvious. Consider calling the underlying handler methods directly (e.g.,self.handle_bunker_message(Message::BunkerStart)) to avoid the full dispatch overhead.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@keep-desktop/src/app.rs`:
- Around line 352-357: The init() state sets window_visible using tray.is_none()
|| !settings.start_minimized which doesn't consider settings.minimize_to_tray
and can desync with new(); update the window_visible initialization to mirror
new()'s hide guard so window_visible is false only when settings.start_minimized
&& has_tray && settings.minimize_to_tray (otherwise true). Locate the
window_visible field in init() and change its expression to use the same
combined condition (referencing init(), new(), window_visible,
settings.start_minimized, settings.minimize_to_tray, and has_tray) so initial
state matches the hide task behavior.
In `@keep-desktop/src/tray.rs`:
- Around line 176-190: In sanitize_notification_field, the character filter
currently removes control chars and '<' and '>' but not '&', which causes Pango
parse errors; update the filter in sanitize_notification_field so it also
excludes '&' (i.e., add *c != '&' to the .filter closure) and ensure truncation
logic (MAX_NOTIFICATION_FIELD_LEN and the truncated branch) remains unchanged.
🧹 Nitpick comments (3)
keep-desktop/src/screen/settings.rs (1)
23-33: Nice DRY improvement with thetoggle_buttonhelper.Consider also refactoring the similar button-building patterns in
auto_lock_cardandclipboard_cardto use this helper or a similar one, as they share the same styling logic.keep-desktop/src/tray.rs (1)
230-236: Test assertion uses byte length (len()) but the function operates on char count.This test passes because the input is ASCII, but it's inconsistent with the function's semantics. The Unicode test at line 275 correctly uses
chars().count(). Consider usingchars().count()here too for consistency.Suggested fix
- assert_eq!(result.len(), MAX_NOTIFICATION_FIELD_LEN + 3); + assert_eq!(result.chars().count(), MAX_NOTIFICATION_FIELD_LEN + 3);keep-desktop/src/app.rs (1)
539-551: Processing tray events duringhandle_tickby collecting then iterating is sound.One consideration: multiple
TrayEvent::ToggleBunkerevents could queue up between ticks, causing rapid start/stop toggling. In practice this is unlikely since menu clicks are rate-limited by user interaction, but if it becomes an issue, consider deduplicating or taking only the last event of each type.
kwsantiago
force-pushed
the
System-tray
branch
from
4cddc5d to
2531e59
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
keep-desktop/src/tray.rs (1)
163-185: Rate-limit tests share mutable static state — may flake under parallel execution.
NOTIFICATION_RATEis a process-wide static. The tests on lines 348–366 reset it directly, but since Rust's test runner executes tests in parallel by default within the same process, concurrent mutations can cause spurious failures.Consider using
serial_testor restructuring the rate limiter to accept injectable state for testability. This isn't a production issue, just a test reliability concern.
…tion improvements
kwsantiago
force-pushed
the
System-tray
branch
from
2531e59 to
95cdd8e
Compare
2 participants
Summary
Test plan
All 37 unit tests passing (23 app tests, 14 tray tests).
Summary by CodeRabbit
New Features
Bug Fixes
Chores