fix: client response handling and desktop tray icon safety (5 bugs)#2884
fix: client response handling and desktop tray icon safety (5 bugs)#2884HUQIANTAO wants to merge 2 commits into
Conversation
1. Fix health_check not consuming response body (client.rs:1018-1038) - Consume response body with resp.text().await on success path - Previously held connection open until drop, exhausting connection pool - Same fix applied to maybe_probe_recovery (client.rs:908-922) 2. Fix synthesize_speech response body read error (client.rs:855) - Replace unwrap_or_default() with .context()? propagation - Connection drops now surface as clear error 3. Fix fim_completion response body read error (client.rs:1368) - Replace unwrap_or_default() with .context()? propagation - Consistent with other API call error handling 4. Fix unwrap() on default_window_icon (desktop/tray.rs:10) - Replace .unwrap() with .ok_or() returning descriptive error - Previously panicked if icon resource missing or corrupted - Now returns proper error through Result propagation
There was a problem hiding this comment.
HUQIANTAO has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Thanks @HUQIANTAO for taking the time to contribute. This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in Please read |
There was a problem hiding this comment.
Code Review
This pull request adds a system tray setup for the desktop application and improves connection pooling and error handling in the TUI client by consuming response bodies in health checks and adding context to response parsing. Feedback is provided regarding the lifetime of the Tauri tray icon, which will be dropped immediately unless stored in Tauri's state.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let _tray = TrayIconBuilder::new() | ||
| .icon(icon.clone()) | ||
| .tooltip("CodeWhale Desktop") | ||
| .on_tray_icon_event(|tray, event| { | ||
| if let TrayIconEvent::Click { | ||
| button: MouseButton::Left, | ||
| button_state: MouseButtonState::Up, | ||
| .. | ||
| } = event | ||
| { | ||
| let app = tray.app_handle(); | ||
| if let Some(window) = app.get_webview_window("main") { | ||
| let _ = window.show(); | ||
| let _ = window.set_focus(); | ||
| } | ||
| } | ||
| }) | ||
| .build(app)?; |
There was a problem hiding this comment.
In Tauri v2, dropping the TrayIcon instance immediately removes the tray icon from the system tray. Since _tray is a local variable in the setup function, it will go out of scope and be dropped at the end of the function, causing the tray icon to disappear immediately after the application starts. To keep the tray icon alive, you should manage its lifetime by storing it in Tauri's state using app.manage(tray).
| let _tray = TrayIconBuilder::new() | |
| .icon(icon.clone()) | |
| .tooltip("CodeWhale Desktop") | |
| .on_tray_icon_event(|tray, event| { | |
| if let TrayIconEvent::Click { | |
| button: MouseButton::Left, | |
| button_state: MouseButtonState::Up, | |
| .. | |
| } = event | |
| { | |
| let app = tray.app_handle(); | |
| if let Some(window) = app.get_webview_window("main") { | |
| let _ = window.show(); | |
| let _ = window.set_focus(); | |
| } | |
| } | |
| }) | |
| .build(app)?; | |
| let tray = TrayIconBuilder::new() | |
| .icon(icon.clone()) | |
| .tooltip("CodeWhale Desktop") | |
| .on_tray_icon_event(|tray, event| { | |
| if let TrayIconEvent::Click { | |
| button: MouseButton::Left, | |
| button_state: MouseButtonState::Up, | |
| .. | |
| } = event | |
| { | |
| let app = tray.app_handle(); | |
| if let Some(window) = app.get_webview_window("main") { | |
| let _ = window.show(); | |
| let _ = window.set_focus(); | |
| } | |
| } | |
| }) | |
| .build(app)?; | |
| app.manage(tray); |
In Tauri v2, dropping the TrayIcon instance immediately removes the tray icon from the system tray. Since the setup function's local variable would go out of scope, use app.manage(tray) to transfer ownership to Tauri's managed state, keeping the icon alive for the application's lifetime.
There was a problem hiding this comment.
HUQIANTAO has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Harvested from PR #2884 by @ousamabenyounes. Drops the orphan desktop tray.rs module (dead code, never wired) from that PR. Co-Authored-By: Ben Younes <2910651+ousamabenyounes@users.noreply.github.com>
|
Thanks @HUQIANTAO — your contribution landed in
Closing this PR now that the code is on If you want to land more work and would prefer your future PRs merge cleanly without a harvest step, the |
Summary
Fixes 5 client and desktop application bugs related to HTTP connection pool management and Tauri component lifecycle.
Bugs Fixed
Connection Pool Management
health_checknot consuming response body (client.rs:1018-1038)resp.text().awaitto consume the body.maybe_probe_recoverynot consuming response body (client.rs:908-922)resp.text().await.synthesize_speechresponse body read error (client.rs:855)unwrap_or_default()converts connection drops to empty string, causing misleading JSON parse error..context()?error propagation.fim_completionresponse body read error (client.rs:1368).context()?error propagation.Tauri Desktop
desktop/tray.rs:10)TrayIconinstance removes it from the system tray. The local variable goes out of scope at the end ofsetup().app.manage(tray)to transfer ownership to Tauri's managed state, keeping the icon alive for the application's lifetime.