fix: confirm desktop close behavior before hiding to tray#118
Open
zouyonghe wants to merge 18 commits intoAstrBotDevs:mainfrom
Open
fix: confirm desktop close behavior before hiding to tray#118zouyonghe wants to merge 18 commits intoAstrBotDevs:mainfrom
zouyonghe wants to merge 18 commits intoAstrBotDevs:mainfrom
Conversation
* fix: add backend startup heartbeat liveness probe * fix: tighten startup heartbeat validation * refactor: centralize startup heartbeat metadata * fix: surface heartbeat invalidation sooner * fix: harden startup heartbeat parsing * fix: warn on stop-time heartbeat failures * refactor: simplify startup heartbeat control flow * refactor: flatten readiness heartbeat helpers * refactor: clarify heartbeat helper responsibilities * docs: clarify startup heartbeat path coupling * fix: harden startup heartbeat coordination * fix: make startup heartbeat checks monotonic * fix: clean up heartbeat test and exit handling
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
desktop_bridge_submit_close_prompt, consider treating failures to close theclose-confirmwindow as non-fatal (log and still returnok: true) since the primary action (tray/exit) has already been executed and surfacing this as an error unnecessarily complicates the frontend flow. - The
CloseActionstring mapping is duplicated via theserdeannotations,CloseAction::parse,parse_close_action, andas_state_value; you could simplify by relying onserdefor serialization/deserialization and removing the manual string conversions to reduce drift when new actions are added. - The localization logic in
ui/close-confirm.htmloverridestrayButton.textContentandexitButton.textContentconditionally even thoughmessagesalready holds localized strings; simplifying this to always usecopy.tray/copy.exit(and optionally avoiding leaking rawinvokeErrormessages to users) would make the UI code more consistent and robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `desktop_bridge_submit_close_prompt`, consider treating failures to close the `close-confirm` window as non-fatal (log and still return `ok: true`) since the primary action (tray/exit) has already been executed and surfacing this as an error unnecessarily complicates the frontend flow.
- The `CloseAction` string mapping is duplicated via the `serde` annotations, `CloseAction::parse`, `parse_close_action`, and `as_state_value`; you could simplify by relying on `serde` for serialization/deserialization and removing the manual string conversions to reduce drift when new actions are added.
- The localization logic in `ui/close-confirm.html` overrides `trayButton.textContent` and `exitButton.textContent` conditionally even though `messages` already holds localized strings; simplifying this to always use `copy.tray`/`copy.exit` (and optionally avoiding leaking raw `invokeError` messages to users) would make the UI code more consistent and robust.
## Individual Comments
### Comment 1
<location path="src-tauri/src/bridge/commands.rs" line_range="330-341" />
<code_context>
+ }
+
+ match action {
+ CloseAction::Tray => {
+ window::actions::hide_main_window(
+ &app_handle,
+ DEFAULT_SHELL_LOCALE,
+ append_desktop_log,
+ );
+ if let Some(prompt_window) = app_handle.get_webview_window("close-confirm") {
+ if let Err(error) = prompt_window.close() {
+ let reason = format!("Failed to close close confirm prompt window: {error}");
+ append_desktop_log(&reason);
+ return BackendBridgeResult {
+ ok: false,
+ reason: Some(reason),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider treating prompt-window close failures as non-fatal for the selected tray action.
In the tray path the main window is already hidden before `prompt_window.close()` is called, so the chosen action (minimize to tray) has succeeded even if closing the `close-confirm` window fails. Returning `{ ok: false }` here will surface this as a failed action in the UI, which is misleading because only a cosmetic cleanup failed. Consider logging the error but still returning `{ ok: true }` (or otherwise distinguishing cosmetic failures) so the UI doesn’t show an error for a successful tray action.
```suggestion
if let Some(prompt_window) = app_handle.get_webview_window("close-confirm") {
if let Err(error) = prompt_window.close() {
append_desktop_log(&format!(
"Failed to close close confirm prompt window: {error}"
));
}
}
BackendBridgeResult {
```
</issue_to_address>
### Comment 2
<location path="ui/close-confirm.html" line_range="204-205" />
<code_context>
+ document.getElementById("title").textContent = copy.title;
+ document.getElementById("description").textContent = copy.description;
+ document.getElementById("remember-label").textContent = copy.remember;
+ trayButton.textContent = locale === "zh-CN" ? copy.tray : "Minimize to tray";
+ exitButton.textContent = locale === "zh-CN" ? copy.exit : "Exit now";
+
+ function setPending(pending) {
</code_context>
<issue_to_address>
**nitpick:** Avoid duplicating the English labels already present in the locale map.
For `en-US`, `copy.tray` and `copy.exit` already provide these strings, so the ternaries only special-case `zh-CN` while duplicating the English literals. This risks the literals and the locale map drifting out of sync. Prefer always using `copy.tray` / `copy.exit` so the `messages` map remains the single source of truth.
</issue_to_address>
### Comment 3
<location path="src-tauri/src/close_behavior.rs" line_range="15" />
<code_context>
+ Value::Object(Map::new())
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
+#[serde(rename_all = "lowercase")]
+pub(crate) enum CloseAction {
</code_context>
<issue_to_address>
**issue (complexity):** Consider leveraging serde derives, a small typed DesktopState struct, and simpler path handling to reduce the amount of custom JSON and enum plumbing in this module while keeping the same behavior.
You can keep the same behavior while trimming quite a bit of surface area. Concretely:
### 1. Let serde handle enum parsing/serialization
`CloseAction::parse`, `as_state_value`, and `parse_close_action` duplicate what serde already does for you given the `#[serde(rename_all = "lowercase")]` derive.
You can drop those helpers and use serde directly:
```rust
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "lowercase")]
pub(crate) enum CloseAction {
Tray,
Exit,
}
// Example usage in read/write:
fn parse_close_action_value(v: &Value) -> Option<CloseAction> {
serde_json::from_value::<CloseAction>(v.clone()).ok()
}
fn close_action_to_value(action: CloseAction) -> Value {
serde_json::to_value(action).expect("enum to serialize")
}
```
Then your read/write code can work directly with `CloseAction` via serde instead of manual `"tray"` / `"exit"` mapping.
### 2. Replace generic `Value` machinery with a small state struct
You currently have `empty_state_object`, `load_state_value`, and `ensure_object` to normalize JSON into an object and preserve unknown fields. You can keep all of that behavior but collapse it into a single typed struct that uses `#[serde(flatten)]`:
```rust
use serde::{Deserialize, Serialize};
use serde_json::{Map, Value};
const CLOSE_ACTION_FIELD: &str = "closeActionOnWindowClose";
#[derive(Serialize, Deserialize, Default)]
struct DesktopState {
#[serde(rename = "closeActionOnWindowClose")]
#[serde(skip_serializing_if = "Option::is_none")]
close_action: Option<CloseAction>,
// Preserve all other fields verbatim
#[serde(flatten)]
rest: Map<String, Value>,
}
fn load_desktop_state(raw: &str, log_subject: &str) -> DesktopState {
match serde_json::from_str::<DesktopState>(raw) {
Ok(state) => state,
Err(error) => {
crate::append_desktop_log(&format!(
"failed to parse {log_subject}: {error}. resetting state semantics"
));
DesktopState::default()
}
}
}
```
Then your read/write become much simpler:
```rust
fn read_cached_close_action_from_state_path(state_path: Option<PathBuf>) -> Option<CloseAction> {
let raw = fs::read_to_string(state_path?).ok()?;
let state = load_desktop_state(&raw, "desktop close behavior state");
state.close_action
}
fn write_cached_close_action_to_state_path(
action: Option<CloseAction>,
state_path: Option<PathBuf>,
) -> Result<(), String> {
let Some(state_path) = state_path else {
crate::append_desktop_log(
"close behavior state path is unavailable; skipping close action persistence",
);
return Ok(());
};
let mut state = match fs::read_to_string(&state_path) {
Ok(raw) => load_desktop_state(
&raw,
&format!("close behavior state {}", state_path.display()),
),
Err(e) if e.kind() == std::io::ErrorKind::NotFound => DesktopState::default(),
Err(e) => {
return Err(format!(
"Failed to read close behavior state {}: {}",
state_path.display(),
e
));
}
};
state.close_action = action;
save_state(&state_path, &serde_json::to_value(&state).unwrap())
}
```
This removes `empty_state_object`, `ensure_object`, manual map insert/remove, and the `CLOSE_ACTION_FIELD` string manipulation, while still:
- Treating malformed/incorrect-root JSON as “reset to empty object”.
- Preserving unrelated fields via `rest`.
- Allowing the `None` case to remove the close-action field.
### 3. Collapse the `Option<Path>` plumbing
The `*_from_state_path` / `*_to_state_path` pairs add an extra level of indirection for the `Option<PathBuf>` that doesn’t buy much.
You can push the `Option` handling to the public API and make the internal helpers work with `&Path`, which simplifies signatures and branching:
```rust
pub(crate) fn read_cached_close_action(
packaged_root_dir: Option<&Path>,
) -> Option<CloseAction> {
let state_path =
crate::desktop_state::resolve_desktop_state_path(packaged_root_dir)?;
read_cached_close_action_at_path(&state_path)
}
fn read_cached_close_action_at_path(state_path: &Path) -> Option<CloseAction> {
let raw = fs::read_to_string(state_path).ok()?;
let state = load_desktop_state(&raw, "desktop close behavior state");
state.close_action
}
pub(crate) fn write_cached_close_action(
action: Option<CloseAction>,
packaged_root_dir: Option<&Path>,
) -> Result<(), String> {
let Some(state_path) =
crate::desktop_state::resolve_desktop_state_path(packaged_root_dir)
else {
crate::append_desktop_log(
"close behavior state path is unavailable; skipping close action persistence",
);
return Ok(());
};
write_cached_close_action_at_path(action, &state_path)
}
fn write_cached_close_action_at_path(
action: Option<CloseAction>,
state_path: &Path,
) -> Result<(), String> {
// same as above, but with &Path
}
```
This keeps the behavior (no-op + log when path is unavailable) but reduces the number of functions and avoids threading `Option<PathBuf>` through the whole module.
---
If there is already a shared “atomic JSON save” helper elsewhere in the crate, wiring `save_state` through that helper instead of hand-rolling the temp-file pattern here would further cut down conceptual surface area while preserving the durability guarantees.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a close confirmation dialog that allows users to choose between minimizing the application to the system tray or exiting completely. It introduces a new UI component, a persistence layer for saving user preferences, and logic within the window event loop to handle close requests based on these preferences. Key feedback includes addressing a potential file-locking issue on Windows during state persistence, migrating from internal to public Tauri APIs, and optimizing performance by avoiding synchronous I/O on the main thread. Other suggestions focus on improving code maintainability through the use of constants and simplifying localization logic.
Comment on lines
+112
to
+128
| let mut file = fs::File::create(&tmp_path).map_err(|error| { | ||
| format!( | ||
| "Failed to create temporary close behavior state file {}: {}", | ||
| tmp_path.display(), | ||
| error | ||
| ) | ||
| })?; | ||
| file.write_all(serialized.as_bytes()) | ||
| .and_then(|_| file.sync_all()) | ||
| .map_err(|error| { | ||
| format!( | ||
| "Failed to write temporary close behavior state file {}: {}", | ||
| tmp_path.display(), | ||
| error | ||
| ) | ||
| })?; | ||
| fs::rename(&tmp_path, path).map_err(|error| { |
Contributor
There was a problem hiding this comment.
On Windows, fs::rename will likely fail if the source file is still open. The file handle created at line 112 remains in scope and holds an open handle to the temporary file until the end of the function. You should explicitly drop the file handle or wrap the write operations in a block before attempting to rename the file.
Suggested change
| let mut file = fs::File::create(&tmp_path).map_err(|error| { | |
| format!( | |
| "Failed to create temporary close behavior state file {}: {}", | |
| tmp_path.display(), | |
| error | |
| ) | |
| })?; | |
| file.write_all(serialized.as_bytes()) | |
| .and_then(|_| file.sync_all()) | |
| .map_err(|error| { | |
| format!( | |
| "Failed to write temporary close behavior state file {}: {}", | |
| tmp_path.display(), | |
| error | |
| ) | |
| })?; | |
| fs::rename(&tmp_path, path).map_err(|error| { | |
| let mut file = fs::File::create(&tmp_path).map_err(|error| { | |
| format!( | |
| "Failed to create temporary close behavior state file {}: {}", | |
| tmp_path.display(), | |
| error | |
| ) | |
| })?; | |
| file.write_all(serialized.as_bytes()) | |
| .and_then(|_| file.sync_all()) | |
| .map_err(|error| { | |
| format!( | |
| "Failed to write temporary close behavior state file {}: {}", | |
| tmp_path.display(), | |
| error | |
| ) | |
| })?; | |
| drop(file); | |
| fs::rename(&tmp_path, path).map_err(|error| { |
| setPending(true); | ||
|
|
||
| try { | ||
| const result = await window.__TAURI_INTERNALS__.invoke( |
Contributor
There was a problem hiding this comment.
Using window.__TAURI_INTERNALS__ is discouraged as it targets internal APIs that are subject to change without notice. For Tauri v2, you should use the standard window.__TAURI__.core.invoke if the global Tauri object is enabled, or import invoke from the @tauri-apps/api package if using a bundler.
Suggested change
| const result = await window.__TAURI_INTERNALS__.invoke( | |
| const result = await window.__TAURI__.core.invoke( |
Comment on lines
+27
to
+29
| let packaged_root_dir = crate::runtime_paths::default_packaged_root_dir(); | ||
| let saved_close_action = | ||
| close_behavior::read_cached_close_action(packaged_root_dir.as_deref()); |
Contributor
There was a problem hiding this comment.
Performing synchronous file I/O and environment/home directory resolution within the window event loop can lead to UI unresponsiveness or stutters, as this closure runs on the main thread. Consider caching the packaged_root_dir and the saved_close_action in the BackendState during application startup to avoid blocking the event loop on every close request.
| DEFAULT_SHELL_LOCALE, | ||
| append_desktop_log, | ||
| ); | ||
| if let Some(prompt_window) = app_handle.get_webview_window("close-confirm") { |
Contributor
There was a problem hiding this comment.
Avoid using hardcoded strings for window labels. It is better to use the constant defined in the window::close_confirm module to ensure consistency across the codebase.
Suggested change
| if let Some(prompt_window) = app_handle.get_webview_window("close-confirm") { | |
| if let Some(prompt_window) = app_handle.get_webview_window(window::close_confirm::CLOSE_CONFIRM_WINDOW_LABEL) { |
| @@ -0,0 +1,78 @@ | |||
| use tauri::{AppHandle, Manager, WebviewUrl, WebviewWindowBuilder}; | |||
|
|
|||
| const CLOSE_CONFIRM_WINDOW_LABEL: &str = "close-confirm"; | |||
Contributor
There was a problem hiding this comment.
This constant should be made pub(crate) so it can be referenced by the bridge commands instead of using hardcoded strings.
Suggested change
| const CLOSE_CONFIRM_WINDOW_LABEL: &str = "close-confirm"; | |
| pub(crate) const CLOSE_CONFIRM_WINDOW_LABEL: &str = "close-confirm"; |
Comment on lines
+204
to
+205
| trayButton.textContent = locale === "zh-CN" ? copy.tray : "Minimize to tray"; | ||
| exitButton.textContent = locale === "zh-CN" ? copy.exit : "Exit now"; |
Contributor
There was a problem hiding this comment.
The ternary logic here is redundant because the copy object is already localized based on the detected locale. You can directly use the properties from the copy object.
Suggested change
| trayButton.textContent = locale === "zh-CN" ? copy.tray : "Minimize to tray"; | |
| exitButton.textContent = locale === "zh-CN" ? copy.exit : "Exit now"; | |
| trayButton.textContent = copy.tray; | |
| exitButton.textContent = copy.exit; |
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src-tauri/src/bridge/commands.rs" line_range="177-178" />
<code_context>
+ Log: Fn(&str),
+{
+ if let Err(error) = cleanup_result {
+ log(&format!(
+ "Failed to close close confirm prompt window: {error}"
+ ));
+ }
</code_context>
<issue_to_address>
**nitpick (typo):** The log message for close-confirm cleanup reads slightly awkwardly with a repeated "close".
The string "Failed to close close confirm prompt window" has a duplicated "close" that makes the log harder to scan. Consider rephrasing to something like "Failed to close confirm prompt window" or "Failed to close the close-confirm prompt window" for readability.
Suggested implementation:
```rust
fn finish_tray_close_prompt_cleanup<Log>(
cleanup_result: Result<(), String>,
log: Log,
) -> BackendBridgeResult
where
Log: Fn(&str),
{
if let Err(error) = cleanup_result {
log(&format!(
"Failed to close confirm prompt window: {error}"
));
}
Ok(BackendBridgeResponse::Success)
}
```
1. If `BackendBridgeResult` / `BackendBridgeResponse::Success` are not the correct result types for this function in your codebase, adjust the return to match the existing convention (e.g. `Ok(())` or another success variant).
2. Ensure `BackendBridgeResponse` is imported or in scope if that's the expected return type; otherwise, replace it with the appropriate type already used elsewhere in `commands.rs`.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/bridge/commands.rs" line_range="164" />
<code_context>
}
}
+fn parse_close_prompt_action(raw_action: &str) -> Result<CloseAction, String> {
+ close_behavior::parse_close_action(raw_action)
+ .ok_or_else(|| "Invalid close action. Expected 'tray' or 'exit'.".to_string())
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the close-prompt cleanup and parsing logic into the command to avoid extra helper layers that add indirection without clear benefit.
You can reduce indirection here without losing any functionality.
### 1. Remove `finish_tray_close_prompt_cleanup` indirection
The generic helper is only used once and always with `append_desktop_log`. You can inline the logic and drop the generic function and its tests:
```rust
match action {
CloseAction::Tray => {
window::actions::hide_main_window(
&app_handle,
DEFAULT_SHELL_LOCALE,
append_desktop_log,
);
if let Some(prompt_window) = app_handle.get_webview_window("close-confirm") {
if let Err(error) = prompt_window.close() {
append_desktop_log(&format!(
"Failed to close close confirm prompt window: {error}"
));
}
}
BackendBridgeResult {
ok: true,
reason: None,
}
}
CloseAction::Exit => {
let state = app_handle.state::<BackendState>();
state.mark_quitting();
app_handle.exit(0);
BackendBridgeResult {
ok: true,
reason: None,
}
}
}
```
Then you can delete `finish_tray_close_prompt_cleanup` and its tests.
### 2. Avoid wrapping `parse_close_action` with another parser
Instead of `parse_close_prompt_action`, you can map the `Option` from `close_behavior::parse_close_action` directly at the call site, keeping only one parsing API:
```rust
#[tauri::command]
pub(crate) fn desktop_bridge_submit_close_prompt(
app_handle: AppHandle,
action: String,
remember: bool,
) -> BackendBridgeResult {
let action = match close_behavior::parse_close_action(&action) {
Some(action) => action,
None => {
return BackendBridgeResult {
ok: false,
reason: Some("Invalid close action. Expected 'tray' or 'exit'.".to_string()),
};
}
};
// ... rest unchanged ...
}
```
If you still want a testable parsing function, you can move an ergonomic `Result`-returning parser into `close_behavior` itself:
```rust
// in close_behavior
pub fn parse_close_action_result(raw: &str) -> Result<CloseAction, String> {
parse_close_action(raw)
.ok_or_else(|| "Invalid close action. Expected 'tray' or 'exit'.".to_string())
}
```
And in the command:
```rust
let action = match close_behavior::parse_close_action_result(&action) {
Ok(action) => action,
Err(error) => {
return BackendBridgeResult {
ok: false,
reason: Some(error),
};
}
};
```
This removes the extra abstraction layer in the bridge module and keeps the flow in `desktop_bridge_submit_close_prompt` more direct.
</issue_to_address>
### Comment 3
<location path="src-tauri/src/close_behavior.rs" line_range="34" />
<code_context>
+ rest: Map<String, Value>,
+}
+
+pub(crate) fn parse_close_action(raw: &str) -> Option<CloseAction> {
+ serde_json::from_value(Value::String(raw.to_string())).ok()
+}
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying CloseAction parsing and deserialization by matching directly on strings instead of routing through serde_json::Value and from_value, which will keep behavior while making the code easier to follow.
You can reduce indirection around `CloseAction` parsing/deserialization without changing behavior.
### 1. Avoid going through `serde_json::Value` for `CloseAction`
Both `parse_close_action` and `deserialize_close_action_option` currently route through `serde_json::Value` and `serde_json::from_value`, which makes a simple string→enum mapping harder to follow.
You can keep the “invalid → None (no error)” semantics and the `#[serde(rename_all = "lowercase")]` behavior while simplifying to direct string matching:
```rust
pub(crate) fn parse_close_action(raw: &str) -> Option<CloseAction> {
match raw {
"tray" => Some(CloseAction::Tray),
"exit" => Some(CloseAction::Exit),
_ => None,
}
}
```
Then reuse this in the custom deserializer, instead of touching `serde_json::Value` at all:
```rust
fn deserialize_close_action_option<'de, D>(
deserializer: D,
) -> Result<Option<CloseAction>, D::Error>
where
D: Deserializer<'de>,
{
let raw = Option::<String>::deserialize(deserializer)?;
Ok(raw.as_deref().and_then(parse_close_action))
}
```
This keeps:
- `tray` / `exit` accepted,
- anything else treated as `None` (as your tests expect),
- unknown fields preserved via `rest: Map<String, Value>`,
but makes the control flow much easier to read and debug by removing JSON-specific machinery from a simple enum mapping.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
|
|
||
| fn parse_close_prompt_action(raw_action: &str) -> Result<CloseAction, String> { |
There was a problem hiding this comment.
issue (complexity): Consider inlining the close-prompt cleanup and parsing logic into the command to avoid extra helper layers that add indirection without clear benefit.
You can reduce indirection here without losing any functionality.
1. Remove finish_tray_close_prompt_cleanup indirection
The generic helper is only used once and always with append_desktop_log. You can inline the logic and drop the generic function and its tests:
match action {
CloseAction::Tray => {
window::actions::hide_main_window(
&app_handle,
DEFAULT_SHELL_LOCALE,
append_desktop_log,
);
if let Some(prompt_window) = app_handle.get_webview_window("close-confirm") {
if let Err(error) = prompt_window.close() {
append_desktop_log(&format!(
"Failed to close close confirm prompt window: {error}"
));
}
}
BackendBridgeResult {
ok: true,
reason: None,
}
}
CloseAction::Exit => {
let state = app_handle.state::<BackendState>();
state.mark_quitting();
app_handle.exit(0);
BackendBridgeResult {
ok: true,
reason: None,
}
}
}Then you can delete finish_tray_close_prompt_cleanup and its tests.
2. Avoid wrapping parse_close_action with another parser
Instead of parse_close_prompt_action, you can map the Option from close_behavior::parse_close_action directly at the call site, keeping only one parsing API:
#[tauri::command]
pub(crate) fn desktop_bridge_submit_close_prompt(
app_handle: AppHandle,
action: String,
remember: bool,
) -> BackendBridgeResult {
let action = match close_behavior::parse_close_action(&action) {
Some(action) => action,
None => {
return BackendBridgeResult {
ok: false,
reason: Some("Invalid close action. Expected 'tray' or 'exit'.".to_string()),
};
}
};
// ... rest unchanged ...
}If you still want a testable parsing function, you can move an ergonomic Result-returning parser into close_behavior itself:
// in close_behavior
pub fn parse_close_action_result(raw: &str) -> Result<CloseAction, String> {
parse_close_action(raw)
.ok_or_else(|| "Invalid close action. Expected 'tray' or 'exit'.".to_string())
}And in the command:
let action = match close_behavior::parse_close_action_result(&action) {
Ok(action) => action,
Err(error) => {
return BackendBridgeResult {
ok: false,
reason: Some(error),
};
}
};This removes the extra abstraction layer in the bridge module and keeps the flow in desktop_bridge_submit_close_prompt more direct.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The exit flow is implemented both in
MainWindowAction::ExitApplicationand indesktop_bridge_submit_close_prompt(markingquitting, logging, and callingapp_handle.exit(0)), which could be refactored into a shared helper to keep shutdown behavior and logging consistent across all exit paths. - The close-confirm dialog script calls
window.__TAURI_INTERNALS__.invokedirectly; if you already have a wrapper or public Tauri API usage elsewhere, consider routing this through that instead to avoid depending on Tauri internals and keep the shell layer consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The exit flow is implemented both in `MainWindowAction::ExitApplication` and in `desktop_bridge_submit_close_prompt` (marking `quitting`, logging, and calling `app_handle.exit(0)`), which could be refactored into a shared helper to keep shutdown behavior and logging consistent across all exit paths.
- The close-confirm dialog script calls `window.__TAURI_INTERNALS__.invoke` directly; if you already have a wrapper or public Tauri API usage elsewhere, consider routing this through that instead to avoid depending on Tauri internals and keep the shell layer consistent.
## Individual Comments
### Comment 1
<location path="src-tauri/src/close_behavior.rs" line_range="12-16" />
<code_context>
+ Exit,
+}
+
+fn deserialize_close_action_option<'de, D>(deserializer: D) -> Result<Option<CloseAction>, D::Error>
+where
+ D: Deserializer<'de>,
+{
+ let raw = Option::<String>::deserialize(deserializer)?;
+ Ok(raw.as_deref().and_then(parse_close_action))
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** The custom deserializer for close_action will fail hard on non-string values, dropping the entire state.
Because this helper deserializes via `Option::<String>::deserialize`, any non-string `closeActionOnWindowClose` (e.g., `true`, `0`, or an object) will cause `serde_json::from_str::<DesktopState>` to fail and `load_desktop_state` to fall back to `DesktopState::default()`, discarding the saved state. To avoid losing the whole state on malformed values while keeping the field optional, you could deserialize as `Option<serde_json::Value>` and only call `parse_close_action` for `Value::String(s)`, treating other types like invalid strings (i.e., `None`) instead of an error.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/close_behavior.rs" line_range="65" />
<code_context>
+ state.close_action
+}
+
+fn ensure_parent_dir(path: &Path) -> Result<(), String> {
+ if let Some(parent_dir) = path.parent() {
+ fs::create_dir_all(parent_dir).map_err(|error| {
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the I/O helpers by specializing `save_state` to `DesktopState` and inlining `ensure_parent_dir` to reduce unnecessary abstraction and surface area without changing behavior.
You can shave off a layer of abstraction in the I/O helpers without changing behavior by specializing `save_state` to `DesktopState` and inlining `ensure_parent_dir`. This simplifies the signatures and error surface while keeping the same semantics and tests.
For example:
```rust
fn save_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
if let Some(parent_dir) = path.parent() {
fs::create_dir_all(parent_dir).map_err(|error| {
format!(
"Failed to create close behavior directory {}: {}",
parent_dir.display(),
error
)
})?;
}
let serialized = serde_json::to_string_pretty(state)
.map_err(|error| format!("Failed to serialize close behavior state: {error}"))?;
let tmp_name = format!(
"{}.tmp",
path.file_name()
.map(|value| value.to_string_lossy())
.unwrap_or_default()
);
let tmp_path = path.with_file_name(tmp_name);
let mut file = fs::File::create(&tmp_path).map_err(|error| {
format!(
"Failed to create temporary close behavior state file {}: {}",
tmp_path.display(),
error
)
})?;
file.write_all(serialized.as_bytes())
.and_then(|_| file.sync_all())
.map_err(|error| {
format!(
"Failed to write temporary close behavior state file {}: {}",
tmp_path.display(),
error
)
})?;
fs::rename(&tmp_path, path).map_err(|error| {
format!(
"Failed to atomically replace close behavior state file {}: {}",
path.display(),
error
)
})
}
```
Then update the call site:
```rust
fn write_cached_close_action_at_path(
action: Option<CloseAction>,
state_path: &Path,
) -> Result<(), String> {
let mut state = match fs::read_to_string(state_path) {
Ok(raw) => load_desktop_state(
&raw,
&format!("close behavior state {}", state_path.display()),
),
Err(error) if error.kind() == std::io::ErrorKind::NotFound => DesktopState::default(),
Err(error) => {
return Err(format!(
"Failed to read close behavior state {}: {}",
state_path.display(),
error
));
}
};
state.close_action = action;
save_desktop_state(state_path, &state)
}
```
This keeps all existing behavior (including directory creation, temp file usage, and error messages) while removing a generic and a helper that currently only serve this one type.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src-tauri/src/bridge/commands.rs" line_range="177-179" />
<code_context>
+ .ok_or_else(|| "Invalid close action. Expected 'tray' or 'exit'.".to_string())
+}
+
+fn finish_tray_close_prompt_cleanup<Log>(
+ cleanup_result: Result<(), String>,
+ log: Log,
+) -> BackendBridgeResult
+where
+ Log: Fn(&str),
+{
+ if let Err(error) = cleanup_result {
+ log(&format!(
+ "Failed to close close confirm prompt window: {error}"
+ ));
+ }
</code_context>
<issue_to_address>
**nitpick (typo):** Adjust wording in the log message to avoid the duplicated "close" phrasing.
The log message currently says `"Failed to close close confirm prompt window"`, which is awkward. Please rephrase to remove the duplicated "close", for example: `"Failed to close confirm prompt window"` or `"Failed to close close-confirm prompt window"`.
```suggestion
log(&format!(
"Failed to close confirm prompt window: {error}"
));
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The window label "close-confirm" is hard-coded in
desktop_bridge_submit_close_promptwhen looking up the prompt window; consider reusing theCLOSE_CONFIRM_WINDOW_LABELconstant fromwindow::close_confirmto avoid label drift between modules. - The close action values ("tray"/"exit") are duplicated as string literals in both the Rust layer (
parse_close_action) and the HTML dialog (submit("tray"/"exit")); wiring these through a shared enum/constant or a generated config would reduce the risk of the two drifting out of sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The window label "close-confirm" is hard-coded in `desktop_bridge_submit_close_prompt` when looking up the prompt window; consider reusing the `CLOSE_CONFIRM_WINDOW_LABEL` constant from `window::close_confirm` to avoid label drift between modules.
- The close action values ("tray"/"exit") are duplicated as string literals in both the Rust layer (`parse_close_action`) and the HTML dialog (`submit("tray"/"exit")`); wiring these through a shared enum/constant or a generated config would reduce the risk of the two drifting out of sync.
## Individual Comments
### Comment 1
<location path="ui/close-confirm.html" line_range="196-199" />
<code_context>
+ const trayButton = document.getElementById("tray-button");
+ const exitButton = document.getElementById("exit-button");
+ const error = document.getElementById("error");
+ const invokeTauri = window.__TAURI__?.core?.invoke || window.__TAURI__?.invoke || window.__TAURI_INTERNALS__?.invoke;
+ let recoveryTimer = null;
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Relying on `window.__TAURI_INTERNALS__` as a fallback is brittle; consider failing gracefully instead of depending on private APIs.
This fallback couples the dialog to a private Tauri API that may change without notice. Since there is already a user‑friendly path when `invokeTauri` is not a function, consider removing the `__TAURI_INTERNALS__` branch and relying only on `window.__TAURI__?.core?.invoke || window.__TAURI__?.invoke` to stay within supported APIs.
```suggestion
const exitButton = document.getElementById("exit-button");
const error = document.getElementById("error");
const invokeTauri = window.__TAURI__?.core?.invoke || window.__TAURI__?.invoke;
let recoveryTimer = null;
```
</issue_to_address>
### Comment 2
<location path="src-tauri/src/close_behavior.rs" line_range="12" />
<code_context>
+ Exit,
+}
+
+fn deserialize_close_action_option<'de, D>(deserializer: D) -> Result<Option<CloseAction>, D::Error>
+where
+ D: Deserializer<'de>,
</code_context>
<issue_to_address>
**issue (complexity):** Consider deserializing `close_action` via `Option<String>` instead of `Option<Value>` to express the optional string-to-`CloseAction` mapping more directly.
You can simplify the custom deserialization for `close_action` and avoid going through `serde_json::Value`, which reduces indirection without changing behavior.
Currently:
```rust
fn deserialize_close_action_option<'de, D>(deserializer: D) -> Result<Option<CloseAction>, D::Error>
where
D: Deserializer<'de>,
{
let raw = Option::<Value>::deserialize(deserializer)?;
Ok(match raw {
Some(Value::String(raw)) => parse_close_action(&raw),
_ => None,
})
}
```
This can be expressed more directly by deserializing to `Option<String>` instead of `Option<Value>` (you still treat non-strings and invalid values as `None`):
```rust
fn deserialize_close_action_option<'de, D>(deserializer: D) -> Result<Option<CloseAction>, D::Error>
where
D: Deserializer<'de>,
{
let raw = Option::<String>::deserialize(deserializer)?;
Ok(raw.as_deref().and_then(parse_close_action))
}
```
This keeps all current semantics:
- Non-string `closeActionOnWindowClose` → `None`
- Invalid string value → `None`
- Other fields preserved via `rest` unchanged
…but removes the need to pattern-match on `Value` and makes the intent of “optional string, mapped through `parse_close_action`” clearer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
close_behavior::load_desktop_stateandwrite_cached_close_action, consider accepting a logging callback instead of callingcrate::append_desktop_logdirectly so the module remains easier to reuse and test in isolation without depending on global logging. - In
parse_close_prompt_action, build the error message usingCLOSE_ACTION_TRAYandCLOSE_ACTION_EXITrather than hard-coded'tray'/'exit'strings so the validation message stays in sync if the allowed values ever change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `close_behavior::load_desktop_state` and `write_cached_close_action`, consider accepting a logging callback instead of calling `crate::append_desktop_log` directly so the module remains easier to reuse and test in isolation without depending on global logging.
- In `parse_close_prompt_action`, build the error message using `CLOSE_ACTION_TRAY` and `CLOSE_ACTION_EXIT` rather than hard-coded `'tray'`/`'exit'` strings so the validation message stays in sync if the allowed values ever change.
## Individual Comments
### Comment 1
<location path="src-tauri/src/bridge/commands.rs" line_range="164-167" />
<code_context>
}
}
+fn parse_close_prompt_action(raw_action: &str) -> Result<CloseAction, String> {
+ close_behavior::parse_close_action(raw_action)
+ .ok_or_else(|| "Invalid close action. Expected 'tray' or 'exit'.".to_string())
+}
</code_context>
<issue_to_address>
**suggestion:** Avoid hardcoding expected action strings in the error message to stay consistent with `close_behavior` constants.
The error text hardcodes `'tray'` and `'exit'` while the accepted values are defined in `close_behavior::CLOSE_ACTION_TRAY` / `CLOSE_ACTION_EXIT`. If those constants change, the message will be wrong. Please build the error text from those constants (or the `CloseAction` enum) so it stays in sync.
```suggestion
fn parse_close_prompt_action(raw_action: &str) -> Result<CloseAction, String> {
close_behavior::parse_close_action(raw_action).ok_or_else(|| {
format!(
"Invalid close action. Expected '{}' or '{}'.",
close_behavior::CLOSE_ACTION_TRAY,
close_behavior::CLOSE_ACTION_EXIT,
)
})
}
```
</issue_to_address>
### Comment 2
<location path="src-tauri/src/window/close_confirm.rs" line_range="26-30" />
<code_context>
+where
+ F: Fn(&str),
+{
+ if let Some(window) = app_handle.get_webview_window(CLOSE_CONFIRM_WINDOW_LABEL) {
+ if let Err(error) = window.unminimize() {
+ log(&format!(
+ "failed to unminimize close confirm window: {error}"
+ ));
+ }
+ if let Err(error) = window.show() {
+ log(&format!("failed to show close confirm window: {error}"));
+ }
+ if let Err(error) = window.set_focus() {
+ log(&format!("failed to focus close confirm window: {error}"));
+ }
+ return Ok(());
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The reopen path silently ignores failures to unminimize/show/focus the existing window, which might mask issues.
When the window already exists, errors from `unminimize`, `show`, and `set_focus` are only logged and the function still returns `Ok(())`. This can leave callers believing the dialog is visible and focused when it isn’t. Consider returning an `Err` on the first failure so callers can apply a fallback (e.g., hide the main window directly or show an alternative UX) instead of treating this as success.
```suggestion
log: F,
) -> Result<(), String>
where
F: Fn(&str),
{
if let Some(window) = app_handle.get_webview_window(CLOSE_CONFIRM_WINDOW_LABEL) {
if let Err(error) = window.unminimize() {
let message = format!("failed to unminimize close confirm window: {error}");
log(&message);
return Err(message);
}
if let Err(error) = window.show() {
let message = format!("failed to show close confirm window: {error}");
log(&message);
return Err(message);
}
if let Err(error) = window.set_focus() {
let message = format!("failed to focus close confirm window: {error}");
log(&message);
return Err(message);
}
return Ok(());
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src-tauri/src/close_behavior.rs" line_range="55" />
<code_context>
+ }
+}
+
+fn load_desktop_state<F>(raw: &str, log_subject: &str, log: &F) -> DesktopState
+where
+ F: Fn(&str),
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the logging and file-writing utilities by centralizing logger usage and extracting a reusable atomic JSON writer helper to reduce boilerplate and clarify responsibilities.
You can reduce complexity most effectively by simplifying the logging plumbing and centralizing the IO helpers, without changing behavior.
### 1. Narrow the logging surface (`Fn` generics → shared logger type)
Right now almost every function is generic over `F: Fn(&str)`, including internal helpers like `load_desktop_state` and `*_at_path`. That adds noise and duplicates the “logging capability” concept at every layer.
You can define a shared logger type and use `&Logger` internally, while keeping the public API generic for flexibility:
```rust
type Logger = dyn Fn(&str);
fn load_desktop_state(
raw: &str,
log_subject: &str,
log: &Logger,
) -> DesktopState {
match serde_json::from_str::<DesktopState>(raw) {
Ok(state) => state,
Err(error) => {
log(&format!(
"failed to parse {log_subject}: {error}. resetting state semantics"
));
DesktopState::default()
}
}
}
fn read_cached_close_action_at_path(
state_path: &Path,
log: &Logger,
) -> Option<CloseAction> {
let raw = fs::read_to_string(state_path).ok()?;
let state = load_desktop_state(&raw, "desktop close behavior state", log);
state.close_action
}
fn write_cached_close_action_at_path(
action: Option<CloseAction>,
state_path: &Path,
log: &Logger,
) -> Result<(), String> {
// unchanged body, just `log` is now `&Logger`
// ...
Ok(())
}
// Public API stays generic and just passes a `&Logger` through:
pub(crate) fn read_cached_close_action<F>(
packaged_root_dir: Option<&Path>,
log: F,
) -> Option<CloseAction>
where
F: Fn(&str),
{
let state_path = crate::desktop_state::resolve_desktop_state_path(packaged_root_dir)?;
read_cached_close_action_at_path(&state_path, &log)
}
pub(crate) fn write_cached_close_action<F>(
action: Option<CloseAction>,
packaged_root_dir: Option<&Path>,
log: F,
) -> Result<(), String>
where
F: Fn(&str),
{
let Some(state_path) = crate::desktop_state::resolve_desktop_state_path(packaged_root_dir)
else {
log("close behavior state path is unavailable; skipping close action persistence");
return Ok(());
};
write_cached_close_action_at_path(action, &state_path, &log)
}
```
Tests can keep using `noop_log` as `&Logger` with no behavior change. This removes generic parameters from internal helpers and makes signatures more readable.
### 2. Extract a tiny reusable atomic JSON writer
`save_desktop_state` mixes state semantics with low-level atomic write details. You can keep all behavior but move the IO pattern into a small helper, which also makes future reuse easier:
```rust
fn atomic_write_json(path: &Path, value: &impl Serialize, what: &str) -> Result<(), String> {
if let Some(parent_dir) = path.parent() {
fs::create_dir_all(parent_dir).map_err(|error| {
format!("Failed to create {what} directory {}: {}", parent_dir.display(), error)
})?;
}
let serialized = serde_json::to_string_pretty(value)
.map_err(|error| format!("Failed to serialize {what}: {error}"))?;
let tmp_name = format!(
"{}.tmp",
path.file_name()
.map(|value| value.to_string_lossy())
.unwrap_or_default()
);
let tmp_path = path.with_file_name(tmp_name);
let mut file = fs::File::create(&tmp_path).map_err(|error| {
format!(
"Failed to create temporary {what} file {}: {}",
tmp_path.display(),
error
)
})?;
file.write_all(serialized.as_bytes())
.and_then(|_| file.sync_all())
.map_err(|error| {
format!(
"Failed to write temporary {what} file {}: {}",
tmp_path.display(),
error
)
})?;
fs::rename(&tmp_path, path).map_err(|error| {
format!(
"Failed to atomically replace {what} file {}: {}",
path.display(),
error
)
})
}
fn save_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
atomic_write_json(path, state, "close behavior state")
}
```
This keeps all existing error messages structurally the same (only `what` is parameterized), preserves atomic behavior, and makes the state-related code (`state.close_action = action`) clearly separate from IO mechanics, which should address part of the “responsibility mix” concern without changing functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src-tauri/src/close_behavior.rs" line_range="8" />
<code_context>
+pub(crate) const CLOSE_ACTION_TRAY: &str = "tray";
+pub(crate) const CLOSE_ACTION_EXIT: &str = "exit";
+
+type Logger<'a> = dyn Fn(&str) + 'a;
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the logging and write helpers by using a generic `Fn(&str)` logger instead of a `Logger` trait object and by specializing the atomic write function to desktop state only, keeping behavior but making the code more direct and focused.
You can reduce the complexity without changing behavior by (1) dropping the `Logger` trait object and (2) specializing `atomic_write_json` to desktop state.
### 1. Remove `Logger` trait object and use generics consistently
You’re already generic at the public API; you can carry that through to the internals and drop the `type Logger` indirection and `&Logger` references:
```rust
// remove
- type Logger<'a> = dyn Fn(&str) + 'a;
- fn load_desktop_state(raw: &str, log_subject: &str, log: &Logger<'_>) -> DesktopState {
+ fn load_desktop_state<F>(raw: &str, log_subject: &str, log: &F) -> DesktopState
+ where
+ F: Fn(&str),
+ {
match serde_json::from_str::<DesktopState>(raw) {
Ok(state) => state,
Err(error) => {
log(&format!(
"failed to parse {log_subject}: {error}. resetting state semantics"
));
DesktopState::default()
}
}
}
- fn read_cached_close_action_at_path(state_path: &Path, log: &Logger<'_>) -> Option<CloseAction> {
+ fn read_cached_close_action_at_path<F>(state_path: &Path, log: &F) -> Option<CloseAction>
+ where
+ F: Fn(&str),
+ {
let raw = fs::read_to_string(state_path).ok()?;
let state = load_desktop_state(&raw, "desktop close behavior state", log);
state.close_action
}
- fn write_cached_close_action_at_path(
- action: Option<CloseAction>,
- state_path: &Path,
- log: &Logger<'_>,
- ) -> Result<(), String> {
+ fn write_cached_close_action_at_path<F>(
+ action: Option<CloseAction>,
+ state_path: &Path,
+ log: &F,
+ ) -> Result<(), String>
+ where
+ F: Fn(&str),
+ {
let mut state = match fs::read_to_string(state_path) {
Ok(raw) => load_desktop_state(
&raw,
&format!("close behavior state {}", state_path.display()),
log,
),
// ...
};
// ...
}
```
Call sites (including tests) keep using `&noop_log` or `&log` unchanged, but the logging path is now a straightforward generic `Fn(&str)` with no trait object alias or lifetime gymnastics.
### 2. Specialize `atomic_write_json` for desktop state
Since this helper is only used for the desktop state, you can narrow its purpose and fold the `what` string into the messages. This makes the write path easier to follow:
```rust
- fn atomic_write_json(path: &Path, value: &impl Serialize, what: &str) -> Result<(), String> {
+ fn atomic_write_desktop_state(path: &Path, value: &DesktopState) -> Result<(), String> {
if let Some(parent_dir) = path.parent() {
fs::create_dir_all(parent_dir).map_err(|error| {
format!(
- "Failed to create {what} directory {}: {}",
+ "Failed to create desktop state directory {}: {}",
parent_dir.display(),
error
)
})?;
}
let serialized = serde_json::to_string_pretty(value)
- .map_err(|error| format!("Failed to serialize {what}: {error}"))?;
+ .map_err(|error| format!("Failed to serialize desktop state: {error}"))?;
// ...
let mut file = fs::File::create(&tmp_path).map_err(|error| {
format!(
- "Failed to create temporary {what} file {}: {}",
+ "Failed to create temporary desktop state file {}: {}",
tmp_path.display(),
error
)
})?;
// ...
fs::rename(&tmp_path, path).map_err(|error| {
format!(
- "Failed to atomically replace {what} file {}: {}",
+ "Failed to atomically replace desktop state file {}: {}",
path.display(),
error
)
})
}
- fn save_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
- atomic_write_json(path, state, "close behavior state")
- }
+ fn save_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
+ atomic_write_desktop_state(path, state)
+ }
```
This keeps behavior and messages effectively the same, but removes generic `Serialize` and the generic `what` concept, so the write path is clearly and solely about the desktop state.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src-tauri/src/close_behavior.rs" line_range="33" />
<code_context>
+ })
+}
+
+#[derive(Debug, Default, Serialize, Deserialize)]
+struct DesktopState {
+ #[serde(
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying this module by removing the custom `DesktopState`/serde layer, collapsing the read/write helpers, and using a generic atomic JSON writer while preserving all existing behaviors.
You can keep the same behavior while cutting a lot of the bespoke machinery by:
1. Dropping `DesktopState` + custom serde
2. Collapsing the read/write layering
3. Generalizing the atomic write helper
### 1. Replace `DesktopState` + custom deserialization with `serde_json::Value`
All of this:
- `DesktopState`
- `deserialize_close_action_option`
- `load_desktop_state`
- `rest: Map<String, Value>`
exists mainly to preserve unknown fields and tolerate invalid `closeActionOnWindowClose` values. You can do that directly on `serde_json::Value`:
```rust
pub(crate) fn parse_close_action(raw: &str) -> Option<CloseAction> {
match raw {
CLOSE_ACTION_TRAY => Some(CloseAction::Tray),
CLOSE_ACTION_EXIT => Some(CloseAction::Exit),
_ => None,
}
}
fn load_state_value<F>(raw: &str, log_subject: &str, log: &F) -> serde_json::Value
where
F: Fn(&str),
{
match serde_json::from_str::<serde_json::Value>(raw) {
Ok(serde_json::Value::Object(map)) => serde_json::Value::Object(map),
Ok(_) | Err(_) => {
log(&format!(
"failed to parse {log_subject}: resetting state semantics"
));
serde_json::json!({})
}
}
}
```
Then `read_cached_close_action_at_path` becomes:
```rust
fn read_cached_close_action_at_path<F>(state_path: &Path, log: &F) -> Option<CloseAction>
where
F: Fn(&str),
{
let raw = fs::read_to_string(state_path).ok()?;
let state = load_state_value(&raw, "desktop close behavior state", log);
state
.get("closeActionOnWindowClose")
.and_then(|v| v.as_str())
.and_then(parse_close_action)
}
```
This preserves:
- arbitrary other fields (you never drop them)
- invalid value → `None`
- non‑string value → `None`
- malformed JSON or non‑object root → log + treat as empty object
### 2. Simplify the write path and remove `*_at_path` and `save_desktop_state`
You can inline `save_desktop_state` and remove the layered `*_at_path` variant by working directly on `Value`:
```rust
fn write_cached_close_action_at_path<F>(
action: Option<CloseAction>,
state_path: &Path,
log: &F,
) -> Result<(), String>
where
F: Fn(&str),
{
let mut state = match fs::read_to_string(state_path) {
Ok(raw) => load_state_value(
&raw,
&format!("close behavior state {}", state_path.display()),
log,
),
Err(error) if error.kind() == std::io::ErrorKind::NotFound => serde_json::json!({}),
Err(error) => {
return Err(format!(
"Failed to read close behavior state {}: {}",
state_path.display(),
error
));
}
};
let obj = state.as_object_mut().expect("load_state_value guarantees object");
match action {
Some(CloseAction::Tray) => {
obj.insert(
"closeActionOnWindowClose".to_string(),
serde_json::json!("tray"),
);
}
Some(CloseAction::Exit) => {
obj.insert(
"closeActionOnWindowClose".to_string(),
serde_json::json!("exit"),
);
}
None => {
obj.remove("closeActionOnWindowClose");
}
}
atomic_write_json(state_path, &state)
}
```
The public entry point can stay basically as‑is, but now calls the simplified helper:
```rust
pub(crate) fn write_cached_close_action<F>(
action: Option<CloseAction>,
packaged_root_dir: Option<&Path>,
log: F,
) -> Result<(), String>
where
F: Fn(&str),
{
let Some(state_path) = crate::desktop_state::resolve_desktop_state_path(packaged_root_dir)
else {
log("close behavior state path is unavailable; skipping close action persistence");
return Ok(());
};
write_cached_close_action_at_path(action, &state_path, &log)
}
```
This keeps:
- preserving unrelated fields
- resetting malformed state to object with only `closeActionOnWindowClose`
- `None` removing only the close‑action field
…without custom serde types or a separate `DesktopState` struct.
### 3. Generalize the atomic write helper
You can keep your existing atomic write semantics but make it reusable and less `DesktopState`‑specific:
```rust
fn atomic_write_json(path: &Path, value: &impl Serialize) -> Result<(), String> {
if let Some(parent_dir) = path.parent() {
fs::create_dir_all(parent_dir).map_err(|error| {
format!(
"Failed to create desktop state directory {}: {}",
parent_dir.display(),
error
)
})?;
}
let serialized =
serde_json::to_string_pretty(value).map_err(|error| {
format!("Failed to serialize desktop state: {error}")
})?;
let tmp_name = format!(
"{}.tmp",
path.file_name()
.map(|value| value.to_string_lossy())
.unwrap_or_default()
);
let tmp_path = path.with_file_name(tmp_name);
let mut file = fs::File::create(&tmp_path).map_err(|error| {
format!(
"Failed to create temporary desktop state file {}: {}",
tmp_path.display(),
error
)
})?;
file.write_all(serialized.as_bytes())
.and_then(|_| file.sync_all())
.map_err(|error| {
format!(
"Failed to write temporary desktop state file {}: {}",
tmp_path.display(),
error
)
})?;
fs::rename(&tmp_path, path).map_err(|error| {
format!(
"Failed to atomically replace desktop state file {}: {}",
path.display(),
error
)
})
}
```
Then you can remove `atomic_write_desktop_state` and `save_desktop_state` entirely and just use `atomic_write_json`.
These tweaks keep all the current semantics (including logging, malformed‑file handling, and preservation of unknown fields) but significantly reduce the amount of custom serde and layering you have to reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }) | ||
| } | ||
|
|
||
| #[derive(Debug, Default, Serialize, Deserialize)] |
There was a problem hiding this comment.
issue (complexity): Consider simplifying this module by removing the custom DesktopState/serde layer, collapsing the read/write helpers, and using a generic atomic JSON writer while preserving all existing behaviors.
You can keep the same behavior while cutting a lot of the bespoke machinery by:
- Dropping
DesktopState+ custom serde - Collapsing the read/write layering
- Generalizing the atomic write helper
1. Replace DesktopState + custom deserialization with serde_json::Value
All of this:
DesktopStatedeserialize_close_action_optionload_desktop_staterest: Map<String, Value>
exists mainly to preserve unknown fields and tolerate invalid closeActionOnWindowClose values. You can do that directly on serde_json::Value:
pub(crate) fn parse_close_action(raw: &str) -> Option<CloseAction> {
match raw {
CLOSE_ACTION_TRAY => Some(CloseAction::Tray),
CLOSE_ACTION_EXIT => Some(CloseAction::Exit),
_ => None,
}
}
fn load_state_value<F>(raw: &str, log_subject: &str, log: &F) -> serde_json::Value
where
F: Fn(&str),
{
match serde_json::from_str::<serde_json::Value>(raw) {
Ok(serde_json::Value::Object(map)) => serde_json::Value::Object(map),
Ok(_) | Err(_) => {
log(&format!(
"failed to parse {log_subject}: resetting state semantics"
));
serde_json::json!({})
}
}
}Then read_cached_close_action_at_path becomes:
fn read_cached_close_action_at_path<F>(state_path: &Path, log: &F) -> Option<CloseAction>
where
F: Fn(&str),
{
let raw = fs::read_to_string(state_path).ok()?;
let state = load_state_value(&raw, "desktop close behavior state", log);
state
.get("closeActionOnWindowClose")
.and_then(|v| v.as_str())
.and_then(parse_close_action)
}This preserves:
- arbitrary other fields (you never drop them)
- invalid value →
None - non‑string value →
None - malformed JSON or non‑object root → log + treat as empty object
2. Simplify the write path and remove *_at_path and save_desktop_state
You can inline save_desktop_state and remove the layered *_at_path variant by working directly on Value:
fn write_cached_close_action_at_path<F>(
action: Option<CloseAction>,
state_path: &Path,
log: &F,
) -> Result<(), String>
where
F: Fn(&str),
{
let mut state = match fs::read_to_string(state_path) {
Ok(raw) => load_state_value(
&raw,
&format!("close behavior state {}", state_path.display()),
log,
),
Err(error) if error.kind() == std::io::ErrorKind::NotFound => serde_json::json!({}),
Err(error) => {
return Err(format!(
"Failed to read close behavior state {}: {}",
state_path.display(),
error
));
}
};
let obj = state.as_object_mut().expect("load_state_value guarantees object");
match action {
Some(CloseAction::Tray) => {
obj.insert(
"closeActionOnWindowClose".to_string(),
serde_json::json!("tray"),
);
}
Some(CloseAction::Exit) => {
obj.insert(
"closeActionOnWindowClose".to_string(),
serde_json::json!("exit"),
);
}
None => {
obj.remove("closeActionOnWindowClose");
}
}
atomic_write_json(state_path, &state)
}The public entry point can stay basically as‑is, but now calls the simplified helper:
pub(crate) fn write_cached_close_action<F>(
action: Option<CloseAction>,
packaged_root_dir: Option<&Path>,
log: F,
) -> Result<(), String>
where
F: Fn(&str),
{
let Some(state_path) = crate::desktop_state::resolve_desktop_state_path(packaged_root_dir)
else {
log("close behavior state path is unavailable; skipping close action persistence");
return Ok(());
};
write_cached_close_action_at_path(action, &state_path, &log)
}This keeps:
- preserving unrelated fields
- resetting malformed state to object with only
closeActionOnWindowClose Noneremoving only the close‑action field
…without custom serde types or a separate DesktopState struct.
3. Generalize the atomic write helper
You can keep your existing atomic write semantics but make it reusable and less DesktopState‑specific:
fn atomic_write_json(path: &Path, value: &impl Serialize) -> Result<(), String> {
if let Some(parent_dir) = path.parent() {
fs::create_dir_all(parent_dir).map_err(|error| {
format!(
"Failed to create desktop state directory {}: {}",
parent_dir.display(),
error
)
})?;
}
let serialized =
serde_json::to_string_pretty(value).map_err(|error| {
format!("Failed to serialize desktop state: {error}")
})?;
let tmp_name = format!(
"{}.tmp",
path.file_name()
.map(|value| value.to_string_lossy())
.unwrap_or_default()
);
let tmp_path = path.with_file_name(tmp_name);
let mut file = fs::File::create(&tmp_path).map_err(|error| {
format!(
"Failed to create temporary desktop state file {}: {}",
tmp_path.display(),
error
)
})?;
file.write_all(serialized.as_bytes())
.and_then(|_| file.sync_all())
.map_err(|error| {
format!(
"Failed to write temporary desktop state file {}: {}",
tmp_path.display(),
error
)
})?;
fs::rename(&tmp_path, path).map_err(|error| {
format!(
"Failed to atomically replace desktop state file {}: {}",
path.display(),
error
)
})
}Then you can remove atomic_write_desktop_state and save_desktop_state entirely and just use atomic_write_json.
These tweaks keep all the current semantics (including logging, malformed‑file handling, and preservation of unknown fields) but significantly reduce the amount of custom serde and layering you have to reason about.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
desktop_bridge_submit_close_prompt, a failure to persist the remembered close action (write_cached_close_actionerror or missing state path) causes the whole close operation to fail; consider treating persistence as best-effort (log and proceed with the selected action) so users aren’t blocked from closing/minimizing the app when the state file is unavailable or unwritable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `desktop_bridge_submit_close_prompt`, a failure to persist the remembered close action (`write_cached_close_action` error or missing state path) causes the whole close operation to fail; consider treating persistence as best-effort (log and proceed with the selected action) so users aren’t blocked from closing/minimizing the app when the state file is unavailable or unwritable.
## Individual Comments
### Comment 1
<location path="src-tauri/src/close_behavior.rs" line_range="90" />
<code_context>
+ state.close_action
+}
+
+fn atomic_write_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
+ if let Some(parent_dir) = path.parent() {
+ fs::create_dir_all(parent_dir).map_err(|error| {
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the redundant `save_desktop_state` wrapper and replacing the repeated `F: Fn(&str)` generics with a shared logger type alias to simplify the module’s control flow and signatures.
You can trim some of the generic/plumbing complexity without changing behavior or the JSON format.
### 1. Drop the `save_desktop_state` wrapper
`save_desktop_state` is a thin wrapper over `atomic_write_desktop_state` and doesn’t add semantics. Calling the atomic function directly simplifies the control flow.
```rust
fn atomic_write_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
// unchanged
}
// remove this function:
// fn save_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
// atomic_write_desktop_state(path, state)
// }
fn write_cached_close_action_at_path<F>(
action: Option<CloseAction>,
state_path: &Path,
log: &F,
) -> Result<(), String>
where
F: Fn(&str),
{
let mut state = match fs::read_to_string(state_path) {
// unchanged
};
state.close_action = action;
atomic_write_desktop_state(state_path, &state)?;
Ok(())
}
```
This removes one extra layer in the write path without affecting tests or behavior.
### 2. Replace generic `F: Fn(&str)` with a simple logger type
The generic `F: Fn(&str)` on multiple functions increases surface area. You can keep the same call sites and testability, but avoid propagating generics by using a trait object alias:
```rust
type Logger<'a> = &'a dyn Fn(&str);
fn load_desktop_state(raw: &str, log_subject: &str, log: Logger<'_>) -> DesktopState {
match serde_json::from_str::<DesktopState>(raw) {
Ok(state) => state,
Err(error) => {
log(&format!(
"failed to parse {log_subject}: {error}. resetting state semantics"
));
DesktopState::default()
}
}
}
pub(crate) fn read_cached_close_action(
packaged_root_dir: Option<&Path>,
log: Logger<'_>,
) -> Option<CloseAction> {
let state_path = crate::desktop_state::resolve_desktop_state_path(packaged_root_dir)?;
read_cached_close_action_at_path(&state_path, log)
}
fn read_cached_close_action_at_path(state_path: &Path, log: Logger<'_>) -> Option<CloseAction> {
let raw = fs::read_to_string(state_path).ok()?;
let state = load_desktop_state(&raw, "desktop close behavior state", log);
state.close_action
}
pub(crate) fn write_cached_close_action(
action: Option<CloseAction>,
packaged_root_dir: Option<&Path>,
log: Logger<'_>,
) -> Result<(), String> {
let Some(state_path) = crate::desktop_state::resolve_desktop_state_path(packaged_root_dir)
else {
let message =
"close behavior state path is unavailable; skipping close action persistence";
log(message);
return Err(message.to_string());
};
write_cached_close_action_at_path(action, &state_path, log)
}
fn write_cached_close_action_at_path(
action: Option<CloseAction>,
state_path: &Path,
log: Logger<'_>,
) -> Result<(), String> {
// body unchanged except for log type
}
```
Tests can continue to pass `&noop_log` or closures by reference, and production code still passes `append_desktop_log`, but the module no longer needs a generic parameter on each function.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| state.close_action | ||
| } | ||
|
|
||
| fn atomic_write_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> { |
There was a problem hiding this comment.
issue (complexity): Consider inlining the redundant save_desktop_state wrapper and replacing the repeated F: Fn(&str) generics with a shared logger type alias to simplify the module’s control flow and signatures.
You can trim some of the generic/plumbing complexity without changing behavior or the JSON format.
1. Drop the save_desktop_state wrapper
save_desktop_state is a thin wrapper over atomic_write_desktop_state and doesn’t add semantics. Calling the atomic function directly simplifies the control flow.
fn atomic_write_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
// unchanged
}
// remove this function:
// fn save_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
// atomic_write_desktop_state(path, state)
// }
fn write_cached_close_action_at_path<F>(
action: Option<CloseAction>,
state_path: &Path,
log: &F,
) -> Result<(), String>
where
F: Fn(&str),
{
let mut state = match fs::read_to_string(state_path) {
// unchanged
};
state.close_action = action;
atomic_write_desktop_state(state_path, &state)?;
Ok(())
}This removes one extra layer in the write path without affecting tests or behavior.
2. Replace generic F: Fn(&str) with a simple logger type
The generic F: Fn(&str) on multiple functions increases surface area. You can keep the same call sites and testability, but avoid propagating generics by using a trait object alias:
type Logger<'a> = &'a dyn Fn(&str);
fn load_desktop_state(raw: &str, log_subject: &str, log: Logger<'_>) -> DesktopState {
match serde_json::from_str::<DesktopState>(raw) {
Ok(state) => state,
Err(error) => {
log(&format!(
"failed to parse {log_subject}: {error}. resetting state semantics"
));
DesktopState::default()
}
}
}
pub(crate) fn read_cached_close_action(
packaged_root_dir: Option<&Path>,
log: Logger<'_>,
) -> Option<CloseAction> {
let state_path = crate::desktop_state::resolve_desktop_state_path(packaged_root_dir)?;
read_cached_close_action_at_path(&state_path, log)
}
fn read_cached_close_action_at_path(state_path: &Path, log: Logger<'_>) -> Option<CloseAction> {
let raw = fs::read_to_string(state_path).ok()?;
let state = load_desktop_state(&raw, "desktop close behavior state", log);
state.close_action
}
pub(crate) fn write_cached_close_action(
action: Option<CloseAction>,
packaged_root_dir: Option<&Path>,
log: Logger<'_>,
) -> Result<(), String> {
let Some(state_path) = crate::desktop_state::resolve_desktop_state_path(packaged_root_dir)
else {
let message =
"close behavior state path is unavailable; skipping close action persistence";
log(message);
return Err(message.to_string());
};
write_cached_close_action_at_path(action, &state_path, log)
}
fn write_cached_close_action_at_path(
action: Option<CloseAction>,
state_path: &Path,
log: Logger<'_>,
) -> Result<(), String> {
// body unchanged except for log type
}Tests can continue to pass &noop_log or closures by reference, and production code still passes append_desktop_log, but the module no longer needs a generic parameter on each function.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
desktop_bridge_submit_close_prompt, a failure to persist the remembered close action aborts the selected close/exit behavior; consider treating persistence failures as non-fatal (log and continue) so users aren’t blocked from closing the app when the state file is unavailable. - The
CloseRequestedwindow event handler synchronously reads the desktop state from disk viaread_cached_close_action; you may want to cache the close behavior in memory (e.g., inBackendState) and update it only when the prompt is confirmed to avoid filesystem I/O in the hot path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `desktop_bridge_submit_close_prompt`, a failure to persist the remembered close action aborts the selected close/exit behavior; consider treating persistence failures as non-fatal (log and continue) so users aren’t blocked from closing the app when the state file is unavailable.
- The `CloseRequested` window event handler synchronously reads the desktop state from disk via `read_cached_close_action`; you may want to cache the close behavior in memory (e.g., in `BackendState`) and update it only when the prompt is confirmed to avoid filesystem I/O in the hot path.
## Individual Comments
### Comment 1
<location path="src-tauri/src/close_behavior.rs" line_range="136" />
<code_context>
+ })
+}
+
+fn save_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
+ atomic_write_desktop_state(path, state)
+}
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the close behavior module by inlining trivial wrappers, separating strict parsing from lenient reset-on-error semantics, and centralizing repeated IO error formatting.
You can shave off a bit of indirection without changing behavior by tightening a few spots:
### 1. Inline `save_desktop_state` to remove a trivial layer
`save_desktop_state` is a thin wrapper over `atomic_write_desktop_state` and doesn’t add semantics.
```rust
// Before
fn save_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
atomic_write_desktop_state(path, state)
}
fn write_cached_close_action_at_path<F>(
action: Option<CloseAction>,
state_path: &Path,
log: &F,
) -> Result<(), String>
where
F: Fn(&str),
{
// ...
save_desktop_state(state_path, &state)?;
Ok(())
}
```
You can call `atomic_write_desktop_state` directly and drop the extra function:
```rust
fn write_cached_close_action_at_path<F>(
action: Option<CloseAction>,
state_path: &Path,
log: &F,
) -> Result<(), String>
where
F: Fn(&str),
{
let mut state = match fs::read_to_string(state_path) {
Ok(raw) => load_desktop_state(
&raw,
&format!("close behavior state {}", state_path.display()),
log,
),
Err(error) if error.kind() == std::io::ErrorKind::NotFound => DesktopState::default(),
Err(error) => {
return Err(format!(
"Failed to read close behavior state {}: {}",
state_path.display(),
error
));
}
};
state.close_action = action;
atomic_write_desktop_state(state_path, &state)?;
Ok(())
}
```
This keeps all behavior and tests intact while removing one level of indirection.
---
### 2. Separate strict vs lenient state parsing to make reset semantics explicit
`load_desktop_state` has the “on any error, reset state” policy baked in. That’s fine, but it’s used in both read and write paths, which obscures where the reset decision is actually made.
You can factor out a strict parser and make `load_desktop_state` a small, lenient wrapper. Call sites that want the “reset semantics” keep calling `load_desktop_state`; if you later need strict behavior, it’s available without new logic:
```rust
fn parse_desktop_state(raw: &str) -> Result<DesktopState, serde_json::Error> {
serde_json::from_str::<DesktopState>(raw)
}
fn load_desktop_state<F>(raw: &str, log_subject: &str, log: &F) -> DesktopState
where
F: Fn(&str),
{
match parse_desktop_state(raw) {
Ok(state) => state,
Err(error) => {
log(&format!(
"failed to parse {log_subject}: {error}. resetting state semantics"
));
DesktopState::default()
}
}
}
```
Call sites remain the same, but the “reset on parse error” policy is now encoded in a tiny wrapper rather than in the only parsing function, which reduces cognitive load when following the read/write flows.
---
### 3. Centralize repeated error string formatting
The repeated `"Failed to ... desktop state ..."` strings add noise and couple formatting to multiple call sites. A tiny helper keeps this DRY and makes the main logic easier to scan:
```rust
fn desktop_state_io_error(context: &str, path: &Path, error: &std::io::Error) -> String {
format!("{context} {}: {error}", path.display())
}
```
Usage:
```rust
if let Some(parent_dir) = path.parent() {
fs::create_dir_all(parent_dir)
.map_err(|error| desktop_state_io_error("Failed to create desktop state directory", parent_dir, &error))?;
}
let mut file = fs::File::create(&tmp_path)
.map_err(|error| desktop_state_io_error("Failed to create temporary desktop state file", &tmp_path, &error))?;
```
This keeps behavior and messages identical while trimming repetition.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/window/close_confirm.rs" line_range="23" />
<code_context>
+ (CLOSE_CONFIRM_WINDOW_WIDTH, CLOSE_CONFIRM_WINDOW_HEIGHT)
+}
+
+fn handle_existing_window_operation_result<F>(
+ operation_result: Result<(), String>,
+ operation: &str,
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the close-confirm window error handling into `show_close_confirm_window` to avoid a single-use helper function and keep all related logic in one place.
You can simplify this by inlining the error-handling instead of using `handle_existing_window_operation_result`, which is only used in one place and adds indirection.
You can keep the same error message format and logging behavior locally in `show_close_confirm_window`, e.g.:
```rust
pub(crate) fn show_close_confirm_window<F>(
app_handle: &AppHandle,
default_shell_locale: &'static str,
log: F,
) -> Result<(), String>
where
F: Fn(&str),
{
if let Some(window) = app_handle.get_webview_window(CLOSE_CONFIRM_WINDOW_LABEL) {
let log_and_err = |operation: &str, error: String| -> String {
let message =
format!("failed to {operation} close confirm window: {error}");
log(&message);
message
};
if let Err(e) = window.unminimize() {
return Err(log_and_err("unminimize", e.to_string()));
}
if let Err(e) = window.show() {
return Err(log_and_err("show", e.to_string()));
}
if let Err(e) = window.set_focus() {
return Err(log_and_err("focus", e.to_string()));
}
return Ok(());
}
let locale = crate::shell_locale::resolve_shell_locale(
default_shell_locale,
crate::runtime_paths::default_packaged_root_dir(),
);
let url = build_close_confirm_url(locale);
let (width, height) = close_confirm_window_size();
WebviewWindowBuilder::new(app_handle, CLOSE_CONFIRM_WINDOW_LABEL, url)
.title("AstrBot")
.inner_size(width, height)
.resizable(false)
.maximizable(false)
.minimizable(false)
.visible(true)
.center()
.build()
.map(|_| ())
.map_err(|error| format!("Failed to create close confirm window: {error}"))
}
```
With this, you can drop `handle_existing_window_operation_result` and its dedicated test. The logic remains in one place, the error strings stay identical, and the control flow is easier to scan.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }) | ||
| } | ||
|
|
||
| fn save_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> { |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the close behavior module by inlining trivial wrappers, separating strict parsing from lenient reset-on-error semantics, and centralizing repeated IO error formatting.
You can shave off a bit of indirection without changing behavior by tightening a few spots:
1. Inline save_desktop_state to remove a trivial layer
save_desktop_state is a thin wrapper over atomic_write_desktop_state and doesn’t add semantics.
// Before
fn save_desktop_state(path: &Path, state: &DesktopState) -> Result<(), String> {
atomic_write_desktop_state(path, state)
}
fn write_cached_close_action_at_path<F>(
action: Option<CloseAction>,
state_path: &Path,
log: &F,
) -> Result<(), String>
where
F: Fn(&str),
{
// ...
save_desktop_state(state_path, &state)?;
Ok(())
}You can call atomic_write_desktop_state directly and drop the extra function:
fn write_cached_close_action_at_path<F>(
action: Option<CloseAction>,
state_path: &Path,
log: &F,
) -> Result<(), String>
where
F: Fn(&str),
{
let mut state = match fs::read_to_string(state_path) {
Ok(raw) => load_desktop_state(
&raw,
&format!("close behavior state {}", state_path.display()),
log,
),
Err(error) if error.kind() == std::io::ErrorKind::NotFound => DesktopState::default(),
Err(error) => {
return Err(format!(
"Failed to read close behavior state {}: {}",
state_path.display(),
error
));
}
};
state.close_action = action;
atomic_write_desktop_state(state_path, &state)?;
Ok(())
}This keeps all behavior and tests intact while removing one level of indirection.
2. Separate strict vs lenient state parsing to make reset semantics explicit
load_desktop_state has the “on any error, reset state” policy baked in. That’s fine, but it’s used in both read and write paths, which obscures where the reset decision is actually made.
You can factor out a strict parser and make load_desktop_state a small, lenient wrapper. Call sites that want the “reset semantics” keep calling load_desktop_state; if you later need strict behavior, it’s available without new logic:
fn parse_desktop_state(raw: &str) -> Result<DesktopState, serde_json::Error> {
serde_json::from_str::<DesktopState>(raw)
}
fn load_desktop_state<F>(raw: &str, log_subject: &str, log: &F) -> DesktopState
where
F: Fn(&str),
{
match parse_desktop_state(raw) {
Ok(state) => state,
Err(error) => {
log(&format!(
"failed to parse {log_subject}: {error}. resetting state semantics"
));
DesktopState::default()
}
}
}Call sites remain the same, but the “reset on parse error” policy is now encoded in a tiny wrapper rather than in the only parsing function, which reduces cognitive load when following the read/write flows.
3. Centralize repeated error string formatting
The repeated "Failed to ... desktop state ..." strings add noise and couple formatting to multiple call sites. A tiny helper keeps this DRY and makes the main logic easier to scan:
fn desktop_state_io_error(context: &str, path: &Path, error: &std::io::Error) -> String {
format!("{context} {}: {error}", path.display())
}Usage:
if let Some(parent_dir) = path.parent() {
fs::create_dir_all(parent_dir)
.map_err(|error| desktop_state_io_error("Failed to create desktop state directory", parent_dir, &error))?;
}
let mut file = fs::File::create(&tmp_path)
.map_err(|error| desktop_state_io_error("Failed to create temporary desktop state file", &tmp_path, &error))?;This keeps behavior and messages identical while trimming repetition.
| (CLOSE_CONFIRM_WINDOW_WIDTH, CLOSE_CONFIRM_WINDOW_HEIGHT) | ||
| } | ||
|
|
||
| fn handle_existing_window_operation_result<F>( |
There was a problem hiding this comment.
issue (complexity): Consider inlining the close-confirm window error handling into show_close_confirm_window to avoid a single-use helper function and keep all related logic in one place.
You can simplify this by inlining the error-handling instead of using handle_existing_window_operation_result, which is only used in one place and adds indirection.
You can keep the same error message format and logging behavior locally in show_close_confirm_window, e.g.:
pub(crate) fn show_close_confirm_window<F>(
app_handle: &AppHandle,
default_shell_locale: &'static str,
log: F,
) -> Result<(), String>
where
F: Fn(&str),
{
if let Some(window) = app_handle.get_webview_window(CLOSE_CONFIRM_WINDOW_LABEL) {
let log_and_err = |operation: &str, error: String| -> String {
let message =
format!("failed to {operation} close confirm window: {error}");
log(&message);
message
};
if let Err(e) = window.unminimize() {
return Err(log_and_err("unminimize", e.to_string()));
}
if let Err(e) = window.show() {
return Err(log_and_err("show", e.to_string()));
}
if let Err(e) = window.set_focus() {
return Err(log_and_err("focus", e.to_string()));
}
return Ok(());
}
let locale = crate::shell_locale::resolve_shell_locale(
default_shell_locale,
crate::runtime_paths::default_packaged_root_dir(),
);
let url = build_close_confirm_url(locale);
let (width, height) = close_confirm_window_size();
WebviewWindowBuilder::new(app_handle, CLOSE_CONFIRM_WINDOW_LABEL, url)
.title("AstrBot")
.inner_size(width, height)
.resizable(false)
.maximizable(false)
.minimizable(false)
.visible(true)
.center()
.build()
.map(|_| ())
.map_err(|error| format!("Failed to create close confirm window: {error}"))
}With this, you can drop handle_existing_window_operation_result and its dedicated test. The logic remains in one place, the error strings stay identical, and the control flow is easier to scan.
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
Test Plan
Summary by Sourcery
Add a dedicated close confirmation flow to the desktop app that lets users choose and persist their preferred close behavior (minimize to tray or exit) and wire this flow through Tauri windows, bridge commands, and lifecycle handling.
New Features:
Enhancements:
Tests: