fix: sync preferences.json when switching ASR provider

[PINKIIILQWQ]

User description

Problem

Changing the ASR provider in Settings (e.g. from whisper to volcengine) calls set_active_asr_provider which only writes to the system Keychain, but does not update preferences.json. The preferences file update happens through a separate async path (updatePrefs → setSettings), creating a two-step save that can get out of sync.

If the second step fails (or if the app is restarted before it completes), preferences.json still has the old provider, and the user sees their setting "not saved".

Fix

set_active_asr_provider now also syncs the provider to preferences.json immediately after writing to Keychain, so both stores are always consistent regardless of what the frontend does.

if let Ok(mut prefs) = coord.read_settings() {
    prefs.active_asr_provider = provider.clone();
    let _ = coord.write_settings(prefs);
}

Test

1. Open Settings → change ASR provider
2. Kill and restart OpenLess
3. The selected provider persists correctly

---

PR Type

Bug fix

---

Description

• Sync ASR provider to preferences.json

• Sync LLM provider to preferences.json

• Warn when preference write fails

---

Diagram Walkthrough

flowchart LR
  A["set_active_asr_provider"] -- "write Keychain" --> B["preferences.json sync"]
  C["set_active_llm_provider"] -- "write Keychain" --> D["preferences.json sync"]

Loading

File Walkthrough

Relevant files

Bug fix

commands.rs Sync provider updates to settings file                                      openless-all/app/src-tauri/src/commands.rs After updating the ASR provider in CredentialsVault, also writes active_asr_provider into preferences.json. Changes set_active_llm_provider to accept CoordinatorState, then persists active_llm_provider to settings too. Logs a warning when syncing preferences.json fails, instead of failing the whole command. +19/-2

---

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 3c9612e)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Silent Failure If coord.write_settings fails here, the command still returns Ok(()). The UI will treat the provider change as saved even though preferences.json stays stale and the selection can revert after restart. if let Err(e) = coord.write_settings(prefs) { log::warn!("[set_active_asr_provider] sync preferences.json failed: {e}"); } Silent Failure The same error handling is used in the new LLM provider setter, so a preferences write failure is only logged. In that case the command reports success even though preferences.json was not updated, which can make the user's change appear saved until the next launch. if let Err(e) = coord.write_settings(prefs) { log::warn!("[set_active_llm_provider] sync preferences.json failed: {e}"); }

[github-actions]

Persistent review updated to latest commit 8e14d40

[github-actions]

Persistent review updated to latest commit 754acb9

[github-actions]

Persistent review updated to latest commit 3c9612e

[appergb]

@claude Review.

[claude]

Claude finished @appergb's task in 2m 9s —— View job

---

Code Review

• Gather context and read PR diff
• Analyze changed code in commands.rs
• Post review findings

---

Summary

The fix correctly identifies and addresses the two-step save problem. Writing active_asr_provider / active_llm_provider to preferences.json immediately inside the Tauri command — rather than relying solely on the frontend's async updatePrefs path — is the right approach. The second commit's upgrade from let _ = to log::warn! is a meaningful improvement.

---

Findings

1. Silent failure on write_settings — design trade-off worth documenting

Both handlers now log a warning when write_settings fails, but still return Ok(()):

// commands.rs:815–817
if let Err(e) = coord.write_settings(prefs) {
    log::warn!("[set_active_asr_provider] sync preferences.json failed: {e}");
}

The automated review flagged this correctly. The PR description justifies it (Keychain is the primary store; this write is a best-effort sync), but that reasoning should be captured as a comment near the code rather than only in the PR description. Without it, a future reader might "fix" this by propagating the error, not realising that's intentional. Fix this →

2. Unnecessary .clone() in set_active_llm_provider — minor

// commands.rs:841
prefs.active_llm_provider = provider.clone();

provider is not used after this line in set_active_llm_provider, so the .clone() allocates needlessly. A direct move (= provider) is sufficient. (In set_active_asr_provider the clone is required because provider is borrowed later at lines 818–820.) Fix this →

3. Misleading Chinese comment — minor

// commands.rs:810
// 同步更新偏好文件，确保 Keychain 和 preferences.json 不同步。

"不同步" literally reads as "not in sync / out of sync", the opposite of the intent. The sentence says it ensures the two stores don't stay in sync, which is confusing. Consider: 确保 Keychain 与 preferences.json 保持一致 ("ensure Keychain and preferences.json stay consistent"). Fix this →

4. Double write in normal flow — not a bug, just worth knowing

In the happy path, preferences.json is written twice: once by this new code, and once by the frontend's subsequent updatePrefs → persist_settings → write_settings call. This is harmless (the second write is idempotent for the provider field), but it's worth knowing in case the file-write cost ever becomes a concern on slow storage. The existing PrefStore::set uses atomic_write so there's no corruption risk.

5. TOCTOU on read → modify → write — low practical risk

Both handlers do:

let mut prefs = coord.read_settings();   // lock, clone, release
prefs.active_X_provider = ...;
coord.write_settings(prefs);             // lock, write, release

Between the two lock acquisitions, another concurrent write_settings (e.g. the user saving other settings simultaneously) could be overwritten. In practice this race is extremely narrow and the frontend's subsequent updatePrefs is the authoritative write, so this is low risk — but it's the same pattern that would need addressing if this code ever needs to be made fully concurrent-safe.

---

Verdict

Approve with minor suggestions. The core fix is correct and safe. Issues 1–3 are small polish items; 4–5 are informational. The most actionable improvement is adding an inline comment explaining the intentional Ok(()) on write_settings failure.