mirror of
https://github.com/jamiepine/voicebox.git
synced 2026-09-16 13:20:39 -07:00
fix: address PR #319 review feedback — health validation, error handling, queryClient decoupling
- Rust: Replace fragile body.contains("status") with proper JSON
deserialization validating status=="healthy", model_loaded (bool),
and gpu_available (bool) to prevent misidentifying non-Voicebox services
- Frontend: Validate health response has Voicebox-specific fields before
marking server as ready during fallback polling
- Frontend: Discriminate port-in-use errors (poll for external server) from
real startup failures (missing sidecar, signing issues) — surface errors
immediately with a startupError state and Retry button in the UI
- Frontend: Set explicit startup-error state when 2-minute polling timeout
expires so the loading screen shows actionable feedback instead of hanging
- Architecture: Extract QueryClient to standalone side-effect-free module
(lib/queryClient.ts) to decouple serverStore from React bootstrap entrypoint
This commit is contained in:
+84
-14
@@ -5,6 +5,7 @@ import ShinyText from '@/components/ShinyText';
|
||||
import { TitleBarDragRegion } from '@/components/TitleBarDragRegion';
|
||||
import { useAutoUpdater } from '@/hooks/useAutoUpdater';
|
||||
import { apiClient } from '@/lib/api/client';
|
||||
import type { HealthResponse } from '@/lib/api/types';
|
||||
import { TOP_SAFE_AREA_PADDING } from '@/lib/constants/ui';
|
||||
import { cn } from '@/lib/utils/cn';
|
||||
import { usePlatform } from '@/platform/PlatformContext';
|
||||
@@ -12,6 +13,33 @@ import { router } from '@/router';
|
||||
import { useLogStore } from '@/stores/logStore';
|
||||
import { useServerStore } from '@/stores/serverStore';
|
||||
|
||||
/**
|
||||
* Validate that a health response has the expected Voicebox-specific shape.
|
||||
* Prevents misidentifying an unrelated service on the same port.
|
||||
*/
|
||||
function isVoiceboxHealthResponse(health: HealthResponse): boolean {
|
||||
return (
|
||||
health?.status === 'healthy' &&
|
||||
typeof health.model_loaded === 'boolean' &&
|
||||
typeof health.gpu_available === 'boolean'
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Check whether a startup error indicates the port is occupied by an external
|
||||
* server (which we should try to reuse via health-check polling) vs. a real
|
||||
* failure (missing sidecar, signing issue, etc.) that should surface immediately.
|
||||
*/
|
||||
function isPortInUseError(error: unknown): boolean {
|
||||
const msg = error instanceof Error ? error.message : String(error);
|
||||
return (
|
||||
msg.includes('already in use') ||
|
||||
msg.includes('port') ||
|
||||
msg.includes('EADDRINUSE') ||
|
||||
msg.includes('address already in use')
|
||||
);
|
||||
}
|
||||
|
||||
const LOADING_MESSAGES = [
|
||||
'Warming up tensors...',
|
||||
'Calibrating synthesizer engine...',
|
||||
@@ -38,6 +66,7 @@ const LOADING_MESSAGES = [
|
||||
function App() {
|
||||
const platform = usePlatform();
|
||||
const [serverReady, setServerReady] = useState(false);
|
||||
const [startupError, setStartupError] = useState<string | null>(null);
|
||||
const [loadingMessageIndex, setLoadingMessageIndex] = useState(0);
|
||||
const serverStartingRef = useRef(false);
|
||||
|
||||
@@ -124,14 +153,29 @@ function App() {
|
||||
// @ts-expect-error - adding property to window
|
||||
window.__voiceboxServerStartedByApp = false;
|
||||
|
||||
// Only fall back to health-check polling when the error indicates the
|
||||
// port is occupied (likely an external server). For real failures
|
||||
// (missing sidecar, signing issues, etc.) surface the error immediately.
|
||||
if (!isPortInUseError(error)) {
|
||||
const msg = error instanceof Error ? error.message : String(error);
|
||||
console.error('Real startup failure — not polling:', msg);
|
||||
setStartupError(msg);
|
||||
return;
|
||||
}
|
||||
|
||||
// Fall back to polling: the server may already be running externally
|
||||
// (e.g. started via python/uvicorn/Docker). Poll the health endpoint
|
||||
// until it responds, then transition to the main UI.
|
||||
// until it responds with a valid Voicebox payload, then transition to
|
||||
// the main UI.
|
||||
console.log('Falling back to health-check polling...');
|
||||
const pollInterval = setInterval(async () => {
|
||||
try {
|
||||
await apiClient.getHealth();
|
||||
console.log('External server detected via health check');
|
||||
const health = await apiClient.getHealth();
|
||||
if (!isVoiceboxHealthResponse(health)) {
|
||||
console.log('Health response is not from a Voicebox server, keep polling...');
|
||||
return;
|
||||
}
|
||||
console.log('External Voicebox server detected via health check');
|
||||
clearInterval(pollInterval);
|
||||
setServerReady(true);
|
||||
} catch {
|
||||
@@ -139,8 +183,15 @@ function App() {
|
||||
}
|
||||
}, 2000);
|
||||
|
||||
// Stop polling after 2 minutes to avoid polling forever
|
||||
setTimeout(() => clearInterval(pollInterval), 120_000);
|
||||
// Stop polling after 2 minutes and surface the failure
|
||||
setTimeout(() => {
|
||||
clearInterval(pollInterval);
|
||||
serverStartingRef.current = false;
|
||||
setStartupError(
|
||||
'Could not connect to a Voicebox server within 2 minutes. ' +
|
||||
'Please check that the server is running and try again.',
|
||||
);
|
||||
}, 120_000);
|
||||
});
|
||||
|
||||
// Cleanup: stop server on actual unmount (not StrictMode remount)
|
||||
@@ -187,15 +238,34 @@ function App() {
|
||||
className="w-48 h-48 object-contain animate-fade-in-scale relative z-10"
|
||||
/>
|
||||
</div>
|
||||
<div className="animate-fade-in-delayed">
|
||||
<ShinyText
|
||||
text={LOADING_MESSAGES[loadingMessageIndex]}
|
||||
className="text-lg font-medium text-muted-foreground"
|
||||
speed={2}
|
||||
color="hsl(var(--muted-foreground))"
|
||||
shineColor="hsl(var(--foreground))"
|
||||
/>
|
||||
</div>
|
||||
{startupError ? (
|
||||
<div className="animate-fade-in-delayed max-w-md mx-auto space-y-3">
|
||||
<p className="text-lg font-medium text-destructive">Server startup failed</p>
|
||||
<p className="text-sm text-muted-foreground">{startupError}</p>
|
||||
<button
|
||||
type="button"
|
||||
className="mt-2 px-4 py-2 text-sm rounded-md bg-primary text-primary-foreground hover:bg-primary/90 transition-colors"
|
||||
onClick={() => {
|
||||
setStartupError(null);
|
||||
serverStartingRef.current = false;
|
||||
// Trigger a re-mount of the effect by toggling state
|
||||
window.location.reload();
|
||||
}}
|
||||
>
|
||||
Retry
|
||||
</button>
|
||||
</div>
|
||||
) : (
|
||||
<div className="animate-fade-in-delayed">
|
||||
<ShinyText
|
||||
text={LOADING_MESSAGES[loadingMessageIndex]}
|
||||
className="text-lg font-medium text-muted-foreground"
|
||||
speed={2}
|
||||
color="hsl(var(--muted-foreground))"
|
||||
shineColor="hsl(var(--foreground))"
|
||||
/>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
|
||||
@@ -0,0 +1,19 @@
|
||||
import { QueryClient } from '@tanstack/react-query';
|
||||
|
||||
/**
|
||||
* Shared QueryClient instance used across the app.
|
||||
*
|
||||
* Extracted into its own side-effect-free module so it can be imported from
|
||||
* both the React bootstrap (main.tsx) and non-React code (stores, utilities)
|
||||
* without pulling in ReactDOM or other bootstrap side effects.
|
||||
*/
|
||||
export const queryClient = new QueryClient({
|
||||
defaultOptions: {
|
||||
queries: {
|
||||
staleTime: 1000 * 60 * 5, // 5 minutes
|
||||
gcTime: 1000 * 60 * 10, // 10 minutes (formerly cacheTime)
|
||||
retry: 1,
|
||||
refetchOnWindowFocus: false,
|
||||
},
|
||||
},
|
||||
});
|
||||
+2
-12
@@ -1,20 +1,10 @@
|
||||
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
|
||||
import { QueryClientProvider } from '@tanstack/react-query';
|
||||
// import { ReactQueryDevtools } from '@tanstack/react-query-devtools';
|
||||
import React from 'react';
|
||||
import ReactDOM from 'react-dom/client';
|
||||
import App from './App';
|
||||
import './index.css';
|
||||
|
||||
export const queryClient = new QueryClient({
|
||||
defaultOptions: {
|
||||
queries: {
|
||||
staleTime: 1000 * 60 * 5, // 5 minutes
|
||||
gcTime: 1000 * 60 * 10, // 10 minutes (formerly cacheTime)
|
||||
retry: 1,
|
||||
refetchOnWindowFocus: false,
|
||||
},
|
||||
},
|
||||
});
|
||||
import { queryClient } from './lib/queryClient';
|
||||
|
||||
ReactDOM.createRoot(document.getElementById('root')!).render(
|
||||
<React.StrictMode>
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { create } from 'zustand';
|
||||
import { persist } from 'zustand/middleware';
|
||||
import { queryClient } from '@/lib/queryClient';
|
||||
|
||||
interface ServerStore {
|
||||
serverUrl: string;
|
||||
@@ -31,15 +32,11 @@ interface ServerStore {
|
||||
}
|
||||
|
||||
/**
|
||||
* Invalidate all React Query caches and reset UI selection state.
|
||||
* Called when the server URL changes so stale data from the previous
|
||||
* server is not shown.
|
||||
* Invalidate all React Query caches so stale data from the previous
|
||||
* server is not shown. Called when the server URL changes.
|
||||
*/
|
||||
function invalidateAllServerData() {
|
||||
// Lazy import to avoid circular dependency (main.tsx -> serverStore -> main.tsx)
|
||||
import('@/main').then(({ queryClient }) => {
|
||||
queryClient.invalidateQueries();
|
||||
});
|
||||
queryClient.invalidateQueries();
|
||||
}
|
||||
|
||||
export const useServerStore = create<ServerStore>()(
|
||||
|
||||
@@ -55,9 +55,11 @@ fn find_voicebox_pid_on_port(port: u16) -> Option<u32> {
|
||||
|
||||
/// Check if a Voicebox server is responding on the given port.
|
||||
///
|
||||
/// Sends an HTTP GET to `/health` and returns `true` if the response
|
||||
/// contains the expected JSON field (`"status"`), confirming it's
|
||||
/// a Voicebox backend rather than an unrelated service.
|
||||
/// Sends an HTTP GET to `/health` and returns `true` only if the response
|
||||
/// is valid JSON matching the Voicebox `HealthResponse` schema — specifically
|
||||
/// `status` must be `"healthy"`, and both `model_loaded` and `gpu_available`
|
||||
/// must be present as booleans. This prevents misidentifying an unrelated
|
||||
/// service that happens to expose a `/health` endpoint.
|
||||
#[allow(dead_code)] // Used in platform-specific cfg blocks
|
||||
fn check_health(port: u16) -> bool {
|
||||
let url = format!("http://127.0.0.1:{}/health", port);
|
||||
@@ -70,9 +72,13 @@ fn check_health(port: u16) -> bool {
|
||||
if !resp.status().is_success() {
|
||||
return false;
|
||||
}
|
||||
// Verify the body looks like a Voicebox health response
|
||||
match resp.text() {
|
||||
Ok(body) => body.contains("status"),
|
||||
// Parse as JSON and validate Voicebox-specific fields
|
||||
match resp.json::<serde_json::Value>() {
|
||||
Ok(body) => {
|
||||
body.get("status").and_then(|v| v.as_str()) == Some("healthy")
|
||||
&& body.get("model_loaded").map(|v| v.is_boolean()).unwrap_or(false)
|
||||
&& body.get("gpu_available").map(|v| v.is_boolean()).unwrap_or(false)
|
||||
}
|
||||
Err(_) => false,
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user