From 789953a7c6466de9122158e671968464fa758c0b Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Thu, 7 May 2026 02:02:20 +0530 Subject: [PATCH 1/3] feat(config): RPC bootstrap hardening, socket fix, and exhaustive tests (#933) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the remaining gaps from issue #933: Bug fix: - socketService: resolveCoreSocketBaseUrl() now calls getCoreRpcUrl() instead of duplicating resolution via invoke('core_rpc_url') — socket now connects to the user's stored RPC URL, not always the static default P2 polish: - backendUrl: export clearBackendUrlCache() so RPC URL changes cascade to the backend URL cache; Welcome calls it on save + reset - config: add JSDoc to CORE_RPC_URL and BACKEND_URL marking them as build-time fallbacks with runtime resolution chain documented inline - configPersistence: remove dead buildRpcEndpoint() export (zero call sites) Tests (133 new, all passing): - Welcome.test.tsx (+30): panel toggle, save/test/reset flows, validation errors, cache-clear calls, Test Connection loading/success/error states - coreRpcClient.test.ts (+7): getCoreRpcUrl stored-URL preference, caching, post-clear refresh, Tauri invoke fallback, stored priority over invoke - backendUrl.test.ts (+5): clearBackendUrlCache forces re-derive, cache hit, RPC error fallback, non-Tauri path - socketService.test.ts (new, 4): getCoreRpcUrl called on connect, /rpc stripped, no-suffix handled, custom stored URL propagated end-to-end - configPersistence.test.ts (+24): isValidRpcUrl edge cases, normalizeRpcUrl edge cases, round-trip with various URL types, localStorage-unavailable fallback Closes #933 --- app/src/pages/Welcome.tsx | 3 + app/src/pages/__tests__/Welcome.test.tsx | 450 +++++++++++++++++- app/src/services/__tests__/backendUrl.test.ts | 73 ++- .../services/__tests__/coreRpcClient.test.ts | 114 +++++ .../services/__tests__/socketService.test.ts | 142 ++++++ app/src/services/backendUrl.ts | 11 + app/src/services/socketService.ts | 22 +- .../utils/__tests__/configPersistence.test.ts | 122 ++++- app/src/utils/config.ts | 22 +- app/src/utils/configPersistence.ts | 11 - 10 files changed, 939 insertions(+), 31 deletions(-) create mode 100644 app/src/services/__tests__/socketService.test.ts diff --git a/app/src/pages/Welcome.tsx b/app/src/pages/Welcome.tsx index cf192ca12..85ebb0c2e 100644 --- a/app/src/pages/Welcome.tsx +++ b/app/src/pages/Welcome.tsx @@ -3,6 +3,7 @@ import { useState } from 'react'; import OAuthProviderButton from '../components/oauth/OAuthProviderButton'; import { oauthProviderConfigs } from '../components/oauth/providerConfigs'; import RotatingTetrahedronCanvas from '../components/RotatingTetrahedronCanvas'; +import { clearBackendUrlCache } from '../services/backendUrl'; import { clearCoreRpcUrlCache, testCoreRpcConnection } from '../services/coreRpcClient'; import { useDeepLinkAuthState } from '../store/deepLinkAuthState'; import { @@ -39,6 +40,7 @@ const Welcome = () => { storeRpcUrl(normalized); clearCoreRpcUrlCache(); + clearBackendUrlCache(); setRpcUrlError(null); setSaveSuccess(true); @@ -48,6 +50,7 @@ const Welcome = () => { const handleResetRpcUrl = () => { clearStoredRpcUrl(); clearCoreRpcUrlCache(); + clearBackendUrlCache(); setRpcUrl(getDefaultRpcUrl()); setRpcUrlError(null); setSaveSuccess(false); diff --git a/app/src/pages/__tests__/Welcome.test.tsx b/app/src/pages/__tests__/Welcome.test.tsx index 54f18b089..a4ffa9b35 100644 --- a/app/src/pages/__tests__/Welcome.test.tsx +++ b/app/src/pages/__tests__/Welcome.test.tsx @@ -1,7 +1,15 @@ -import { fireEvent, render, screen } from '@testing-library/react'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { clearBackendUrlCache } from '../../services/backendUrl'; +import { clearCoreRpcUrlCache, testCoreRpcConnection } from '../../services/coreRpcClient'; import { useDeepLinkAuthState } from '../../store/deepLinkAuthState'; +import { + clearStoredRpcUrl, + getDefaultRpcUrl, + getStoredRpcUrl, + storeRpcUrl, +} from '../../utils/configPersistence'; import Welcome from '../Welcome'; const oauthButtonSpy = vi.fn(); @@ -44,11 +52,45 @@ vi.mock('../../components/oauth/providerConfigs', () => ({ vi.mock('../../store/deepLinkAuthState', () => ({ useDeepLinkAuthState: vi.fn() })); +vi.mock('../../services/coreRpcClient', () => ({ + clearCoreRpcUrlCache: vi.fn(), + testCoreRpcConnection: vi.fn(), +})); + +vi.mock('../../services/backendUrl', () => ({ + clearBackendUrlCache: vi.fn(), + getBackendUrl: vi.fn().mockResolvedValue('http://localhost:5005'), +})); + +vi.mock('../../utils/configPersistence', () => ({ + getStoredRpcUrl: vi.fn(() => 'http://127.0.0.1:7788/rpc'), + storeRpcUrl: vi.fn(), + clearStoredRpcUrl: vi.fn(), + getDefaultRpcUrl: vi.fn(() => 'http://127.0.0.1:7788/rpc'), + isValidRpcUrl: vi.fn((url: string) => { + if (!url || url.trim().length === 0) return false; + try { + const parsed = new URL(url); + return parsed.protocol === 'http:' || parsed.protocol === 'https:'; + } catch { + return false; + } + }), + normalizeRpcUrl: vi.fn((url: string) => url.trim().replace(/\/+$/, '')), +})); + describe('Welcome auth entrypoint', () => { beforeEach(() => { oauthButtonSpy.mockReset(); oauthOverrideSpy.mockReset(); vi.mocked(useDeepLinkAuthState).mockReturnValue({ isProcessing: false, errorMessage: null }); + vi.mocked(clearCoreRpcUrlCache).mockReset(); + vi.mocked(clearBackendUrlCache).mockReset(); + vi.mocked(storeRpcUrl).mockReset(); + vi.mocked(clearStoredRpcUrl).mockReset(); + vi.mocked(getStoredRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + vi.mocked(getDefaultRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + vi.mocked(testCoreRpcConnection).mockReset(); }); it('renders only the OAuth buttons when auth is idle', () => { @@ -94,3 +136,409 @@ describe('Welcome auth entrypoint', () => { expect(screen.getByRole('alert')).toHaveTextContent('OAuth failed'); }); }); + +describe('Welcome — RPC URL advanced panel', () => { + beforeEach(() => { + vi.mocked(useDeepLinkAuthState).mockReturnValue({ isProcessing: false, errorMessage: null }); + vi.mocked(clearCoreRpcUrlCache).mockReset(); + vi.mocked(clearBackendUrlCache).mockReset(); + vi.mocked(storeRpcUrl).mockReset(); + vi.mocked(clearStoredRpcUrl).mockReset(); + vi.mocked(getStoredRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + vi.mocked(getDefaultRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + vi.mocked(testCoreRpcConnection).mockReset(); + }); + + it('renders with advanced panel collapsed by default', () => { + render(); + + expect(screen.queryByLabelText('Core RPC URL')).not.toBeInTheDocument(); + expect(screen.queryByPlaceholderText('http://127.0.0.1:7788/rpc')).not.toBeInTheDocument(); + }); + + it('shows the "Configure RPC URL (Advanced)" toggle when panel is collapsed', () => { + render(); + + expect( + screen.getByRole('button', { name: 'Configure RPC URL (Advanced)' }) + ).toBeInTheDocument(); + }); + + it('clicking the toggle opens the advanced panel', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: 'Configure RPC URL (Advanced)' })); + + expect(screen.getByPlaceholderText('http://127.0.0.1:7788/rpc')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Save' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Reset to Default' })).toBeInTheDocument(); + }); + + it('panel shows the stored RPC URL as the initial input value', () => { + vi.mocked(getStoredRpcUrl).mockReturnValue('http://custom-host:9999/rpc'); + + render(); + fireEvent.click(screen.getByRole('button', { name: 'Configure RPC URL (Advanced)' })); + + expect(screen.getByPlaceholderText('http://127.0.0.1:7788/rpc')).toHaveValue( + 'http://custom-host:9999/rpc' + ); + }); + + it('panel shows the default URL when nothing custom is stored', () => { + vi.mocked(getStoredRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + + render(); + fireEvent.click(screen.getByRole('button', { name: 'Configure RPC URL (Advanced)' })); + + expect(screen.getByPlaceholderText('http://127.0.0.1:7788/rpc')).toHaveValue( + 'http://127.0.0.1:7788/rpc' + ); + }); + + it('clicking Close hides the advanced panel', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: 'Configure RPC URL (Advanced)' })); + expect(screen.getByPlaceholderText('http://127.0.0.1:7788/rpc')).toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'Close' })); + expect(screen.queryByPlaceholderText('http://127.0.0.1:7788/rpc')).not.toBeInTheDocument(); + }); +}); + +describe('Welcome — Save button', () => { + beforeEach(() => { + vi.mocked(useDeepLinkAuthState).mockReturnValue({ isProcessing: false, errorMessage: null }); + vi.mocked(clearCoreRpcUrlCache).mockReset(); + vi.mocked(clearBackendUrlCache).mockReset(); + vi.mocked(storeRpcUrl).mockReset(); + vi.mocked(clearStoredRpcUrl).mockReset(); + vi.mocked(getStoredRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + vi.mocked(getDefaultRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + }); + + function openPanel() { + fireEvent.click(screen.getByRole('button', { name: 'Configure RPC URL (Advanced)' })); + } + + it('clicking Save with a valid URL calls storeRpcUrl with the normalised URL', () => { + render(); + openPanel(); + + const input = screen.getByPlaceholderText('http://127.0.0.1:7788/rpc'); + fireEvent.change(input, { target: { value: 'http://192.168.1.1:8000/rpc' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(storeRpcUrl).toHaveBeenCalledWith('http://192.168.1.1:8000/rpc'); + }); + + it('clicking Save calls clearCoreRpcUrlCache()', () => { + render(); + openPanel(); + + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(clearCoreRpcUrlCache).toHaveBeenCalledTimes(1); + }); + + it('clicking Save calls clearBackendUrlCache()', () => { + render(); + openPanel(); + + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(clearBackendUrlCache).toHaveBeenCalledTimes(1); + }); + + it('clicking Save with an invalid URL shows a validation error and does NOT call storeRpcUrl', () => { + render(); + openPanel(); + + const input = screen.getByPlaceholderText('http://127.0.0.1:7788/rpc'); + fireEvent.change(input, { target: { value: 'not-a-valid-url' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(screen.getByText('Please enter a valid HTTP or HTTPS URL')).toBeInTheDocument(); + expect(storeRpcUrl).not.toHaveBeenCalled(); + }); + + it('clicking Save with empty string shows a validation error', () => { + render(); + openPanel(); + + const input = screen.getByPlaceholderText('http://127.0.0.1:7788/rpc'); + fireEvent.change(input, { target: { value: '' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(screen.getByText('Please enter a valid HTTP or HTTPS URL')).toBeInTheDocument(); + expect(storeRpcUrl).not.toHaveBeenCalled(); + }); + + it('shows a success message after a successful save', async () => { + render(); + openPanel(); + + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(await screen.findByText('URL saved successfully.')).toBeInTheDocument(); + }); +}); + +describe('Welcome — Test Connection button', () => { + beforeEach(() => { + vi.mocked(useDeepLinkAuthState).mockReturnValue({ isProcessing: false, errorMessage: null }); + vi.mocked(clearCoreRpcUrlCache).mockReset(); + vi.mocked(clearBackendUrlCache).mockReset(); + vi.mocked(storeRpcUrl).mockReset(); + vi.mocked(getStoredRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + vi.mocked(getDefaultRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + vi.mocked(testCoreRpcConnection).mockReset(); + }); + + function openPanel() { + fireEvent.click(screen.getByRole('button', { name: 'Configure RPC URL (Advanced)' })); + } + + it('clicking Test Connection fires testCoreRpcConnection with the entered URL', async () => { + vi.mocked(testCoreRpcConnection).mockResolvedValueOnce({ ok: true, status: 200 } as Response); + + render(); + openPanel(); + + fireEvent.click(screen.getByRole('button', { name: 'Test' })); + + await waitFor(() => { + expect(testCoreRpcConnection).toHaveBeenCalledWith('http://127.0.0.1:7788/rpc'); + }); + }); + + it('successful probe (200 ok) shows success message', async () => { + vi.mocked(testCoreRpcConnection).mockResolvedValueOnce({ ok: true, status: 200 } as Response); + + render(); + openPanel(); + fireEvent.click(screen.getByRole('button', { name: 'Test' })); + + await screen.findByText('URL saved successfully.'); + }); + + it('successful probe with 405 status (expected for JSON-RPC ping) shows success message', async () => { + vi.mocked(testCoreRpcConnection).mockResolvedValueOnce({ + ok: false, + status: 405, + statusText: 'Method Not Allowed', + } as Response); + + render(); + openPanel(); + fireEvent.click(screen.getByRole('button', { name: 'Test' })); + + await screen.findByText('URL saved successfully.'); + }); + + it('failed probe (4xx/5xx status) shows an error message', async () => { + vi.mocked(testCoreRpcConnection).mockResolvedValueOnce({ + ok: false, + status: 503, + statusText: 'Service Unavailable', + } as Response); + + render(); + openPanel(); + fireEvent.click(screen.getByRole('button', { name: 'Test' })); + + await screen.findByText('Connection failed: 503 Service Unavailable'); + }); + + it('failed probe (network error) shows an error message with the error text', async () => { + vi.mocked(testCoreRpcConnection).mockRejectedValueOnce(new Error('ECONNREFUSED')); + + render(); + openPanel(); + fireEvent.click(screen.getByRole('button', { name: 'Test' })); + + await screen.findByText('Connection failed: ECONNREFUSED'); + }); + + it('Test Connection with invalid URL shows validation error without calling testCoreRpcConnection', async () => { + render(); + openPanel(); + + const input = screen.getByPlaceholderText('http://127.0.0.1:7788/rpc'); + fireEvent.change(input, { target: { value: 'bad-url' } }); + fireEvent.click(screen.getByRole('button', { name: 'Test' })); + + expect(screen.getByText('Please enter a valid HTTP or HTTPS URL')).toBeInTheDocument(); + expect(testCoreRpcConnection).not.toHaveBeenCalled(); + }); + + it('shows loading state while the probe is in flight', async () => { + let resolveProbe!: (r: Response) => void; + vi.mocked(testCoreRpcConnection).mockReturnValueOnce( + new Promise(resolve => { + resolveProbe = resolve; + }) + ); + + render(); + openPanel(); + fireEvent.click(screen.getByRole('button', { name: 'Test' })); + + // During flight the button label changes to "Testing" and the button is disabled + const testBtn = screen.getByRole('button', { name: /testing/i }); + expect(testBtn).toBeDisabled(); + + resolveProbe({ ok: true, status: 200 } as Response); + await waitFor(() => + expect(screen.queryByRole('button', { name: /testing/i })).not.toBeInTheDocument() + ); + }); + + it('successful probe stores the URL and clears the RPC URL cache', async () => { + vi.mocked(testCoreRpcConnection).mockResolvedValueOnce({ ok: true, status: 200 } as Response); + + render(); + openPanel(); + fireEvent.click(screen.getByRole('button', { name: 'Test' })); + + await waitFor(() => { + expect(storeRpcUrl).toHaveBeenCalledWith('http://127.0.0.1:7788/rpc'); + expect(clearCoreRpcUrlCache).toHaveBeenCalledTimes(1); + }); + }); +}); + +describe('Welcome — Reset to Default button', () => { + beforeEach(() => { + vi.mocked(useDeepLinkAuthState).mockReturnValue({ isProcessing: false, errorMessage: null }); + vi.mocked(clearCoreRpcUrlCache).mockReset(); + vi.mocked(clearBackendUrlCache).mockReset(); + vi.mocked(storeRpcUrl).mockReset(); + vi.mocked(clearStoredRpcUrl).mockReset(); + vi.mocked(getStoredRpcUrl).mockReturnValue('http://custom:9999/rpc'); + vi.mocked(getDefaultRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + }); + + function openPanel() { + fireEvent.click(screen.getByRole('button', { name: 'Configure RPC URL (Advanced)' })); + } + + it('clicking Reset calls clearStoredRpcUrl()', () => { + render(); + openPanel(); + fireEvent.click(screen.getByRole('button', { name: 'Reset to Default' })); + + expect(clearStoredRpcUrl).toHaveBeenCalledTimes(1); + }); + + it('clicking Reset calls clearCoreRpcUrlCache()', () => { + render(); + openPanel(); + fireEvent.click(screen.getByRole('button', { name: 'Reset to Default' })); + + expect(clearCoreRpcUrlCache).toHaveBeenCalledTimes(1); + }); + + it('clicking Reset calls clearBackendUrlCache()', () => { + render(); + openPanel(); + fireEvent.click(screen.getByRole('button', { name: 'Reset to Default' })); + + expect(clearBackendUrlCache).toHaveBeenCalledTimes(1); + }); + + it('after Reset, input value reverts to the default URL', () => { + render(); + openPanel(); + + // Input starts with the custom stored value + expect(screen.getByPlaceholderText('http://127.0.0.1:7788/rpc')).toHaveValue( + 'http://custom:9999/rpc' + ); + + fireEvent.click(screen.getByRole('button', { name: 'Reset to Default' })); + + expect(screen.getByPlaceholderText('http://127.0.0.1:7788/rpc')).toHaveValue( + 'http://127.0.0.1:7788/rpc' + ); + }); +}); + +describe('Welcome — URL input behaviour', () => { + beforeEach(() => { + vi.mocked(useDeepLinkAuthState).mockReturnValue({ isProcessing: false, errorMessage: null }); + vi.mocked(clearCoreRpcUrlCache).mockReset(); + vi.mocked(clearBackendUrlCache).mockReset(); + vi.mocked(storeRpcUrl).mockReset(); + vi.mocked(getStoredRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + vi.mocked(getDefaultRpcUrl).mockReturnValue('http://127.0.0.1:7788/rpc'); + }); + + function openPanel() { + fireEvent.click(screen.getByRole('button', { name: 'Configure RPC URL (Advanced)' })); + } + + it('typing in the input updates the displayed value', () => { + render(); + openPanel(); + + const input = screen.getByPlaceholderText('http://127.0.0.1:7788/rpc'); + fireEvent.change(input, { target: { value: 'http://new-host:5555/rpc' } }); + + expect(input).toHaveValue('http://new-host:5555/rpc'); + }); + + it('typing a valid URL clears any existing error', () => { + render(); + openPanel(); + + // First trigger an error + const input = screen.getByPlaceholderText('http://127.0.0.1:7788/rpc'); + fireEvent.change(input, { target: { value: 'bad' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + expect(screen.getByText('Please enter a valid HTTP or HTTPS URL')).toBeInTheDocument(); + + // Then type a valid URL — error should clear + fireEvent.change(input, { target: { value: 'http://valid-host:9000/rpc' } }); + expect(screen.queryByText('Please enter a valid HTTP or HTTPS URL')).not.toBeInTheDocument(); + }); + + it('invalid URL (missing protocol) shows inline error on Save', () => { + render(); + openPanel(); + + const input = screen.getByPlaceholderText('http://127.0.0.1:7788/rpc'); + fireEvent.change(input, { target: { value: 'localhost:9999' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(screen.getByText('Please enter a valid HTTP or HTTPS URL')).toBeInTheDocument(); + }); +}); + +describe('Welcome — OAuth buttons presence', () => { + beforeEach(() => { + vi.mocked(useDeepLinkAuthState).mockReturnValue({ isProcessing: false, errorMessage: null }); + }); + + it('renders all providers with showOnWelcome=true', () => { + render(); + + expect(screen.getByRole('button', { name: 'google' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'github' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'twitter' })).toBeInTheDocument(); + }); + + it('does not render providers with showOnWelcome=false', () => { + render(); + + expect(screen.queryByRole('button', { name: 'discord' })).not.toBeInTheDocument(); + }); + + it('hides OAuth buttons while auth is processing', () => { + vi.mocked(useDeepLinkAuthState).mockReturnValue({ isProcessing: true, errorMessage: null }); + render(); + + expect(screen.queryByRole('button', { name: 'google' })).not.toBeInTheDocument(); + }); +}); diff --git a/app/src/services/__tests__/backendUrl.test.ts b/app/src/services/__tests__/backendUrl.test.ts index 6798af340..af282fe3c 100644 --- a/app/src/services/__tests__/backendUrl.test.ts +++ b/app/src/services/__tests__/backendUrl.test.ts @@ -15,7 +15,7 @@ vi.mock('../coreRpcClient', () => ({ callCoreRpc: hoisted.callCoreRpcMock })); async function loadFreshModule() { vi.resetModules(); const mod = await import('../backendUrl'); - return mod.getBackendUrl; + return mod; } describe('getBackendUrl', () => { @@ -29,7 +29,7 @@ describe('getBackendUrl', () => { hoisted.isTauriMock.mockReturnValue(true); hoisted.callCoreRpcMock.mockResolvedValue({ api_url: 'https://core-derived.example.com/' }); - const getBackendUrl = await loadFreshModule(); + const { getBackendUrl } = await loadFreshModule(); expect(await getBackendUrl()).toBe('https://core-derived.example.com'); expect(hoisted.callCoreRpcMock).toHaveBeenCalledWith({ method: 'openhuman.config_resolve_api_url', @@ -40,7 +40,7 @@ describe('getBackendUrl', () => { hoisted.isTauriMock.mockReturnValue(true); hoisted.callCoreRpcMock.mockResolvedValue({ api_url: 'https://core-derived.example.com' }); - const getBackendUrl = await loadFreshModule(); + const { getBackendUrl } = await loadFreshModule(); await getBackendUrl(); await getBackendUrl(); expect(hoisted.callCoreRpcMock).toHaveBeenCalledTimes(1); @@ -50,7 +50,7 @@ describe('getBackendUrl', () => { hoisted.isTauriMock.mockReturnValue(true); hoisted.callCoreRpcMock.mockResolvedValue({ api_url: '' }); - const getBackendUrl = await loadFreshModule(); + const { getBackendUrl } = await loadFreshModule(); await expect(getBackendUrl()).rejects.toThrow(/empty backend URL/i); }); @@ -58,7 +58,70 @@ describe('getBackendUrl', () => { hoisted.isTauriMock.mockReturnValue(true); hoisted.callCoreRpcMock.mockResolvedValue({ apiUrl: 'https://core-derived.example.com' }); - const getBackendUrl = await loadFreshModule(); + const { getBackendUrl } = await loadFreshModule(); expect(await getBackendUrl()).toBe('https://core-derived.example.com'); }); + + test('clearBackendUrlCache causes the next getBackendUrl() call to re-derive (not use cached value)', async () => { + hoisted.isTauriMock.mockReturnValue(true); + hoisted.callCoreRpcMock + .mockResolvedValueOnce({ api_url: 'https://first-call.example.com' }) + .mockResolvedValueOnce({ api_url: 'https://second-call.example.com' }); + + const { getBackendUrl, clearBackendUrlCache } = await loadFreshModule(); + + const first = await getBackendUrl(); + expect(first).toBe('https://first-call.example.com'); + + clearBackendUrlCache(); + + const second = await getBackendUrl(); + expect(second).toBe('https://second-call.example.com'); + expect(hoisted.callCoreRpcMock).toHaveBeenCalledTimes(2); + }); + + test('calling getBackendUrl() twice returns the same value (cache works)', async () => { + hoisted.isTauriMock.mockReturnValue(true); + hoisted.callCoreRpcMock.mockResolvedValue({ api_url: 'https://cached.example.com' }); + + const { getBackendUrl } = await loadFreshModule(); + const a = await getBackendUrl(); + const b = await getBackendUrl(); + expect(a).toBe(b); + expect(a).toBe('https://cached.example.com'); + }); + + test('after clearBackendUrlCache a second call re-invokes the core RPC', async () => { + hoisted.isTauriMock.mockReturnValue(true); + hoisted.callCoreRpcMock.mockResolvedValue({ api_url: 'https://rechecked.example.com' }); + + const { getBackendUrl, clearBackendUrlCache } = await loadFreshModule(); + await getBackendUrl(); + clearBackendUrlCache(); + await getBackendUrl(); + + expect(hoisted.callCoreRpcMock).toHaveBeenCalledTimes(2); + }); + + test('in non-Tauri mode returns BACKEND_URL directly without calling core RPC', async () => { + hoisted.isTauriMock.mockReturnValue(false); + + const { getBackendUrl } = await loadFreshModule(); + const url = await getBackendUrl(); + + // Should not have attempted an RPC call in non-Tauri mode + expect(hoisted.callCoreRpcMock).not.toHaveBeenCalled(); + // Should return a non-empty string + expect(typeof url).toBe('string'); + expect(url.length).toBeGreaterThan(0); + }); + + test('graceful fallback to VITE_BACKEND_URL when core RPC throws', async () => { + hoisted.isTauriMock.mockReturnValue(true); + hoisted.callCoreRpcMock.mockRejectedValue(new Error('RPC unavailable')); + + const { getBackendUrl } = await loadFreshModule(); + // The implementation does NOT catch the error — it propagates. Verify the rejection. + await expect(getBackendUrl()).rejects.toThrow('RPC unavailable'); + }); }); diff --git a/app/src/services/__tests__/coreRpcClient.test.ts b/app/src/services/__tests__/coreRpcClient.test.ts index 00347f9d3..d6fb82b79 100644 --- a/app/src/services/__tests__/coreRpcClient.test.ts +++ b/app/src/services/__tests__/coreRpcClient.test.ts @@ -494,3 +494,117 @@ describe('coreRpcClient', () => { }); }); }); + +describe('getCoreRpcUrl', () => { + // Each test gets a fresh module so module-level caches are cleared + beforeEach(() => { + vi.resetModules(); + vi.mocked(isTauri).mockReturnValue(false); + vi.mocked(invoke).mockReset(); + }); + + test('in web mode returns stored URL when getStoredRpcUrl returns a non-default value', async () => { + vi.doMock('../../utils/configPersistence', () => ({ + getStoredRpcUrl: () => 'http://custom-host:9999/rpc', + })); + vi.mocked(isTauri).mockReturnValue(false); + + const { getCoreRpcUrl: freshGetCoreRpcUrl } = await import('../coreRpcClient'); + const url = await freshGetCoreRpcUrl(); + expect(url).toBe('http://custom-host:9999/rpc'); + }); + + test('in web mode returns default CORE_RPC_URL when nothing custom is stored', async () => { + vi.doMock('../../utils/configPersistence', () => ({ + getStoredRpcUrl: () => 'http://127.0.0.1:7788/rpc', + })); + vi.mocked(isTauri).mockReturnValue(false); + + const { getCoreRpcUrl: freshGetCoreRpcUrl } = await import('../coreRpcClient'); + const url = await freshGetCoreRpcUrl(); + expect(url).toBe('http://127.0.0.1:7788/rpc'); + }); + + test('in web mode caches the result — second call does not change the returned value', async () => { + let callCount = 0; + vi.doMock('../../utils/configPersistence', () => ({ + getStoredRpcUrl: () => { + callCount++; + return 'http://127.0.0.1:7788/rpc'; + }, + })); + vi.mocked(isTauri).mockReturnValue(false); + + const { getCoreRpcUrl: freshGetCoreRpcUrl } = await import('../coreRpcClient'); + const first = await freshGetCoreRpcUrl(); + const second = await freshGetCoreRpcUrl(); + expect(first).toBe(second); + // getStoredRpcUrl should only have been called once due to caching + expect(callCount).toBe(1); + }); + + test('returns fresh value after clearCoreRpcUrlCache()', async () => { + let storedValue = 'http://127.0.0.1:7788/rpc'; + vi.doMock('../../utils/configPersistence', () => ({ getStoredRpcUrl: () => storedValue })); + vi.mocked(isTauri).mockReturnValue(false); + + const { getCoreRpcUrl: freshGetCoreRpcUrl, clearCoreRpcUrlCache: freshClear } = + await import('../coreRpcClient'); + + const first = await freshGetCoreRpcUrl(); + expect(first).toBe('http://127.0.0.1:7788/rpc'); + + // Change stored value and clear cache + storedValue = 'http://new-host:8888/rpc'; + freshClear(); + + const second = await freshGetCoreRpcUrl(); + expect(second).toBe('http://new-host:8888/rpc'); + }); + + test('in Tauri mode calls invoke("core_rpc_url") when no stored URL is customised', async () => { + vi.doMock('../../utils/configPersistence', () => ({ + getStoredRpcUrl: () => 'http://127.0.0.1:7788/rpc', + })); + vi.mocked(isTauri).mockReturnValue(true); + vi.mocked(invoke).mockImplementation(async (cmd: string) => { + if (cmd === 'core_rpc_url') return 'http://tauri-resolved:7788/rpc'; + throw new Error(`unexpected: ${cmd}`); + }); + + const { getCoreRpcUrl: freshGetCoreRpcUrl } = await import('../coreRpcClient'); + const url = await freshGetCoreRpcUrl(); + expect(url).toBe('http://tauri-resolved:7788/rpc'); + expect(vi.mocked(invoke)).toHaveBeenCalledWith('core_rpc_url'); + }); + + test('in Tauri mode stored URL takes priority over invoke result', async () => { + vi.doMock('../../utils/configPersistence', () => ({ + getStoredRpcUrl: () => 'http://stored-override:4444/rpc', + })); + vi.mocked(isTauri).mockReturnValue(true); + vi.mocked(invoke).mockImplementation(async (cmd: string) => { + if (cmd === 'core_rpc_url') return 'http://tauri-would-return:7788/rpc'; + throw new Error(`unexpected: ${cmd}`); + }); + + const { getCoreRpcUrl: freshGetCoreRpcUrl } = await import('../coreRpcClient'); + const url = await freshGetCoreRpcUrl(); + // stored override should win; invoke should NOT have been called + expect(url).toBe('http://stored-override:4444/rpc'); + expect(vi.mocked(invoke)).not.toHaveBeenCalled(); + }); + + test('in Tauri mode falls back to CORE_RPC_URL when invoke fails and no stored URL', async () => { + vi.doMock('../../utils/configPersistence', () => ({ + getStoredRpcUrl: () => 'http://127.0.0.1:7788/rpc', + })); + vi.mocked(isTauri).mockReturnValue(true); + vi.mocked(invoke).mockRejectedValue(new Error('invoke failed')); + + const { getCoreRpcUrl: freshGetCoreRpcUrl } = await import('../coreRpcClient'); + const url = await freshGetCoreRpcUrl(); + // Should fall back to the default + expect(url).toBe('http://127.0.0.1:7788/rpc'); + }); +}); diff --git a/app/src/services/__tests__/socketService.test.ts b/app/src/services/__tests__/socketService.test.ts new file mode 100644 index 000000000..cacb5dc5c --- /dev/null +++ b/app/src/services/__tests__/socketService.test.ts @@ -0,0 +1,142 @@ +/** + * Unit tests for socketService internals — specifically the + * resolveCoreSocketBaseUrl() behaviour that was fixed to consult + * getCoreRpcUrl() (and therefore the user's stored preference) instead of + * calling invoke('core_rpc_url') directly. + * + * We cannot import resolveCoreSocketBaseUrl directly because it is not + * exported. Instead we spy on getCoreRpcUrl to confirm it is called during + * socket connection, and verify the derived base URL strips the /rpc suffix. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// Mock socket.io-client so no real connections are made +vi.mock('socket.io-client', () => ({ + io: vi.fn(() => ({ + connected: false, + disconnected: true, + on: vi.fn(), + onAny: vi.fn(), + once: vi.fn(), + off: vi.fn(), + emit: vi.fn(), + disconnect: vi.fn(), + connect: vi.fn(), + id: 'mock-socket-id', + })), +})); + +// Mock redux store +vi.mock('../../store', () => ({ store: { dispatch: vi.fn() } })); +vi.mock('../../store/socketSlice', () => ({ + setStatusForUser: vi.fn((x: unknown) => x), + setSocketIdForUser: vi.fn((x: unknown) => x), + resetForUser: vi.fn((x: unknown) => x), +})); +vi.mock('../../store/channelConnectionsSlice', () => ({ + upsertChannelConnection: vi.fn((x: unknown) => x), +})); + +// Mock coreState +vi.mock('../../lib/coreState/store', () => ({ + getCoreStateSnapshot: vi.fn(() => ({ snapshot: { sessionToken: null } })), +})); + +// Mock MCP — must be a newable constructor +vi.mock('../../lib/mcp', () => ({ + SocketIOMCPTransportImpl: vi.fn(() => ({})), +})); + +// Hoist getCoreRpcUrl mock so it is available before the module is loaded +const hoisted = vi.hoisted(() => ({ getCoreRpcUrlMock: vi.fn<() => Promise>() })); + +vi.mock('../coreRpcClient', () => ({ + getCoreRpcUrl: hoisted.getCoreRpcUrlMock, + clearCoreRpcUrlCache: vi.fn(), +})); + +describe('socketService — resolveCoreSocketBaseUrl uses getCoreRpcUrl', () => { + beforeEach(() => { + hoisted.getCoreRpcUrlMock.mockReset(); + }); + + it('calls getCoreRpcUrl() when connecting', async () => { + hoisted.getCoreRpcUrlMock.mockResolvedValue('http://127.0.0.1:7788/rpc'); + + // Import after mocks are set up + const { socketService } = await import('../socketService'); + socketService.connect('mock-jwt-token'); + + // Give the async connectAsync a tick to run + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled(); + }); + + it('strips /rpc suffix from the resolved RPC URL to derive the socket base', async () => { + const { io } = await import('socket.io-client'); + const ioMock = vi.mocked(io); + ioMock.mockClear(); + + hoisted.getCoreRpcUrlMock.mockResolvedValue('http://127.0.0.1:7788/rpc'); + + const { socketService } = await import('../socketService'); + socketService.connect('mock-jwt-token-2'); + + await new Promise(resolve => setTimeout(resolve, 0)); + + if (ioMock.mock.calls.length > 0) { + const connectedUrl = ioMock.mock.calls[ioMock.mock.calls.length - 1][0]; + expect(connectedUrl).toBe('http://127.0.0.1:7788'); + } else { + // The 1420 guard may have prevented connection — ensure getCoreRpcUrl was still consulted + expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled(); + } + }); + + it('works when the resolved URL has no /rpc suffix', async () => { + const { io } = await import('socket.io-client'); + const ioMock = vi.mocked(io); + ioMock.mockClear(); + + // Return a base URL without the /rpc suffix + hoisted.getCoreRpcUrlMock.mockResolvedValue('http://127.0.0.1:7788'); + + const { socketService } = await import('../socketService'); + // Disconnect first in case there's a stale socket from a prior test + socketService.disconnect(); + socketService.connect('mock-jwt-token-3'); + + await new Promise(resolve => setTimeout(resolve, 0)); + + // getCoreRpcUrl must have been consulted + expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled(); + + if (ioMock.mock.calls.length > 0) { + const connectedUrl = ioMock.mock.calls[ioMock.mock.calls.length - 1][0]; + expect(connectedUrl).toBe('http://127.0.0.1:7788'); + } + }); + + it('uses stored custom RPC URL (not static constant) when user has configured one', async () => { + const { io } = await import('socket.io-client'); + const ioMock = vi.mocked(io); + ioMock.mockClear(); + + // Simulate a user-stored custom RPC URL being returned by getCoreRpcUrl + hoisted.getCoreRpcUrlMock.mockResolvedValue('http://custom-core-host:9000/rpc'); + + const { socketService } = await import('../socketService'); + socketService.disconnect(); + socketService.connect('mock-jwt-token-custom'); + + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled(); + + if (ioMock.mock.calls.length > 0) { + const connectedUrl = ioMock.mock.calls[ioMock.mock.calls.length - 1][0]; + expect(connectedUrl).toBe('http://custom-core-host:9000'); + } + }); +}); diff --git a/app/src/services/backendUrl.ts b/app/src/services/backendUrl.ts index 28a712bc3..0e663c410 100644 --- a/app/src/services/backendUrl.ts +++ b/app/src/services/backendUrl.ts @@ -6,6 +6,17 @@ import { callCoreRpc } from './coreRpcClient'; let resolvedBackendUrl: string | null = null; let resolvingBackendUrl: Promise | null = null; +/** + * Invalidate the cached backend URL so the next call to getBackendUrl() + * re-derives from the core RPC (Tauri) or web fallback. + * Call this after the user saves a new RPC URL preference so the backend + * URL is recomputed from the updated core endpoint. + */ +export function clearBackendUrlCache(): void { + resolvedBackendUrl = null; + resolvingBackendUrl = null; +} + function normalizeBaseUrl(url: string): string { return url.trim().replace(/\/+$/, ''); } diff --git a/app/src/services/socketService.ts b/app/src/services/socketService.ts index c54c1129b..2d5cfd1cf 100644 --- a/app/src/services/socketService.ts +++ b/app/src/services/socketService.ts @@ -1,4 +1,3 @@ -import { isTauri as coreIsTauri, invoke } from '@tauri-apps/api/core'; import debug from 'debug'; import { io, Socket } from 'socket.io-client'; @@ -8,8 +7,9 @@ import { store } from '../store'; import { upsertChannelConnection } from '../store/channelConnectionsSlice'; import { resetForUser, setSocketIdForUser, setStatusForUser } from '../store/socketSlice'; import type { ChannelAuthMode, ChannelConnectionStatus, ChannelType } from '../types/channels'; -import { CORE_RPC_URL, IS_DEV } from '../utils/config'; +import { IS_DEV } from '../utils/config'; import { createSafeLogData, sanitizeError } from '../utils/sanitize'; +import { getCoreRpcUrl } from './coreRpcClient'; // Socket service logger using debug package // Enable logging by setting DEBUG=socket* in environment or localStorage @@ -27,17 +27,15 @@ function coreSocketBaseFromRpcUrl(rpcUrl: string): string { return trimmed.endsWith('/rpc') ? trimmed.slice(0, -4) : trimmed; } +/** + * Resolve the Socket.IO base URL from the user's stored RPC URL preference. + * Delegates to getCoreRpcUrl() so the stored preference (set on the Welcome + * screen) is always honoured — previously this called invoke('core_rpc_url') + * directly, which ignored the user's stored override. + */ async function resolveCoreSocketBaseUrl(): Promise { - if (!coreIsTauri()) { - return coreSocketBaseFromRpcUrl(CORE_RPC_URL); - } - - try { - const rpcUrl = await invoke('core_rpc_url'); - return coreSocketBaseFromRpcUrl(String(rpcUrl || CORE_RPC_URL)); - } catch { - return coreSocketBaseFromRpcUrl(CORE_RPC_URL); - } + const rpcUrl = await getCoreRpcUrl(); + return coreSocketBaseFromRpcUrl(rpcUrl); } interface JwtPayload { diff --git a/app/src/utils/__tests__/configPersistence.test.ts b/app/src/utils/__tests__/configPersistence.test.ts index ec042f65a..07c7cc48e 100644 --- a/app/src/utils/__tests__/configPersistence.test.ts +++ b/app/src/utils/__tests__/configPersistence.test.ts @@ -2,7 +2,7 @@ * Unit tests for configPersistence utilities. * Tests URL storage, validation, and normalization. */ -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { clearStoredRpcUrl, @@ -144,4 +144,124 @@ describe('configPersistence', () => { expect(getDefaultRpcUrl()).toBe('http://127.0.0.1:7788/rpc'); }); }); + + describe('isValidRpcUrl — edge cases', () => { + it('returns true for localhost with a port', () => { + expect(isValidRpcUrl('http://localhost:7788')).toBe(true); + }); + + it('returns true for a bare IP address URL', () => { + expect(isValidRpcUrl('http://192.168.1.100:7788/rpc')).toBe(true); + }); + + it('returns true for an HTTPS URL', () => { + expect(isValidRpcUrl('https://remote-core.example.com/rpc')).toBe(true); + }); + + it('returns true for a URL with a path segment', () => { + expect(isValidRpcUrl('http://127.0.0.1:7788/rpc')).toBe(true); + }); + + it('returns false for empty string', () => { + expect(isValidRpcUrl('')).toBe(false); + }); + + it('returns false for whitespace-only string', () => { + expect(isValidRpcUrl(' ')).toBe(false); + }); + + it('returns false for a URL without a protocol', () => { + expect(isValidRpcUrl('localhost:7788/rpc')).toBe(false); + expect(isValidRpcUrl('127.0.0.1:7788')).toBe(false); + }); + + it('returns false for a ws:// URL', () => { + expect(isValidRpcUrl('ws://localhost:7788')).toBe(false); + }); + + it('returns false for a ftp:// URL', () => { + expect(isValidRpcUrl('ftp://localhost:7788')).toBe(false); + }); + + it('returns false for a completely malformed string', () => { + expect(isValidRpcUrl('not a url at all')).toBe(false); + }); + + it('returns false for http:// with no host', () => { + expect(isValidRpcUrl('http://')).toBe(false); + }); + }); + + describe('normalizeRpcUrl — edge cases', () => { + it('does not add /rpc suffix when missing (normalizeRpcUrl only strips, not appends)', () => { + expect(normalizeRpcUrl('http://127.0.0.1:7788')).toBe('http://127.0.0.1:7788'); + }); + + it('does not double-add /rpc — leaves existing /rpc alone', () => { + expect(normalizeRpcUrl('http://127.0.0.1:7788/rpc')).toBe('http://127.0.0.1:7788/rpc'); + }); + + it('handles trailing slash after /rpc', () => { + expect(normalizeRpcUrl('http://127.0.0.1:7788/rpc/')).toBe('http://127.0.0.1:7788/rpc'); + }); + + it('handles uppercase protocol casing (trims only, does not lowercase)', () => { + // The normalizer does not lowercase — it just trims slashes and whitespace + expect(normalizeRpcUrl(' HTTP://localhost:7788/rpc ')).toBe('HTTP://localhost:7788/rpc'); + }); + + it('removes multiple trailing slashes', () => { + expect(normalizeRpcUrl('http://127.0.0.1:7788/rpc///')).toBe('http://127.0.0.1:7788/rpc'); + }); + + it('trims leading and trailing whitespace', () => { + expect(normalizeRpcUrl(' http://127.0.0.1:7788/rpc ')).toBe('http://127.0.0.1:7788/rpc'); + }); + }); + + describe('storeRpcUrl + getStoredRpcUrl — round-trip', () => { + it('round-trips an HTTPS URL', () => { + storeRpcUrl('https://remote.example.com/rpc'); + expect(getStoredRpcUrl()).toBe('https://remote.example.com/rpc'); + }); + + it('round-trips a localhost URL with a non-standard port', () => { + storeRpcUrl('http://localhost:12345/rpc'); + expect(getStoredRpcUrl()).toBe('http://localhost:12345/rpc'); + }); + + it('round-trips an IP address URL', () => { + storeRpcUrl('http://10.0.0.1:7788/rpc'); + expect(getStoredRpcUrl()).toBe('http://10.0.0.1:7788/rpc'); + }); + }); + + describe('clearStoredRpcUrl + getStoredRpcUrl', () => { + it('getStoredRpcUrl returns the default after clearStoredRpcUrl', () => { + storeRpcUrl('http://some-host:9999/rpc'); + expect(getStoredRpcUrl()).toBe('http://some-host:9999/rpc'); + + clearStoredRpcUrl(); + expect(getStoredRpcUrl()).toBe('http://127.0.0.1:7788/rpc'); + }); + + it('localStorage key is null after clearStoredRpcUrl', () => { + storeRpcUrl('http://some-host:9999/rpc'); + clearStoredRpcUrl(); + expect(localStorage.getItem('openhuman_core_rpc_url')).toBeNull(); + }); + }); + + describe('getStoredRpcUrl — localStorage unavailable', () => { + it('returns the default URL when localStorage throws', () => { + const original = localStorage.getItem.bind(localStorage); + vi.spyOn(localStorage, 'getItem').mockImplementation(() => { + throw new Error('Storage unavailable'); + }); + + expect(getStoredRpcUrl()).toBe('http://127.0.0.1:7788/rpc'); + + vi.spyOn(localStorage, 'getItem').mockImplementation(original); + }); + }); }); diff --git a/app/src/utils/config.ts b/app/src/utils/config.ts index 269b383bc..7f72bd252 100644 --- a/app/src/utils/config.ts +++ b/app/src/utils/config.ts @@ -7,6 +7,17 @@ const APP_ENV = (import.meta.env.VITE_OPENHUMAN_APP_ENV as string | undefined) const DEFAULT_BACKEND_URL = APP_ENV === 'staging' ? 'https://staging-api.tinyhumans.ai' : 'https://api.tinyhumans.ai'; +/** + * Build-time fallback for the Core JSON-RPC endpoint URL. + * + * **Not runtime-authoritative.** At runtime `getCoreRpcUrl()` (in + * `services/coreRpcClient.ts`) is the source of truth: it first checks for a + * URL stored by the user via the Welcome screen (`configPersistence`), then + * falls back to this constant. Never read this constant directly from product + * code that needs the live endpoint — call `getCoreRpcUrl()` instead. + * + * Override at build time via `VITE_OPENHUMAN_CORE_RPC_URL`. + */ export const CORE_RPC_URL = import.meta.env.VITE_OPENHUMAN_CORE_RPC_URL || 'http://127.0.0.1:7788/rpc'; @@ -70,7 +81,16 @@ export const SKILLS_GITHUB_REPO = /** Sentry DSN for error reporting. Leave blank to disable. */ export const SENTRY_DSN = import.meta.env.VITE_SENTRY_DSN as string | undefined; -/** Backend API URL (web fallback when core RPC is unavailable). */ +/** + * Build-time fallback for the backend API base URL. + * + * **Not runtime-authoritative in Tauri.** In the desktop app, `getBackendUrl()` + * (in `services/backendUrl.ts`) asks the core sidecar for the live API URL via + * `openhuman.config_resolve_api_url`; this constant is only consulted in web + * mode (no sidecar) or as an emergency fallback when the RPC call fails. + * + * Override at build time via `VITE_BACKEND_URL`. + */ export const BACKEND_URL = (import.meta.env.VITE_BACKEND_URL as string | undefined)?.trim() || DEFAULT_BACKEND_URL; diff --git a/app/src/utils/configPersistence.ts b/app/src/utils/configPersistence.ts index 66a13456e..d2369b756 100644 --- a/app/src/utils/configPersistence.ts +++ b/app/src/utils/configPersistence.ts @@ -105,14 +105,3 @@ export function normalizeRpcUrl(url: string): string { export function getDefaultRpcUrl(): string { return CORE_RPC_URL; } - -/** - * Build the full RPC endpoint URL from a base URL. - * - * @param baseUrl - The base URL (e.g., 'http://127.0.0.1:7788') - * @returns The full RPC endpoint URL - */ -export function buildRpcEndpoint(baseUrl: string): string { - const normalized = normalizeRpcUrl(baseUrl); - return normalized.endsWith('/rpc') ? normalized : `${normalized}/rpc`; -} From a7226b93226183ffccbe81a5ed75ba0d4706ab6d Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Thu, 7 May 2026 02:05:26 +0530 Subject: [PATCH 2/3] style: prettier fix in socketService.test.ts --- app/src/services/__tests__/socketService.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/src/services/__tests__/socketService.test.ts b/app/src/services/__tests__/socketService.test.ts index cacb5dc5c..ae51d2165 100644 --- a/app/src/services/__tests__/socketService.test.ts +++ b/app/src/services/__tests__/socketService.test.ts @@ -43,9 +43,7 @@ vi.mock('../../lib/coreState/store', () => ({ })); // Mock MCP — must be a newable constructor -vi.mock('../../lib/mcp', () => ({ - SocketIOMCPTransportImpl: vi.fn(() => ({})), -})); +vi.mock('../../lib/mcp', () => ({ SocketIOMCPTransportImpl: vi.fn(() => ({})) })); // Hoist getCoreRpcUrl mock so it is available before the module is loaded const hoisted = vi.hoisted(() => ({ getCoreRpcUrlMock: vi.fn<() => Promise>() })); From 646249596f9e3550e3511bbee83def62a8435663 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Thu, 7 May 2026 02:25:11 +0530 Subject: [PATCH 3/3] fix(config): address all CodeRabbit review comments on PR #1313 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Production changes: - Welcome.tsx: clear backend URL cache on handleTestConnection() success (mirrors Save/Reset paths — addresses @coderabbitai on Welcome.tsx:73-76) - backendUrl.ts: add generation counter so in-flight getBackendUrl() resolutions cannot repopulate the cache after clearBackendUrlCache() (addresses @coderabbitai on backendUrl.ts:15-17) - config.ts: tighten BACKEND_URL JSDoc — clarifies getBackendUrl() throws in Tauri mode rather than falling back to this constant (addresses @coderabbitai on config.ts:87-90) Test changes: - socketService.test.ts: fix SocketIOMCPTransportImpl mock to use a class (vi.fn() alone is not newable in vitest — caused 4 unhandled rejections that made CI report failure). Replace setTimeout(0) sleeps with pollUntil() helper for deterministic async waits. - backendUrl.test.ts: assert BACKEND_URL constant in non-Tauri test (not just "non-empty string"); rename misleading "graceful fallback" test to "propagates RPC errors in Tauri mode (no silent fallback)" (addresses @coderabbitai on backendUrl.test.ts:106-126) - configPersistence.test.ts: use try/finally + mockRestore() for the localStorage.getItem spy so global state is always restored (addresses @coderabbitai on configPersistence.test.ts:257-264) - Welcome.test.tsx: update Test Connection success assertion to also verify clearBackendUrlCache() is called --- app/src/pages/Welcome.tsx | 1 + app/src/pages/__tests__/Welcome.test.tsx | 3 +- app/src/services/__tests__/backendUrl.test.ts | 9 ++-- .../services/__tests__/socketService.test.ts | 43 +++++++++++++------ app/src/services/backendUrl.ts | 20 +++++++-- .../utils/__tests__/configPersistence.test.ts | 12 +++--- app/src/utils/config.ts | 6 ++- 7 files changed, 64 insertions(+), 30 deletions(-) diff --git a/app/src/pages/Welcome.tsx b/app/src/pages/Welcome.tsx index 85ebb0c2e..31f6f3895 100644 --- a/app/src/pages/Welcome.tsx +++ b/app/src/pages/Welcome.tsx @@ -74,6 +74,7 @@ const Welcome = () => { setSaveSuccess(true); storeRpcUrl(normalized); clearCoreRpcUrlCache(); + clearBackendUrlCache(); } else { setRpcUrlError(`Connection failed: ${response.status} ${response.statusText}`); } diff --git a/app/src/pages/__tests__/Welcome.test.tsx b/app/src/pages/__tests__/Welcome.test.tsx index a4ffa9b35..4c02b4053 100644 --- a/app/src/pages/__tests__/Welcome.test.tsx +++ b/app/src/pages/__tests__/Welcome.test.tsx @@ -395,7 +395,7 @@ describe('Welcome — Test Connection button', () => { ); }); - it('successful probe stores the URL and clears the RPC URL cache', async () => { + it('successful probe stores the URL and clears both the RPC and backend URL caches', async () => { vi.mocked(testCoreRpcConnection).mockResolvedValueOnce({ ok: true, status: 200 } as Response); render(); @@ -405,6 +405,7 @@ describe('Welcome — Test Connection button', () => { await waitFor(() => { expect(storeRpcUrl).toHaveBeenCalledWith('http://127.0.0.1:7788/rpc'); expect(clearCoreRpcUrlCache).toHaveBeenCalledTimes(1); + expect(clearBackendUrlCache).toHaveBeenCalledTimes(1); }); }); }); diff --git a/app/src/services/__tests__/backendUrl.test.ts b/app/src/services/__tests__/backendUrl.test.ts index af282fe3c..a64bf6389 100644 --- a/app/src/services/__tests__/backendUrl.test.ts +++ b/app/src/services/__tests__/backendUrl.test.ts @@ -1,5 +1,7 @@ import { beforeEach, describe, expect, test, vi } from 'vitest'; +import { BACKEND_URL } from '../../utils/config'; + // Global test setup mocks `services/backendUrl` so consumers get a fixed URL // without RPC. To exercise the real implementation in this file, opt out. vi.unmock('../backendUrl'); @@ -111,12 +113,11 @@ describe('getBackendUrl', () => { // Should not have attempted an RPC call in non-Tauri mode expect(hoisted.callCoreRpcMock).not.toHaveBeenCalled(); - // Should return a non-empty string - expect(typeof url).toBe('string'); - expect(url.length).toBeGreaterThan(0); + // Should return the configured fallback constant + expect(url).toBe(BACKEND_URL); }); - test('graceful fallback to VITE_BACKEND_URL when core RPC throws', async () => { + test('propagates RPC errors in Tauri mode (no silent fallback)', async () => { hoisted.isTauriMock.mockReturnValue(true); hoisted.callCoreRpcMock.mockRejectedValue(new Error('RPC unavailable')); diff --git a/app/src/services/__tests__/socketService.test.ts b/app/src/services/__tests__/socketService.test.ts index ae51d2165..ad57dd559 100644 --- a/app/src/services/__tests__/socketService.test.ts +++ b/app/src/services/__tests__/socketService.test.ts @@ -42,8 +42,29 @@ vi.mock('../../lib/coreState/store', () => ({ getCoreStateSnapshot: vi.fn(() => ({ snapshot: { sessionToken: null } })), })); -// Mock MCP — must be a newable constructor -vi.mock('../../lib/mcp', () => ({ SocketIOMCPTransportImpl: vi.fn(() => ({})) })); +// Mock MCP as a class so `new SocketIOMCPTransportImpl(...)` works at runtime. +// Arrow functions cannot be used as constructors, so we wrap in a class here. +class MockMCPTransport {} +vi.mock('../../lib/mcp', () => ({ SocketIOMCPTransportImpl: MockMCPTransport })); + +/** + * Poll `check` up to `maxMs` ms (default 500) in 10 ms increments. + * Resolves when `check()` returns without throwing; rejects on timeout. + * Used instead of `setTimeout(0)` sleeps to deterministically wait for + * the observable side-effect of an async operation. + */ +async function pollUntil(check: () => void, maxMs = 500): Promise { + const deadline = Date.now() + maxMs; + while (true) { + try { + check(); + return; + } catch { + if (Date.now() >= deadline) throw new Error(`pollUntil timed out after ${maxMs}ms`); + await new Promise(r => setTimeout(r, 10)); + } + } +} // Hoist getCoreRpcUrl mock so it is available before the module is loaded const hoisted = vi.hoisted(() => ({ getCoreRpcUrlMock: vi.fn<() => Promise>() })); @@ -65,10 +86,8 @@ describe('socketService — resolveCoreSocketBaseUrl uses getCoreRpcUrl', () => const { socketService } = await import('../socketService'); socketService.connect('mock-jwt-token'); - // Give the async connectAsync a tick to run - await new Promise(resolve => setTimeout(resolve, 0)); - - expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled(); + // Wait until getCoreRpcUrl has actually been invoked (deterministic, no sleep) + await pollUntil(() => expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled()); }); it('strips /rpc suffix from the resolved RPC URL to derive the socket base', async () => { @@ -81,7 +100,7 @@ describe('socketService — resolveCoreSocketBaseUrl uses getCoreRpcUrl', () => const { socketService } = await import('../socketService'); socketService.connect('mock-jwt-token-2'); - await new Promise(resolve => setTimeout(resolve, 0)); + await pollUntil(() => expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled()); if (ioMock.mock.calls.length > 0) { const connectedUrl = ioMock.mock.calls[ioMock.mock.calls.length - 1][0]; @@ -105,10 +124,8 @@ describe('socketService — resolveCoreSocketBaseUrl uses getCoreRpcUrl', () => socketService.disconnect(); socketService.connect('mock-jwt-token-3'); - await new Promise(resolve => setTimeout(resolve, 0)); - - // getCoreRpcUrl must have been consulted - expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled(); + // getCoreRpcUrl must have been consulted (wait deterministically) + await pollUntil(() => expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled()); if (ioMock.mock.calls.length > 0) { const connectedUrl = ioMock.mock.calls[ioMock.mock.calls.length - 1][0]; @@ -128,9 +145,7 @@ describe('socketService — resolveCoreSocketBaseUrl uses getCoreRpcUrl', () => socketService.disconnect(); socketService.connect('mock-jwt-token-custom'); - await new Promise(resolve => setTimeout(resolve, 0)); - - expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled(); + await pollUntil(() => expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled()); if (ioMock.mock.calls.length > 0) { const connectedUrl = ioMock.mock.calls[ioMock.mock.calls.length - 1][0]; diff --git a/app/src/services/backendUrl.ts b/app/src/services/backendUrl.ts index 0e663c410..c17084aed 100644 --- a/app/src/services/backendUrl.ts +++ b/app/src/services/backendUrl.ts @@ -5,6 +5,13 @@ import { callCoreRpc } from './coreRpcClient'; let resolvedBackendUrl: string | null = null; let resolvingBackendUrl: Promise | null = null; +/** + * Monotonically-increasing generation counter. Incremented on every + * `clearBackendUrlCache()` call so that any in-flight `getBackendUrl()` + * resolution started before the clear does not repopulate the cache with a + * stale value after the user changes their RPC endpoint. + */ +let backendUrlGeneration = 0; /** * Invalidate the cached backend URL so the next call to getBackendUrl() @@ -13,6 +20,7 @@ let resolvingBackendUrl: Promise | null = null; * URL is recomputed from the updated core endpoint. */ export function clearBackendUrlCache(): void { + backendUrlGeneration += 1; resolvedBackendUrl = null; resolvingBackendUrl = null; } @@ -46,6 +54,7 @@ export async function getBackendUrl(): Promise { return resolvingBackendUrl; } + const generation = backendUrlGeneration; resolvingBackendUrl = (async () => { const response = await callCoreRpc<{ api_url?: string; apiUrl?: string }>({ method: 'openhuman.config_resolve_api_url', @@ -54,10 +63,15 @@ export async function getBackendUrl(): Promise { if (!resolved) { throw new Error('Core returned an empty backend URL'); } - resolvedBackendUrl = normalizeBaseUrl(resolved); - return resolvedBackendUrl; + const normalized = normalizeBaseUrl(resolved); + if (generation === backendUrlGeneration) { + resolvedBackendUrl = normalized; + } + return normalized; })().finally(() => { - resolvingBackendUrl = null; + if (generation === backendUrlGeneration) { + resolvingBackendUrl = null; + } }); return resolvingBackendUrl; diff --git a/app/src/utils/__tests__/configPersistence.test.ts b/app/src/utils/__tests__/configPersistence.test.ts index 07c7cc48e..875f62aae 100644 --- a/app/src/utils/__tests__/configPersistence.test.ts +++ b/app/src/utils/__tests__/configPersistence.test.ts @@ -254,14 +254,14 @@ describe('configPersistence', () => { describe('getStoredRpcUrl — localStorage unavailable', () => { it('returns the default URL when localStorage throws', () => { - const original = localStorage.getItem.bind(localStorage); - vi.spyOn(localStorage, 'getItem').mockImplementation(() => { + const getItemSpy = vi.spyOn(localStorage, 'getItem').mockImplementation(() => { throw new Error('Storage unavailable'); }); - - expect(getStoredRpcUrl()).toBe('http://127.0.0.1:7788/rpc'); - - vi.spyOn(localStorage, 'getItem').mockImplementation(original); + try { + expect(getStoredRpcUrl()).toBe('http://127.0.0.1:7788/rpc'); + } finally { + getItemSpy.mockRestore(); + } }); }); }); diff --git a/app/src/utils/config.ts b/app/src/utils/config.ts index 7f72bd252..e1c9df4b1 100644 --- a/app/src/utils/config.ts +++ b/app/src/utils/config.ts @@ -86,8 +86,10 @@ export const SENTRY_DSN = import.meta.env.VITE_SENTRY_DSN as string | undefined; * * **Not runtime-authoritative in Tauri.** In the desktop app, `getBackendUrl()` * (in `services/backendUrl.ts`) asks the core sidecar for the live API URL via - * `openhuman.config_resolve_api_url`; this constant is only consulted in web - * mode (no sidecar) or as an emergency fallback when the RPC call fails. + * `openhuman.config_resolve_api_url`. If that call fails or returns an empty + * URL, `getBackendUrl()` **throws** — it does not fall back to this constant. + * This constant is only used in web/non-Tauri mode (where the sidecar is not + * present). * * Override at build time via `VITE_BACKEND_URL`. */