Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 5 additions & 58 deletions src-tauri/src/commands/openai_voice_credentials.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

const KEYCHAIN_SERVICE: &str = "berd-openai-voice";
const KEYCHAIN_ACCOUNT: &str = "api-key";
const LEGACY_TTS_KEYCHAIN_ACCOUNT: &str = "tts-api-key";

#[derive(Clone, Copy)]
pub(crate) enum OpenAiVoiceCredential {
Expand Down Expand Up @@ -59,45 +58,19 @@ fn clear_account(account: &str) -> Result<(), String> {
}
}

fn canonical_mutation_with_legacy_cleanup<T>(
canonical_mutation: impl FnOnce() -> Result<T, String>,
legacy_cleanup: impl FnOnce() -> Result<(), String>,
) -> Result<T, String> {
let value = canonical_mutation()?;
if let Err(error) = legacy_cleanup() {
log::warn!("Could not remove Berd's legacy OpenAI voice credential: {error}");
}
Ok(value)
}

pub(crate) fn read(credential: OpenAiVoiceCredential) -> Result<Option<String>, String> {
if let Some(api_key) = read_account(credential.account())? {
return Ok(Some(api_key));
}
let Some(api_key) = read_account(LEGACY_TTS_KEYCHAIN_ACCOUNT)? else {
return Ok(None);
};
store(credential, &api_key)?;
Ok(Some(api_key))
read_account(credential.account())
}

pub(crate) fn store(credential: OpenAiVoiceCredential, api_key: &str) -> Result<(), String> {
let entry = entry(credential.account())?;
canonical_mutation_with_legacy_cleanup(
|| {
entry
.set_password(api_key)
.map_err(|error| format!("Could not save Berd's OpenAI voice credential: {error}"))
},
|| clear_account(LEGACY_TTS_KEYCHAIN_ACCOUNT),
)
entry
.set_password(api_key)
.map_err(|error| format!("Could not save Berd's OpenAI voice credential: {error}"))
}

pub(crate) fn clear(credential: OpenAiVoiceCredential) -> Result<(), String> {
canonical_mutation_with_legacy_cleanup(
|| clear_account(credential.account()),
|| clear_account(LEGACY_TTS_KEYCHAIN_ACCOUNT),
)
clear_account(credential.account())
}

pub(crate) fn require(credential: OpenAiVoiceCredential) -> Result<String, String> {
Expand All @@ -107,36 +80,10 @@ pub(crate) fn require(credential: OpenAiVoiceCredential) -> Result<String, Strin
#[cfg(test)]
mod tests {
use super::*;
use std::cell::RefCell;

#[test]
fn speech_services_use_the_shared_voice_keychain_account() {
assert_eq!(OpenAiVoiceCredential::SpeechToText.account(), "api-key");
assert_eq!(OpenAiVoiceCredential::TextToSpeech.account(), "api-key");
assert_eq!(OpenAiVoiceCredential::Realtime.account(), "api-key");
}

#[test]
fn legacy_cleanup_failure_does_not_change_canonical_mutation_result() {
let credential = RefCell::new(None);
let save = canonical_mutation_with_legacy_cleanup(
|| {
credential.replace(Some("shared-key"));
Ok(())
},
|| Err("legacy cleanup failed".to_string()),
);
assert_eq!(save, Ok(()));
assert_eq!(*credential.borrow(), Some("shared-key"));

let clear = canonical_mutation_with_legacy_cleanup(
|| {
credential.replace(None);
Ok(())
},
|| Err("legacy cleanup failed".to_string()),
);
assert_eq!(clear, Ok(()));
assert_eq!(*credential.borrow(), None);
}
}
7 changes: 4 additions & 3 deletions src/features/voice-conversation/api/openAiVoice.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { invoke } from "@tauri-apps/api/core";
import { shareInFlight } from "@/shared/lib/shareInFlight";
import { listen, type UnlistenFn } from "@tauri-apps/api/event";
import type { VoiceDeliveryProgress } from "./pocketVoice";
import type {
Expand Down Expand Up @@ -28,9 +29,9 @@ export interface OpenAiVoiceStreamEvent {
delivery?: VoiceDeliveryProgress | null;
}

export function getOpenAiVoiceStatus(): Promise<OpenAiVoiceStatus> {
return invoke<OpenAiVoiceStatus>("get_openai_voice_status");
}
export const getOpenAiVoiceStatus = shareInFlight(
(): Promise<OpenAiVoiceStatus> => invoke("get_openai_voice_status"),
);

export function setOpenAiTtsApiKey(apiKey: string): Promise<void> {
return invoke("set_openai_tts_api_key", { apiKey });
Expand Down
35 changes: 28 additions & 7 deletions src/features/voice-conversation/hooks/useOpenAiVoiceSetup.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,17 @@ import type { OpenAiVoiceStatus } from "../api/openAiVoice";
import { useOpenAiVoiceSetup } from "./useOpenAiVoiceSetup";

const mocks = vi.hoisted(() => ({
getStatus: vi.fn<() => Promise<OpenAiVoiceStatus>>(),
settingsChanged: null as (() => void) | null,
getStatus:
vi.fn<(options?: { coalesce?: boolean }) => Promise<OpenAiVoiceStatus>>(),
settingsChanged: null as ((event?: unknown) => void) | null,
finishListening: null as (() => void) | null,
listenerError: null as Error | null,
}));

vi.mock("../api/openAiVoice", () => ({
getOpenAiVoiceStatus: () => mocks.getStatus(),
listenToOpenAiVoiceSettings: (listener: () => void) => {
getOpenAiVoiceStatus: (options?: { coalesce?: boolean }) =>
mocks.getStatus(options),
listenToOpenAiVoiceSettings: (listener: (event?: unknown) => void) => {
mocks.settingsChanged = listener;
if (mocks.listenerError) return Promise.reject(mocks.listenerError);
return new Promise<() => void>((resolve) => {
Expand Down Expand Up @@ -66,8 +68,16 @@ describe("useOpenAiVoiceSetup", () => {
await waitFor(() => expect(mocks.settingsChanged).not.toBeNull());
act(() => mocks.finishListening?.());
await waitFor(() => expect(mocks.getStatus).toHaveBeenCalledTimes(1));

act(() => mocks.settingsChanged?.());
expect(mocks.getStatus).toHaveBeenLastCalledWith({ coalesce: true });

act(() =>
mocks.settingsChanged?.({
event: "openai-voice:settings-changed",
id: 1,
payload: null,
}),
);
expect(mocks.getStatus).toHaveBeenLastCalledWith(undefined);
refreshed.resolve(status(true));
await waitFor(() =>
expect(result.current.status?.sttConfigured).toBe(true),
Expand Down Expand Up @@ -123,7 +133,7 @@ describe("useOpenAiVoiceSetup", () => {
expect(result.current.error).toBe("Keychain unavailable");
});

it("does not expose cached readiness while disabled", async () => {
it("does not reuse cached readiness after being disabled", async () => {
mocks.listenerError = new Error("listener unavailable");
mocks.getStatus.mockResolvedValue(status(true));
const { result, rerender } = renderHook(
Expand All @@ -137,5 +147,16 @@ describe("useOpenAiVoiceSetup", () => {
rerender({ enabled: false });

expect(result.current).toEqual({ status: null, error: null });

const reloaded = deferred<OpenAiVoiceStatus>();
mocks.getStatus.mockReturnValueOnce(reloaded.promise);
rerender({ enabled: true });

expect(result.current).toEqual({ status: null, error: null });

reloaded.resolve(status(false));
await waitFor(() =>
expect(result.current.status?.ttsConfigured).toBe(false),
);
});
});
14 changes: 9 additions & 5 deletions src/features/voice-conversation/hooks/useOpenAiVoiceSetup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,17 @@ export function useOpenAiVoiceSetup(enabled = true) {
const [error, setError] = useState<string | null>(null);

useEffect(() => {
if (!enabled) return;
if (!enabled) {
setStatus(null);
setError(null);
return;
}
let active = true;
let refreshGeneration = 0;
let unsubscribe: (() => void) | null = null;
const refresh = () => {
const refresh = (coalesce = false) => {
Comment thread
johnmatthewtennant marked this conversation as resolved.
const generation = ++refreshGeneration;
void getOpenAiVoiceStatus().then(
void getOpenAiVoiceStatus(coalesce ? { coalesce: true } : undefined).then(
(next) => {
if (active && generation === refreshGeneration) {
setStatus(next);
Expand All @@ -31,11 +35,11 @@ export function useOpenAiVoiceSetup(enabled = true) {
},
);
};
void listenToOpenAiVoiceSettings(refresh).then(
void listenToOpenAiVoiceSettings(() => refresh()).then(
(nextUnsubscribe) => {
if (active) {
unsubscribe = nextUnsubscribe;
refresh();
refresh(true);
} else nextUnsubscribe();
},
() => {
Expand Down
92 changes: 88 additions & 4 deletions src/features/voice-conversation/ui/VoiceSettings.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,20 @@ import type { VoiceInputBackend } from "../lib/voiceInputPreference";
import type { VoiceOutputBackend } from "../lib/voiceOutputPreference";
import { VoiceSettings } from "./VoiceSettings";

if (!HTMLElement.prototype.hasPointerCapture) {
HTMLElement.prototype.hasPointerCapture = () => false;
}

if (!HTMLElement.prototype.scrollIntoView) {
HTMLElement.prototype.scrollIntoView = () => {};
}

const platformState = vi.hoisted(() => ({ current: "mac" }));

vi.mock("@/shared/lib/platform", () => ({
getPlatform: () => platformState.current,
}));

const setupState = vi.hoisted(() => ({
current: null as PocketVoiceSetup | null,
}));
Expand Down Expand Up @@ -51,6 +65,8 @@ const microphonePermissionState = vi.hoisted(() => ({
openSettings: vi.fn(),
}));
const openAiStatusState = vi.hoisted(() => ({
enabled: null as boolean | null,
loaded: true,
current: {
sttConfigured: true,
ttsConfigured: true,
Expand Down Expand Up @@ -81,10 +97,14 @@ vi.mock("../api/openAiVoice", () => ({
clearOpenAiTtsApiKey: openAiApiMocks.clearTtsApiKey,
}));
vi.mock("../hooks/useOpenAiVoiceSetup", () => ({
useOpenAiVoiceSetup: () => ({
status: openAiStatusState.current,
error: null,
}),
useOpenAiVoiceSetup: (enabled: boolean) => {
openAiStatusState.enabled = enabled;
return {
status:
enabled && openAiStatusState.loaded ? openAiStatusState.current : null,
error: null,
};
},
}));
vi.mock("../hooks/usePocketVoiceSetup", () => ({
usePocketVoiceSetup: () => setupState.current,
Expand Down Expand Up @@ -207,6 +227,8 @@ describe("VoiceSettings", () => {
microphonePermissionState.openSettings.mockReset();
inputState.backend = "parakeet";
outputState.backend = "pocket";
platformState.current = "mac";
openAiStatusState.loaded = true;
macSpeechSetupState.current = {
status: {
supported: false,
Expand Down Expand Up @@ -246,6 +268,68 @@ describe("VoiceSettings", () => {
openAiApiMocks.clearSttApiKey.mockClear();
});

it("does not inspect OpenAI credentials for Apple speech input and output", () => {
inputState.backend = "macos";
outputState.backend = "siri";

renderWithProviders(<VoiceSettings />);

expect(openAiStatusState.enabled).toBe(false);
});

it("keeps OpenAI voice playback selectable without inspecting credentials", async () => {
setupState.current = setup(pocketStatus());
renderWithProviders(<VoiceSettings />);

expect(openAiStatusState.enabled).toBe(false);

const user = userEvent.setup();
await user.click(screen.getByRole("combobox", { name: "Speech output" }));

expect(
screen.getByRole("option", { name: "OpenAI text-to-speech" }),
).toBeInTheDocument();
});

it("does not offer OpenAI voice playback on unsupported platforms", async () => {
platformState.current = "linux";
setupState.current = setup(pocketStatus());
renderWithProviders(<VoiceSettings />);

const user = userEvent.setup();
await user.click(screen.getByRole("combobox", { name: "Speech output" }));

expect(
screen.queryByRole("option", { name: "OpenAI text-to-speech" }),
).not.toBeInTheDocument();
});

it("waits for OpenAI credential status before showing readiness guidance", () => {
outputState.backend = "openai";
openAiStatusState.loaded = false;
setupState.current = setup(pocketStatus({ parakeetInstalled: true }));

renderWithProviders(<VoiceSettings />);

expect(screen.queryByRole("alert")).not.toBeInTheDocument();
expect(
screen.getByText("Checking OpenAI voice settings…"),
).toBeInTheDocument();
});

it.each([
["openai", "siri"],
["macos", "openai"],
["openai", "openai"],
] as const)("inspects OpenAI credentials for %s speech input and %s speech output", (inputBackend, outputBackend) => {
inputState.backend = inputBackend;
outputState.backend = outputBackend;

renderWithProviders(<VoiceSettings />);

expect(openAiStatusState.enabled).toBe(true);
});

it("renders independently selected OpenAI input and output settings", async () => {
inputState.backend = "openai";
outputState.backend = "openai";
Expand Down
Loading