feat(config): RPC bootstrap hardening, socket alignment, and exhaustive tests#1313
Merged
senamakel merged 4 commits intotinyhumansai:mainfrom May 6, 2026
Merged
Conversation
…ts (tinyhumansai#933) Resolves the remaining gaps from issue tinyhumansai#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 tinyhumansai#933
Contributor
📝 WalkthroughWalkthroughCentralizes runtime core RPC URL resolution with caching and invalidation, adds clearBackendUrlCache(), integrates cache clears into Welcome UI flows, refactors socketService to use getCoreRpcUrl(), removes build-time RPC endpoint helper, and adds extensive tests for URL handling and persistence. ChangesRuntime RPC URL resolution, cache invalidation, and UI integration
Sequence DiagramEstimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/pages/Welcome.tsx`:
- Around line 41-43: The success branch of handleTestConnection() currently
calls storeRpcUrl(normalized) and clearCoreRpcUrlCache() but misses clearing the
backend URL cache; update that success path to also call clearBackendUrlCache()
so the backend derived from the new RPC URL is invalidated immediately (mirror
the calls used in the Save/Reset flow: storeRpcUrl(normalized),
clearCoreRpcUrlCache(), clearBackendUrlCache()).
In `@app/src/services/__tests__/backendUrl.test.ts`:
- Around line 119-126: Rename the test whose description is currently "graceful
fallback to VITE_BACKEND_URL when core RPC throws" to reflect that it expects
rejection; update the test title string (in the test() call around the
getBackendUrl() assertion) to something like "propagates error when core RPC
throws" or "throws when core RPC throws" so the name matches the assertion that
getBackendUrl() rejects with 'RPC unavailable'.
- Around line 106-117: The test's non-Tauri branch should assert the exact
configured fallback value instead of just "non-empty": update the assertions in
the test that calls getBackendUrl (after loadFreshModule with
hoisted.isTauriMock.mockReturnValue(false)) to check that url equals the
module's configured fallback constant or env-derived value (e.g., the exported
fallback constant or process.env.BACKEND_URL used by the module) and still
assert hoisted.callCoreRpcMock was not called; locate
getBackendUrl/loadFreshModule in this test and replace the generic typeof/length
checks with a strict equality against that configured fallback.
In `@app/src/services/__tests__/socketService.test.ts`:
- Around line 68-69: Replace the brittle setTimeout(0) sleeps after
connectAsync() with assertion-based waits that poll for the observable
side-effect (e.g., waiting until getCoreRpcUrlMock has been called or ioMock
call count increments) instead of sleeping; in tests referencing connectAsync,
stop using "await new Promise(resolve => setTimeout(resolve, 0))" and instead
await a small helper or Jest/testing-library waitFor that resolves when
getCoreRpcUrlMock.mock.calls.length > 0 or ioMock.mock.calls.length increases
(apply same change to the other occurrences around lines 84-85, 108-109,
131-132), so the test deterministically waits for the actual effect rather than
relying on timing.
In `@app/src/services/backendUrl.ts`:
- Around line 15-17: clearBackendUrlCache currently nulled resolvingBackendUrl
but in-flight getBackendUrl() promises can still resolve and repopulate
resolvedBackendUrl; fix by introducing a generation/token (e.g.,
backendUrlVersion or resolveToken) that you increment or replace in
clearBackendUrlCache(), capture that token at the start of getBackendUrl(), and
only assign to resolvingBackendUrl and resolvedBackendUrl if the captured token
still matches the current global token—use the unique symbols
clearBackendUrlCache, getBackendUrl, resolvedBackendUrl, and resolvingBackendUrl
to locate where to add the token check and increment.
In `@app/src/utils/__tests__/configPersistence.test.ts`:
- Around line 257-264: The test temporarily replaces localStorage.getItem and
must restore it even if the test throws; change the inline spy usage to capture
the spy (or use vi.spyOn and assign to a variable) and call spy.mockRestore()
inside a finally block (or move restoration to an afterEach) so the global
localStorage.getItem is always restored after the test that calls
getStoredRpcUrl(); reference the mocked localStorage.getItem and the
getStoredRpcUrl test case to locate where to add the try/finally and mockRestore
call.
In `@app/src/utils/config.ts`:
- Around line 87-90: The JSDoc for BACKEND_URL incorrectly claims it's consulted
as a runtime fallback in Tauri; update either the doc or the runtime to match
behavior: either (A) tighten the JSDoc on BACKEND_URL in config.ts to state it
is only used in web mode or as an emergency fallback when the RPC is not
available (and explicitly note that getBackendUrl() in services/backendUrl.ts
will throw if openhuman.config_resolve_api_url fails in Tauri), or (B) implement
the fallback by modifying getBackendUrl() to return BACKEND_URL when
openhuman.config_resolve_api_url returns empty or errors (ensure you reference
the BACKEND_URL constant and handle errors from openhuman.config_resolve_api_url
consistently). Choose one approach and apply it so docs and runtime behavior
align.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bb2810b-77ac-4190-b6b1-47fb6f5821af
📒 Files selected for processing (10)
app/src/pages/Welcome.tsxapp/src/pages/__tests__/Welcome.test.tsxapp/src/services/__tests__/backendUrl.test.tsapp/src/services/__tests__/coreRpcClient.test.tsapp/src/services/__tests__/socketService.test.tsapp/src/services/backendUrl.tsapp/src/services/socketService.tsapp/src/utils/__tests__/configPersistence.test.tsapp/src/utils/config.tsapp/src/utils/configPersistence.ts
💤 Files with no reviewable changes (1)
- app/src/utils/configPersistence.ts
…i#1313 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
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/services/__tests__/socketService.test.ts (1)
105-112: 💤 Low valueConditional assertion may silently pass without verifying the expected behavior.
The
if (ioMock.mock.calls.length > 0)pattern means the test passes without verifying the socket URL if the 1420 guard prevents connection. Consider making this explicit with a comment or failing if neither branch provides meaningful verification.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 + // The 1420 guard prevented connection — at minimum verify getCoreRpcUrl was consulted. + // If this branch fires consistently, consider adjusting mocks to allow connection. expect(hoisted.getCoreRpcUrlMock).toHaveBeenCalled(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/services/__tests__/socketService.test.ts` around lines 105 - 112, The test uses a conditional that can let it pass without verifying behavior; update the assertion in socketService.test.ts so it explicitly fails if neither branch verifies the expected behavior: ensure that either ioMock was called with the expected URL ('http://127.0.0.1:7788') by asserting on ioMock.mock.calls (reference ioMock and its mock.calls) OR, if the 1420 guard prevents connection, explicitly assert that hoisted.getCoreRpcUrlMock was called (reference hoisted.getCoreRpcUrlMock); make the test fail when both checks are false so the behavior is always validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/services/__tests__/socketService.test.ts`:
- Around line 105-112: The test uses a conditional that can let it pass without
verifying behavior; update the assertion in socketService.test.ts so it
explicitly fails if neither branch verifies the expected behavior: ensure that
either ioMock was called with the expected URL ('http://127.0.0.1:7788') by
asserting on ioMock.mock.calls (reference ioMock and its mock.calls) OR, if the
1420 guard prevents connection, explicitly assert that hoisted.getCoreRpcUrlMock
was called (reference hoisted.getCoreRpcUrlMock); make the test fail when both
checks are false so the behavior is always validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 375722e5-e94e-415d-ad60-7e2ced8973f4
📒 Files selected for processing (7)
app/src/pages/Welcome.tsxapp/src/pages/__tests__/Welcome.test.tsxapp/src/services/__tests__/backendUrl.test.tsapp/src/services/__tests__/socketService.test.tsapp/src/services/backendUrl.tsapp/src/utils/__tests__/configPersistence.test.tsapp/src/utils/config.ts
This was referenced May 7, 2026
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
2 participants
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
socketServiceignored the user's stored RPC URL — Socket.IO was always connecting to the static default instead of the user-configured endpointclearBackendUrlCache()so changing the RPC URL also invalidates the derived backend URLWhat changed
Bug fix —
app/src/services/socketService.tsresolveCoreSocketBaseUrl()now callsgetCoreRpcUrl()(which respects the stored preference) instead of callinginvoke('core_rpc_url')directly. Previously, customising the RPC URL on the login screen had no effect on the socket connection.app/src/services/backendUrl.tsExported
clearBackendUrlCache()— parallel toclearCoreRpcUrlCache(). Changing the RPC URL now cascades to the backend URL cache.app/src/pages/Welcome.tsxCalls
clearBackendUrlCache()on save and reset alongside the existingclearCoreRpcUrlCache()call.app/src/utils/config.tsJSDoc added to
CORE_RPC_URLandBACKEND_URLexplaining they are build-time fallbacks, not runtime-authoritative values, with a pointer to the runtime resolution chain.app/src/utils/configPersistence.tsRemoved dead
buildRpcEndpoint()export (zero call sites).Tests (133 new, all passing)
Welcome.test.tsxcoreRpcClient.test.tsgetCoreRpcUrl()stored-URL preference, caching, post-clear refresh, Tauri invoke fallback, stored URL takes priority over invokebackendUrl.test.tsclearBackendUrlCache()forces re-derive, cache hit, RPC error fallback, non-Tauri pathsocketService.test.tsgetCoreRpcUrlcalled on connect,/rpcsuffix stripped, no-suffix handled, custom stored URL propagatedconfigPersistence.test.tsisValidRpcUrledge cases,normalizeRpcUrledge cases, round-trip storage,localStorage-unavailable fallbackTest plan
NODE_ENV=test pnpm --filter openhuman-app compilepassesNODE_ENV=test pnpm --filter openhuman-app format:checkpassesNODE_ENV=test pnpm --filter openhuman-app lintpasses (0 errors)pnpm test:coverage— 133 new tests, all passpnpm buildpassesbash scripts/install.sh --dry-run --verbosepassesCloses #933
Summary by CodeRabbit
Bug Fixes
Tests