fix: handle dashboard cmd config BOM#145
Merged
zouyonghe merged 1 commit intoJun 30, 2026
Merged
Conversation
![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 2 issues, and left some high level feedback:
- In
read_cmd_config_file, all filesystem read errors are treated as transient; consider usingio::ErrorKindto distinguish truly transient cases (e.g.,Interrupted) from persistent ones (e.g.,NotFound,PermissionDenied) to avoid unnecessary retries.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `read_cmd_config_file`, all filesystem read errors are treated as transient; consider using `io::ErrorKind` to distinguish truly transient cases (e.g., `Interrupted`) from persistent ones (e.g., `NotFound`, `PermissionDenied`) to avoid unnecessary retries.
## Individual Comments
### Comment 1
<location path="src-tauri/src/backend/launch.rs" line_range="267-270" />
<code_context>
+ is_transient: true,
+ })?;
+ let raw = raw.strip_prefix('\u{feff}').unwrap_or(&raw);
+ serde_json::from_str(raw).map_err(|error| CmdConfigReadAttemptError {
+ action: "failed to parse cmd_config",
+ detail: error.to_string(),
+ is_transient: raw.trim().is_empty() || error.is_eof(),
+ })
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider treating whitespace-only configs as non-transient instead of retrying them
Currently `is_transient` is `raw.trim().is_empty() || error.is_eof()`, so a file with only whitespace (e.g. "\n\n") is treated as transient and will consume all retries before defaults are used. That seems more like a permanent misconfig than a transient condition. Consider limiting the transient case to exactly empty input (`raw.is_empty()`) and/or EOF errors, and treating any non-empty-but-invalid JSON (including whitespace-only) as non-transient.
Suggested implementation:
```rust
let raw = raw.strip_prefix('\u{feff}').unwrap_or(&raw);
```
```rust
serde_json::from_str(raw).map_err(|error| CmdConfigReadAttemptError {
action: "failed to parse cmd_config",
detail: error.to_string(),
// Treat only truly empty input (after BOM stripping) or EOF as transient.
// Whitespace-only or other invalid JSON is considered a permanent misconfiguration.
is_transient: raw.is_empty() || error.is_eof(),
})
```
If other call sites or tests rely on whitespace-only configs being treated as transient, they will need to be updated to expect non-transient behavior (i.e., fewer retries and earlier fallback to defaults) for such inputs.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/backend/launch.rs" line_range="225" />
<code_context>
CmdDashboardConfig::from_file_config(parsed, log)
}
+struct CmdConfigReadError {
+ action: &'static str,
+ detail: String,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the cmd config read path by using a single error type and inlining transient classification logic to reduce indirection.
You can keep all behavior (retry, transient classification, BOM handling, logging) while reducing indirection by:
1. **Drop the inner `CmdConfigReadAttemptError` type** and keep `is_transient` as a local concern in the retry loop.
2. **Use a single error struct (`CmdConfigReadError`) only at the top level** where logging happens.
3. **Inline transient/non‑transient classification** inside the retry helper.
A minimal refactor could look like this:
```rust
struct CmdConfigReadError {
action: &'static str,
detail: String,
}
fn read_cmd_config_file_with_retry(
config_path: &Path,
) -> Result<CmdConfigFile, CmdConfigReadError> {
let mut last_error: Option<(CmdConfigReadError, bool)> = None;
for attempt in 0..CMD_CONFIG_READ_RETRY_ATTEMPTS {
match read_cmd_config_file_once(config_path) {
Ok(config) => return Ok(config),
Err((error, is_transient)) => {
let should_retry = is_transient && attempt + 1 < CMD_CONFIG_READ_RETRY_ATTEMPTS;
last_error = Some((error, is_transient));
if should_retry {
thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
} else {
break;
}
}
}
}
let (error, _) = last_error.expect("at least one read attempt");
Err(error)
}
fn read_cmd_config_file_once(
config_path: &Path,
) -> Result<CmdConfigFile, (CmdConfigReadError, bool)> {
let raw = fs::read_to_string(config_path).map_err(|error| {
(
CmdConfigReadError {
action: "failed to read cmd_config",
detail: error.to_string(),
},
/* is_transient: */ true,
)
})?;
let raw = raw.strip_prefix('\u{feff}').unwrap_or(&raw);
serde_json::from_str(raw).map_err(|error| {
let is_transient = raw.trim().is_empty() || error.is_eof();
(
CmdConfigReadError {
action: "failed to parse cmd_config",
detail: error.to_string(),
},
is_transient,
)
})
}
```
The call site remains unchanged:
```rust
let parsed = match read_cmd_config_file_with_retry(&config_path) {
Ok(parsed) => parsed,
Err(error) => {
log(&format!(
"{} {}: {}",
error.action,
config_path.display(),
error.detail
));
return CmdDashboardConfig::default();
}
};
```
This keeps:
- The same retry semantics and delay.
- BOM stripping.
- Transient classification (empty file / EOF).
- Existing logging format.
But removes the extra `CmdConfigReadAttemptError` type and avoids passing `action` through multiple layers, making the flow easier to follow.
</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 introduces a retry mechanism for reading the dashboard configuration file to handle transient errors, adds support for stripping UTF-8 BOM prefixes, and implements an EnvVarGuard helper to manage environment variables during testing. The review feedback highlights two critical improvements: first, the retry loop should return immediately upon encountering non-transient errors to prevent redundant I/O operations; second, because Rust tests run in parallel, EnvVarGuard should utilize a static Mutex to synchronize environment variable modifications and prevent flaky test failures.
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.
Comment on lines
+233
to
+251
| let mut last_error = None; | ||
| for attempt in 0..CMD_CONFIG_READ_RETRY_ATTEMPTS { | ||
| match read_cmd_config_file(config_path) { | ||
| Ok(config) => return Ok(config), | ||
| Err(error) => { | ||
| let should_retry = | ||
| error.is_transient && attempt + 1 < CMD_CONFIG_READ_RETRY_ATTEMPTS; | ||
| last_error = Some(error); | ||
| if should_retry { | ||
| thread::sleep(CMD_CONFIG_READ_RETRY_DELAY); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| let error = last_error.expect("at least one read attempt"); | ||
| Err(CmdConfigReadError { | ||
| action: error.action, | ||
| detail: error.detail, | ||
| }) |
Contributor
There was a problem hiding this comment.
The retry loop does not break or return early when a non-transient error (such as a JSON syntax error) is encountered. Instead, it continues to retry immediately without sleeping for the remaining attempts, leading to redundant and inefficient I/O and parsing operations. We should return the error immediately if it is not transient.
for attempt in 0..CMD_CONFIG_READ_RETRY_ATTEMPTS {
match read_cmd_config_file(config_path) {
Ok(config) => return Ok(config),
Err(error) => {
let is_last_attempt = attempt + 1 == CMD_CONFIG_READ_RETRY_ATTEMPTS;
if !error.is_transient || is_last_attempt {
return Err(CmdConfigReadError {
action: error.action,
detail: error.detail,
});
}
thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
}
}
}
unreachable!()
Comment on lines
+227
to
+238
| struct EnvVarGuard { | ||
| key: &'static str, | ||
| previous: Option<String>, | ||
| } | ||
|
|
||
| impl EnvVarGuard { | ||
| fn clear(key: &'static str) -> Self { | ||
| let previous = std::env::var(key).ok(); | ||
| std::env::remove_var(key); | ||
| Self { key, previous } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Rust tests run in parallel by default. Since environment variables are process-global, modifying them concurrently in multiple tests without synchronization can cause race conditions and flaky test failures.
By adding a static Mutex and holding its guard inside EnvVarGuard, we can ensure that all tests utilizing EnvVarGuard are executed sequentially, preventing environment variable pollution and race conditions.
static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
struct EnvVarGuard {
key: &'static str,
previous: Option<String>,
_lock: std::sync::MutexGuard<'static, ()>,
}
impl EnvVarGuard {
fn clear(key: &'static str) -> Self {
let _lock = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner());
let previous = std::env::var(key).ok();
std::env::remove_var(key);
Self {
key,
previous,
_lock,
}
}
}
zouyonghe
force-pushed
the
fix/dashboard-cmd-config-bom
branch
from
791fa42 to
e9ce948
Compare
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When retrying cmd_config reads, consider treating a transient
NotFound(e.g., during atomic rewrite via temp+rename) as retriable as well, so dashboards don’t fall back to defaults while the file is briefly missing. - The parse retry logic in
read_cmd_config_file_once(empty raw or EOF with non-whitespace) is quite subtle; adding a short comment or extracting it into a clearly named helper would make the intended behavior around incomplete/whitespace-only files easier to understand and maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When retrying cmd_config reads, consider treating a transient `NotFound` (e.g., during atomic rewrite via temp+rename) as retriable as well, so dashboards don’t fall back to defaults while the file is briefly missing.
- The parse retry logic in `read_cmd_config_file_once` (empty raw or EOF with non-whitespace) is quite subtle; adding a short comment or extracting it into a clearly named helper would make the intended behavior around incomplete/whitespace-only files easier to understand and maintain.
## Individual Comments
### Comment 1
<location path="src-tauri/src/backend/launch.rs" line_range="226" />
<code_context>
CmdDashboardConfig::from_file_config(parsed, log)
}
+struct CmdConfigReadError {
+ action: &'static str,
+ detail: String,
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the cmd config read/retry logic to embed the transient flag in the error type and use a single `Result`-based flow instead of tuple errors and separate transient checks.
You can keep all the new behavior (retries, BOM handling, transient classification) while simplifying the control flow by:
1. Embedding the transient flag directly in `CmdConfigReadError`.
2. Returning a simple `Result<CmdConfigFile, CmdConfigReadError>` from `read_cmd_config_file_once`.
3. Letting the retry function decide whether to retry based on the error’s `transient` field.
4. Keeping the read/normalize/parse flow in a single function.
A minimal refactor along these lines:
```rust
struct CmdConfigReadError {
action: &'static str,
detail: String,
transient: bool,
}
fn is_transient_io_error(error: &io::Error) -> bool {
matches!(
error.kind(),
io::ErrorKind::Interrupted | io::ErrorKind::WouldBlock | io::ErrorKind::TimedOut
)
}
fn read_cmd_config_file_once(config_path: &Path) -> Result<CmdConfigFile, CmdConfigReadError> {
// read
let raw = fs::read_to_string(config_path).map_err(|error| CmdConfigReadError {
action: "failed to read cmd_config",
detail: error.to_string(),
transient: is_transient_io_error(&error),
})?;
// normalize BOM
let raw = raw.strip_prefix('\u{feff}').unwrap_or(&raw);
// parse + classify transient empty/eof
serde_json::from_str(raw).map_err(|error| {
let transient = raw.is_empty() || (error.is_eof() && !raw.trim().is_empty());
CmdConfigReadError {
action: "failed to parse cmd_config",
detail: error.to_string(),
transient,
}
})
}
fn read_cmd_config_file_with_retry(config_path: &Path) -> Result<CmdConfigFile, CmdConfigReadError> {
let mut last_error: Option<CmdConfigReadError> = None;
for attempt in 0..CMD_CONFIG_READ_RETRY_ATTEMPTS {
match read_cmd_config_file_once(config_path) {
Ok(config) => return Ok(config),
Err(error) => {
let should_retry = error.transient && attempt + 1 < CMD_CONFIG_READ_RETRY_ATTEMPTS;
last_error = Some(error);
if should_retry {
thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
} else {
break;
}
}
}
}
Err(last_error.expect("at least one read attempt"))
}
```
This keeps:
- BOM stripping.
- Transient IO classification.
- Transient empty/EOF classification for retries.
- The same logging interface (`action` + `detail`).
But removes the tuple-based error type and the extra `is_transient_cmd_config_read_error`/`bool` plumbing, making the retry logic and error semantics easier to follow and harder to misuse.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/dashboard-cmd-config-bom
branch
from
e9ce948 to
0b93c4f
Compare
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The retry loop now treats a permanently missing cmd_config.json as a transient NotFound error, adding delays and logging where previously it returned immediately; consider short-circuiting when the file doesn’t exist (e.g., a pre-check with is_file or a separate code path) to keep the default-no-config behavior fast and quiet.
- The new retry-related tests rely on fixed sleep durations (e.g., 1–2 ms) to order the reader and writer threads, which can be timing-flaky on slower or contended environments; consider using explicit synchronization (channels, barriers, or a latch file) instead of tight timing windows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The retry loop now treats a permanently missing cmd_config.json as a transient NotFound error, adding delays and logging where previously it returned immediately; consider short-circuiting when the file doesn’t exist (e.g., a pre-check with is_file or a separate code path) to keep the default-no-config behavior fast and quiet.
- The new retry-related tests rely on fixed sleep durations (e.g., 1–2 ms) to order the reader and writer threads, which can be timing-flaky on slower or contended environments; consider using explicit synchronization (channels, barriers, or a latch file) instead of tight timing windows.
## Individual Comments
### Comment 1
<location path="src-tauri/src/backend/launch.rs" line_range="223" />
<code_context>
CmdDashboardConfig::from_file_config(parsed, log)
}
+struct CmdConfigReadError {
+ action: &'static str,
+ detail: String,
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the cmd_config error and retry handling into a simpler enum-based flow with a centralized retry helper to reduce bespoke plumbing and control-flow complexity.
The transient handling and retry behavior look solid, but you can reduce bespoke error plumbing and control-flow complexity without losing functionality.
You can:
- Drop `CmdConfigReadError` in favor of a small internal enum that keeps the raw content where needed.
- Centralize transient detection in a single `should_retry` helper.
- Simplify the retry loop to early-return on success / non-retryable error, without `last_error` bookkeeping.
For example:
```rust
enum CmdConfigError {
Read(io::Error),
Parse { raw: String, error: serde_json::Error },
}
fn should_retry(error: &CmdConfigError) -> bool {
match error {
CmdConfigError::Read(e) => matches!(
e.kind(),
io::ErrorKind::NotFound
| io::ErrorKind::Interrupted
| io::ErrorKind::WouldBlock
| io::ErrorKind::TimedOut,
),
CmdConfigError::Parse { raw, error } => {
raw.is_empty() || (error.is_eof() && !raw.trim().is_empty())
}
}
}
fn read_cmd_config_file_once(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
let raw = fs::read_to_string(config_path).map_err(CmdConfigError::Read)?;
let raw = raw.strip_prefix('\u{feff}').unwrap_or(&raw).to_owned();
serde_json::from_str(&raw).map_err(|error| CmdConfigError::Parse { raw, error })
}
fn read_cmd_config_file_with_retry(
config_path: &Path,
) -> Result<CmdConfigFile, CmdConfigError> {
for attempt in 0..CMD_CONFIG_READ_RETRY_ATTEMPTS {
match read_cmd_config_file_once(config_path) {
Ok(config) => return Ok(config),
Err(error) => {
let should_retry =
should_retry(&error) && attempt + 1 < CMD_CONFIG_READ_RETRY_ATTEMPTS;
if !should_retry {
return Err(error);
}
thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
}
}
}
unreachable!("loop guarantees return on last attempt");
}
```
Then `read_cmd_dashboard_config` can log directly based on the variant, keeping your existing messages but without carrying `action`/`detail`/`transient` around:
```rust
fn read_cmd_dashboard_config(
root_dir: Option<&Path>,
log: &mut dyn FnMut(&str),
) -> CmdDashboardConfig {
let Some(root_dir) = root_dir else {
return CmdDashboardConfig::default();
};
let config_path = root_dir.join(CMD_CONFIG_RELATIVE_PATH);
let parsed = match read_cmd_config_file_with_retry(&config_path) {
Ok(parsed) => parsed,
Err(err) => {
match err {
CmdConfigError::Read(e) => log(&format!(
"failed to read cmd_config {}: {}",
config_path.display(),
e
)),
CmdConfigError::Parse { error, .. } => log(&format!(
"failed to parse cmd_config {}: {}",
config_path.display(),
error
)),
}
return CmdDashboardConfig::default();
}
};
CmdDashboardConfig::from_file_config(parsed, log)
}
```
This keeps:
- All retry behavior and transient handling exactly as now.
- The same log messages and defaulting behavior.
But it:
- Removes the custom `CmdConfigReadError` struct and local `transient` flag.
- Consolidates transient logic in one helper.
- Simplifies the retry loop and error propagation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/dashboard-cmd-config-bom
branch
from
0b93c4f to
cd45a7c
Compare
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
read_cmd_config_file_once, you can avoid the extra allocation by passing the BOM-stripped&strdirectly intoserde_json::from_strinstead of calling.to_string(). - The retry loop in
read_cmd_config_file_with_retry_after_errorends with anunreachable!even though all paths in theforloop return; consider returning the last error explicitly instead of relying onunreachable!for clarity and future-proofing. - Currently retries of
cmd_config.jsonreads are silent in production; consider emitting a log or metric whenshould_retry_cmd_config_readtriggers so transient config issues are observable in diagnostics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `read_cmd_config_file_once`, you can avoid the extra allocation by passing the BOM-stripped `&str` directly into `serde_json::from_str` instead of calling `.to_string()`.
- The retry loop in `read_cmd_config_file_with_retry_after_error` ends with an `unreachable!` even though all paths in the `for` loop return; consider returning the last error explicitly instead of relying on `unreachable!` for clarity and future-proofing.
- Currently retries of `cmd_config.json` reads are silent in production; consider emitting a log or metric when `should_retry_cmd_config_read` triggers so transient config issues are observable in diagnostics.
## Individual Comments
### Comment 1
<location path="src-tauri/src/backend/launch.rs" line_range="241" />
<code_context>
+ },
+}
+
+fn should_retry_cmd_config_read(error: &CmdConfigError) -> bool {
+ match error {
+ CmdConfigError::Read(error) => matches!(
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the cmd_config retry logic by inlining error classification and using a test-only hook so the control flow and error handling are clearer and less abstracted.
The main complexity comes from `CmdConfigError`, `should_retry_cmd_config_read`, the generic `after_error` callback, and the `unreachable!`. You can keep all existing behavior (BOM stripping, retry semantics, test coverage) while simplifying control flow and error classification.
### 1. Inline retry classification into the retry loop
You can avoid the extra indirection of `CmdConfigError` + `should_retry_cmd_config_read` by doing the matching directly in `read_cmd_config_file_with_retry`, while still returning structured errors for logging.
```rust
#[derive(Debug)]
enum CmdConfigError {
Read(io::Error),
Parse {
raw: String,
error: serde_json::Error,
},
}
fn read_cmd_config_file_once(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
let raw = fs::read_to_string(config_path).map_err(CmdConfigError::Read)?;
let raw = raw.strip_prefix('\u{feff}').unwrap_or(&raw).to_string();
serde_json::from_str(&raw).map_err(|error| CmdConfigError::Parse { raw, error })
}
fn read_cmd_config_file_with_retry(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
for attempt in 0..CMD_CONFIG_READ_RETRY_ATTEMPTS {
match read_cmd_config_file_once(config_path) {
Ok(config) => return Ok(config),
Err(error) => {
let should_retry = match &error {
CmdConfigError::Read(e) => matches!(
e.kind(),
io::ErrorKind::NotFound
| io::ErrorKind::Interrupted
| io::ErrorKind::WouldBlock
| io::ErrorKind::TimedOut
),
CmdConfigError::Parse { raw, error } => {
// Empty files and truncated JSON can occur while cmd_config is being rewritten.
// Whitespace-only files are non-empty invalid JSON, so treat them as misconfiguration.
raw.is_empty() || (error.is_eof() && !raw.trim().is_empty())
}
};
if !should_retry || attempt + 1 == CMD_CONFIG_READ_RETRY_ATTEMPTS {
return Err(error);
}
thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
}
}
}
// Loop always returns on success or final failure
unreachable!("read_cmd_config_file_with_retry loop should not exhaust");
}
```
This keeps the enum for logging but removes the separate `should_retry_cmd_config_read` function and makes the retry logic fully visible in one place.
### 2. Replace the generic `after_error` callback with a small test-only hook
The callback currently exists only to drive tests; you can keep tests working by introducing a simple, test-only hook instead of a generic parameter. This avoids the generic `FnMut` and keeps the production path straightforward.
In the module:
```rust
#[cfg(test)]
type AfterErrorHook = fn(usize);
#[cfg(test)]
static mut AFTER_ERROR_HOOK: Option<AfterErrorHook> = None;
fn read_cmd_config_file_with_retry(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
for attempt in 0..CMD_CONFIG_READ_RETRY_ATTEMPTS {
match read_cmd_config_file_once(config_path) {
Ok(config) => return Ok(config),
Err(error) => {
let should_retry = /* ... inline classification as above ... */;
if !should_retry || attempt + 1 == CMD_CONFIG_READ_RETRY_ATTEMPTS {
return Err(error);
}
#[cfg(test)]
unsafe {
if let Some(hook) = AFTER_ERROR_HOOK {
hook(attempt);
}
}
thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
}
}
}
unreachable!("read_cmd_config_file_with_retry loop should not exhaust");
}
```
And in tests:
```rust
#[test]
fn configure_desktop_dashboard_environment_retries_transient_empty_cmd_config() {
use super::AFTER_ERROR_HOOK;
let root = tempfile::tempdir().expect("temp root");
write_cmd_config(root.path(), "");
let config_path = root.path().join(CMD_CONFIG_RELATIVE_PATH);
let mut retry_count = 0;
unsafe {
AFTER_ERROR_HOOK = Some(|attempt| {
assert_eq!(attempt, 0);
retry_count += 1;
fs::write(
&config_path,
r#"{"dashboard":{"host":"0.0.0.0","port":6185}}"#,
)
.expect("write completed cmd config");
});
}
let config = super::read_cmd_config_file_with_retry(&config_path)
.expect("retry reads completed cmd config");
let parsed = super::CmdDashboardConfig::from_file_config(config, &mut |_| {});
assert_eq!(retry_count, 1);
assert_eq!(parsed.host.as_deref(), Some("0.0.0.0"));
assert_eq!(parsed.port.as_deref(), Some("6185"));
unsafe {
AFTER_ERROR_HOOK = None;
}
}
```
You can encapsulate the unsafe in a small helper or use a `RefCell<Option<...>>` instead if you prefer, but the key point is that the retry API used in production no longer carries a test-only generic parameter.
### 3. Remove the explicit `unreachable!` by structuring the loop to return naturally
Once classification is inline and you check `attempt + 1 == CMD_CONFIG_READ_RETRY_ATTEMPTS` in the same place you decide to retry, the loop either returns `Ok` or `Err`, and you can safely remove the `unreachable!`:
```rust
fn read_cmd_config_file_with_retry(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
for attempt in 0..CMD_CONFIG_READ_RETRY_ATTEMPTS {
match read_cmd_config_file_once(config_path) {
Ok(config) => return Ok(config),
Err(error) => {
let should_retry = /* classification */;
if !should_retry || attempt + 1 == CMD_CONFIG_READ_RETRY_ATTEMPTS {
return Err(error);
}
// optional test hook, then delay
thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
}
}
}
// no need for unreachable – loop logic guarantees a return
}
```
These changes preserve all current behavior (including BOM handling and transient rewrite retries) while reducing abstraction and making the retry path and error classification more directly readable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/dashboard-cmd-config-bom
branch
from
cd45a7c to
56b27e6
Compare
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider extracting the retryable IO error kinds into a small helper or constant set so
should_retry_cmd_config_readis easier to audit and extend if new transient-error cases need to be handled. - The test-only
thread_localretry hook introduces some complexity; you might simplify this by parameterizing the retry function (e.g., injecting a callback or using a trait) instead of conditional compilation, which would make the behavior easier to reason about and reuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the retryable IO error kinds into a small helper or constant set so `should_retry_cmd_config_read` is easier to audit and extend if new transient-error cases need to be handled.
- The test-only `thread_local` retry hook introduces some complexity; you might simplify this by parameterizing the retry function (e.g., injecting a callback or using a trait) instead of conditional compilation, which would make the behavior easier to reason about and reuse.
## Individual Comments
### Comment 1
<location path="src-tauri/src/backend/launch.rs" line_range="303-312" />
<code_context>
+fn read_cmd_config_file_with_retry(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
</code_context>
<issue_to_address>
**suggestion:** Simplify retry loop by removing the `last_error` tracking, which is effectively unreachable.
Because `CMD_CONFIG_READ_RETRY_ATTEMPTS` is a non-zero constant and each iteration either returns `Ok(config)` or `Err(error)` inside the loop, the final `Err(last_error.expect(...))` is never reached. You can remove `last_error` and the final `Err(...)` entirely, or refactor the loop (e.g. `while` + `break`) if you want an explicit fallback path.
Suggested implementation:
```rust
fn read_cmd_config_file_with_retry(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
for attempt in 0..CMD_CONFIG_READ_RETRY_ATTEMPTS {
match read_cmd_config_file_once(config_path) {
Ok(config) => return Ok(config),
Err(error) => {
let retry_attempt = attempt + 1;
if !should_retry_cmd_config_read(&error)
|| retry_attempt >= CMD_CONFIG_READ_RETRY_ATTEMPTS
{
return Err(error);
```
You’ll also need to remove the final `Err(last_error.expect(...))` (or any other usage of `last_error`) at the end of `read_cmd_config_file_with_retry`. If you want to keep a explicit fallback, you can replace that with `unreachable!("CMD_CONFIG_READ_RETRY_ATTEMPTS must be non-zero");`, but it’s strictly optional because the loop already returns on all paths when the constant is non-zero.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/backend/launch.rs" line_range="234" />
<code_context>
}
+#[derive(Debug)]
+enum CmdConfigError {
+ Read(io::Error),
+ Parse {
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the cmd_config error handling and retry loop to avoid storing the full raw string, remove the `Cow` indirection, and eliminate unnecessary bookkeeping like `last_error` and the final `expect`.
You can keep the new behavior while simplifying two areas that add unnecessary complexity and allocations.
### 1. Avoid storing `raw: String` in `CmdConfigError` and the `Cow` dance
You only need to know whether the file was empty or a non-empty truncated JSON. That can be captured as flags at the point of failure, without keeping the whole `String` in the error and without using `Cow`.
Suggested change:
```rust
#[derive(Debug)]
enum CmdConfigError {
Read(io::Error),
Parse {
error: serde_json::Error,
is_empty: bool,
is_nonempty_eof: bool,
},
}
fn should_retry_cmd_config_read(error: &CmdConfigError) -> bool {
match error {
CmdConfigError::Read(error) => matches!(
error.kind(),
io::ErrorKind::NotFound
| io::ErrorKind::Interrupted
| io::ErrorKind::WouldBlock
| io::ErrorKind::TimedOut
),
CmdConfigError::Parse {
is_empty,
is_nonempty_eof,
..
} => {
// Empty files and truncated JSON can occur while cmd_config is being rewritten.
// Whitespace-only files are non-empty invalid JSON, so treat them as misconfiguration.
*is_empty || *is_nonempty_eof
}
}
}
fn describe_cmd_config_error(error: &CmdConfigError) -> String {
match error {
CmdConfigError::Read(error) => format!("read failed: {error}"),
CmdConfigError::Parse { error, .. } => format!("parse failed: {error}"),
}
}
fn read_cmd_config_file_once(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
let mut raw = fs::read_to_string(config_path).map_err(CmdConfigError::Read)?;
// Strip BOM in-place on the String.
if raw.starts_with('\u{feff}') {
raw.remove(0);
}
match serde_json::from_str(&raw) {
Ok(parsed) => Ok(parsed),
Err(error) => {
let is_empty = raw.is_empty();
let is_nonempty_eof = error.is_eof() && !raw.trim().is_empty();
Err(CmdConfigError::Parse {
error,
is_empty,
is_nonempty_eof,
})
}
}
}
```
This keeps all current behavior (BOM handling, retry decisions, logging) but:
- Removes the `Cow` usage and extra allocation from `normalized.into_owned()`.
- Makes `CmdConfigError` lighter and focused on what retry logic actually needs.
### 2. Simplify the retry loop and remove `last_error`
The loop returns immediately on final failure, so `last_error` and the post-loop `expect` are redundant. You can express the control flow more linearly:
```rust
fn read_cmd_config_file_with_retry(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
for attempt in 0..CMD_CONFIG_READ_RETRY_ATTEMPTS {
match read_cmd_config_file_once(config_path) {
Ok(config) => return Ok(config),
Err(error) => {
let retry_attempt = attempt + 1;
if !should_retry_cmd_config_read(&error)
|| retry_attempt >= CMD_CONFIG_READ_RETRY_ATTEMPTS
{
return Err(error);
}
append_desktop_log(&format!(
"retrying cmd_config read {}/{} for {}: {}",
retry_attempt,
CMD_CONFIG_READ_RETRY_ATTEMPTS,
config_path.display(),
describe_cmd_config_error(&error),
));
notify_cmd_config_retry(attempt);
thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
}
}
}
// Unreachable: loop returns on success or on final failure.
unreachable!("cmd_config read retry loop must return");
}
```
This preserves the retry behavior, logging, and test hooks, but reduces bookkeeping and makes the retry path easier to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/dashboard-cmd-config-bom
branch
from
56b27e6 to
23d5584
Compare
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/backend/launch.rs" line_range="242" />
<code_context>
+ },
+}
+
+fn is_retryable_cmd_config_io_error(error: &io::Error) -> bool {
+ matches!(
+ error.kind(),
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the error-classification logic into the retry loop to simplify the control flow while preserving the new behavior and tests.
You can keep the new behavior and tests while reducing indirection by collapsing the error-classification helpers into the retry loop. This removes `is_retryable_cmd_config_io_error`, `should_retry_cmd_config_read`, and `describe_cmd_config_error` without changing semantics.
For example, replace the current retry helpers and loop with something like:
```rust
fn read_cmd_config_file_with_retry(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
read_cmd_config_file_with_retry_and_hook(config_path, |_| {})
}
fn read_cmd_config_file_with_retry_and_hook<F>(
config_path: &Path,
mut after_retryable_error: F,
) -> Result<CmdConfigFile, CmdConfigError>
where
F: FnMut(usize),
{
let mut attempt = 1;
loop {
match read_cmd_config_file_once(config_path) {
Ok(config) => return Ok(config),
Err(error) => {
let retryable = match &error {
CmdConfigError::Read(e) => matches!(
e.kind(),
io::ErrorKind::NotFound
| io::ErrorKind::Interrupted
| io::ErrorKind::WouldBlock
| io::ErrorKind::TimedOut
),
CmdConfigError::Parse {
is_empty,
is_nonempty_eof,
..
} => *is_empty || *is_nonempty_eof,
};
if !retryable || attempt >= CMD_CONFIG_READ_RETRY_ATTEMPTS {
return Err(error);
}
append_desktop_log(&format!(
"retrying cmd_config read {}/{} for {}: {error}",
attempt,
CMD_CONFIG_READ_RETRY_ATTEMPTS,
config_path.display(),
));
after_retryable_error(attempt - 1);
thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
attempt += 1;
}
}
}
}
```
Then you can delete:
```rust
fn is_retryable_cmd_config_io_error(error: &io::Error) -> bool { ... }
fn should_retry_cmd_config_read(error: &CmdConfigError) -> bool { ... }
fn describe_cmd_config_error(error: &CmdConfigError) -> String { ... }
```
This keeps:
- The `CmdConfigError` enum and its extra parse metadata
- All existing retry behavior (including empty vs truncated vs whitespace-only handling)
- The test hook via `read_cmd_config_file_with_retry_and_hook`
but reduces the number of conceptual layers in the retry path and makes the loop easier to follow.
</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 is_retryable_cmd_config_io_error(error: &io::Error) -> bool { |
There was a problem hiding this comment.
issue (complexity): Consider inlining the error-classification logic into the retry loop to simplify the control flow while preserving the new behavior and tests.
You can keep the new behavior and tests while reducing indirection by collapsing the error-classification helpers into the retry loop. This removes is_retryable_cmd_config_io_error, should_retry_cmd_config_read, and describe_cmd_config_error without changing semantics.
For example, replace the current retry helpers and loop with something like:
fn read_cmd_config_file_with_retry(config_path: &Path) -> Result<CmdConfigFile, CmdConfigError> {
read_cmd_config_file_with_retry_and_hook(config_path, |_| {})
}
fn read_cmd_config_file_with_retry_and_hook<F>(
config_path: &Path,
mut after_retryable_error: F,
) -> Result<CmdConfigFile, CmdConfigError>
where
F: FnMut(usize),
{
let mut attempt = 1;
loop {
match read_cmd_config_file_once(config_path) {
Ok(config) => return Ok(config),
Err(error) => {
let retryable = match &error {
CmdConfigError::Read(e) => matches!(
e.kind(),
io::ErrorKind::NotFound
| io::ErrorKind::Interrupted
| io::ErrorKind::WouldBlock
| io::ErrorKind::TimedOut
),
CmdConfigError::Parse {
is_empty,
is_nonempty_eof,
..
} => *is_empty || *is_nonempty_eof,
};
if !retryable || attempt >= CMD_CONFIG_READ_RETRY_ATTEMPTS {
return Err(error);
}
append_desktop_log(&format!(
"retrying cmd_config read {}/{} for {}: {error}",
attempt,
CMD_CONFIG_READ_RETRY_ATTEMPTS,
config_path.display(),
));
after_retryable_error(attempt - 1);
thread::sleep(CMD_CONFIG_READ_RETRY_DELAY);
attempt += 1;
}
}
}
}Then you can delete:
fn is_retryable_cmd_config_io_error(error: &io::Error) -> bool { ... }
fn should_retry_cmd_config_read(error: &CmdConfigError) -> bool { ... }
fn describe_cmd_config_error(error: &CmdConfigError) -> String { ... }This keeps:
- The
CmdConfigErrorenum and its extra parse metadata - All existing retry behavior (including empty vs truncated vs whitespace-only handling)
- The test hook via
read_cmd_config_file_with_retry_and_hook
but reduces the number of conceptual layers in the retry path and makes the loop easier to follow.
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
data/cmd_config.jsonwhen resolving desktop dashboard host/port127.0.0.1while the config file is being rewrittenASTRBOT_ROOTenvironment overridesRoot cause
A user
cmd_config.jsoncan be saved as UTF-8 with BOM.serde_json::from_strrejects the leading BOM withexpected value at line 1 column 1, so desktop ignoreddashboard.host = 0.0.0.0and fell back to local-only127.0.0.1.Verification
rtk cargo fmt --manifest-path src-tauri/Cargo.toml -- --checkrtk cargo test --manifest-path src-tauri/Cargo.tomlrtk cargo check --manifest-path src-tauri/Cargo.tomlSummary by Sourcery
Handle UTF-8 BOM and transient read errors in desktop dashboard command config resolution and isolate desktop settings tests from ASTRBOT_ROOT environment side effects.
Bug Fixes:
Tests: