fix(oauth): surface backend outage instead of opening browser to 504 (#1985)#2147
Conversation
…inyhumansai#1985) Click → GET /health pre-flight (4s budget) → block openUrl and show "OpenHuman cloud sign-in is temporarily unavailable" banner on Cloudflare 5xx, DNS, abort, or offline. After a healthy click, a background probe on focus/visibility/90s-timeout return still surfaces the banner if the backend went down mid-flow. Before this change, a 504 from Cloudflare during OAuth (tinyhumansai#1985) sent the user into the system browser to an error page and back to a silently re-enabling button. There was no way to tell "service down" from "user cancelled" — both paths just reset and waited. The new services/backendHealth service is independent of services/backendUrl and treats only 5xx + network/abort failures as unhealthy; a 404 still proves the edge is reachable. Tests cover happy / 4xx / 5xx / AbortError / TypeError / resolve-failure plus the OAuthProviderButton wiring for both pre-flight and post-return probe paths.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a backend /health probe service and integrates pre-flight and return-path health checks into the OAuth provider button; tests and OAuth test harnesses are updated to mock and validate probe outcomes. ChangesOAuth Backend Health Pre-Flight and Return-Path Probing
Sequence DiagramsequenceDiagram
participant User
participant OAuthProviderButton
participant checkBackendHealthy
participant getBackendUrl
participant FetchAPI
participant SystemBrowser
User->>OAuthProviderButton: click provider button
OAuthProviderButton->>checkBackendHealthy: call()
checkBackendHealthy->>getBackendUrl: resolve backend URL
alt resolved
checkBackendHealthy->>FetchAPI: GET {backendUrl}/health (AbortController)
FetchAPI->>checkBackendHealthy: response (status / error)
alt healthy
checkBackendHealthy-->>OAuthProviderButton: healthy
OAuthProviderButton->>SystemBrowser: open browser (set browserOpenedRef)
SystemBrowser-->>OAuthProviderButton: user returns (focus/visibility)
OAuthProviderButton->>checkBackendHealthy: probe on return
else unhealthy
checkBackendHealthy-->>OAuthProviderButton: unhealthy -> set BACKEND_UNAVAILABLE_MESSAGE
end
else resolve-failure
getBackendUrl-->>checkBackendHealthy: throw -> checkBackendHealthy returns resolve-failure
checkBackendHealthy-->>OAuthProviderButton: unhealthy -> set BACKEND_UNAVAILABLE_MESSAGE
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/src/components/oauth/OAuthProviderButton.tsx (1)
84-101: ⚡ Quick winSwitch new
console.*diagnostics to the project’s namespaced debug logger.The newly added OAuth probe/preflight logs should align with the repo’s frontend logging convention.
As per coding guidelines: “Follow existing patterns for debug logging (e.g. the
debugnpm package with a namespace per area) plus dev-only detail where useful.”Also applies to: 173-177
🤖 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/components/oauth/OAuthProviderButton.tsx` around lines 84 - 101, Replace the ad-hoc console.* calls in the OAuthProviderButton probe flow with the project's namespaced debug logger: import and instantiate the module-scoped debug logger (using the repo convention and a namespace like 'oauth:oauth-button' or the existing namespace used elsewhere) and use it for the three probe branches (unhealthy log, healthy log, and thrown error) inside the OAuthProviderButton component (the code that references provider.id, label, setStartupError, BACKEND_UNAVAILABLE_MESSAGE and the checkBackendHealthy promise). Ensure the unhealthy log includes the result.reason, latencyMs, and status (same fields as before) via the debug logger, the healthy branch logs status and latencyMs, and the catch branch logs the error via debug; also update the other similar console.* occurrences around the second probe block (lines 173-177) to use the same namespaced debug logger so all frontend OAuth probe diagnostics follow the project convention.app/src/services/backendHealth.ts (1)
35-36: ⚡ Quick winUse namespaced
debuglogging instead ofconsole.*in frontend TS code.New logging here should follow the repo’s namespaced debug pattern with dev-only detail gating.
As per coding guidelines: “Follow existing patterns for debug logging (e.g. the
debugnpm package with a namespace per area) plus dev-only detail where useful.”Also applies to: 52-64
🤖 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/backendHealth.ts` around lines 35 - 36, Replace console.* calls with the repo’s namespaced debug logger: import the debug factory (e.g. from 'debug') and create a namespace for this module like const debug = createDebug('app:backend-health') (or use the existing debug helper if present), then replace console.debug and other console.* usages in backendHealth.ts (the branch around the console.debug('[backend-health] could not resolve backend URL', err) and the similar blocks in lines 52-64) with debug(...) calls. Gate verbose/error detail behind a dev-only check (e.g. if (process.env.NODE_ENV === 'development') debug('... %O', err) for stack/obj details) while keeping concise messages in non-dev; ensure the new debug import and namespace are used consistently across all replaced console statements in this file.app/src/services/__tests__/backendHealth.test.ts (1)
6-6: ⚡ Quick winMake
getBackendUrlmocking explicit in this test file.
vi.mocked(getBackendUrl)assumes the module is already mocked elsewhere. Add a localvi.mock('../backendUrl', ...)to avoid hidden setup coupling and keep this suite deterministic.As per coding guidelines: “Keep tests deterministic: avoid real network calls, time-sensitive flakes, or hidden global state.”Proposed patch
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { BACKEND_HEALTH_TIMEOUT_MS, checkBackendHealthy } from '../backendHealth'; import { getBackendUrl } from '../backendUrl'; + +vi.mock('../backendUrl', () => ({ getBackendUrl: vi.fn() }));Also applies to: 16-18
🤖 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__/backendHealth.test.ts` at line 6, Add an explicit vi.mock for the module that exports getBackendUrl at the top of this test file and replace the implicit vi.mocked(getBackendUrl) usage with a mocked function from that explicit mock; specifically, call vi.mock(...) to return an object with getBackendUrl: vi.fn(), then obtain mockedGetBackendUrl = vi.mocked(getBackendUrl) and set its mockImplementation/mockResolvedValue as needed (also update the other uses of mockedGetBackendUrl around the existing assertions where it’s referenced). Ensure the mock is local to this suite so tests remain deterministic.
🤖 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/components/oauth/OAuthProviderButton.tsx`:
- Around line 171-181: The preflight call to checkBackendHealthy can throw and
leave the component in a loading state; wrap the await checkBackendHealthy({
timeoutMs: OAUTH_PREFLIGHT_TIMEOUT_MS }) in a try/catch, log the error
(including provider.id), call setStartupError(BACKEND_UNAVAILABLE_MESSAGE),
ensure setIsLoading(false) is called in the catch, and return early so the
button doesn't stay disabled; keep the existing healthy-branch logic intact.
In `@app/src/services/backendHealth.ts`:
- Around line 43-48: Replace the raw fetchImpl call used to probe
`${backendUrl}/health` with an in-process core relay invocation using
invoke('core_rpc_relay', ...) so the health check goes through the local core
socket (not CORS/network). Locate the fetchImpl usage in this file (the call
that passes method: 'GET', cache: 'no-store', credentials: 'omit', signal:
controller.signal) and change it to call the core relay with the equivalent HTTP
method and path ('/health'), forward the abort signal/timeout behavior from
controller.signal, and handle the response/parsing the same way; remove reliance
on backendUrl and fetchImpl for this status probe so it always uses the
in-process core socket. Ensure error handling and timeouts remain consistent
with the previous logic in this module (use the same controller and response
handling code paths).
---
Nitpick comments:
In `@app/src/components/oauth/OAuthProviderButton.tsx`:
- Around line 84-101: Replace the ad-hoc console.* calls in the
OAuthProviderButton probe flow with the project's namespaced debug logger:
import and instantiate the module-scoped debug logger (using the repo convention
and a namespace like 'oauth:oauth-button' or the existing namespace used
elsewhere) and use it for the three probe branches (unhealthy log, healthy log,
and thrown error) inside the OAuthProviderButton component (the code that
references provider.id, label, setStartupError, BACKEND_UNAVAILABLE_MESSAGE and
the checkBackendHealthy promise). Ensure the unhealthy log includes the
result.reason, latencyMs, and status (same fields as before) via the debug
logger, the healthy branch logs status and latencyMs, and the catch branch logs
the error via debug; also update the other similar console.* occurrences around
the second probe block (lines 173-177) to use the same namespaced debug logger
so all frontend OAuth probe diagnostics follow the project convention.
In `@app/src/services/__tests__/backendHealth.test.ts`:
- Line 6: Add an explicit vi.mock for the module that exports getBackendUrl at
the top of this test file and replace the implicit vi.mocked(getBackendUrl)
usage with a mocked function from that explicit mock; specifically, call
vi.mock(...) to return an object with getBackendUrl: vi.fn(), then obtain
mockedGetBackendUrl = vi.mocked(getBackendUrl) and set its
mockImplementation/mockResolvedValue as needed (also update the other uses of
mockedGetBackendUrl around the existing assertions where it’s referenced).
Ensure the mock is local to this suite so tests remain deterministic.
In `@app/src/services/backendHealth.ts`:
- Around line 35-36: Replace console.* calls with the repo’s namespaced debug
logger: import the debug factory (e.g. from 'debug') and create a namespace for
this module like const debug = createDebug('app:backend-health') (or use the
existing debug helper if present), then replace console.debug and other
console.* usages in backendHealth.ts (the branch around the
console.debug('[backend-health] could not resolve backend URL', err) and the
similar blocks in lines 52-64) with debug(...) calls. Gate verbose/error detail
behind a dev-only check (e.g. if (process.env.NODE_ENV === 'development')
debug('... %O', err) for stack/obj details) while keeping concise messages in
non-dev; ensure the new debug import and namespace are used consistently across
all replaced console statements in this file.
🪄 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: 1c011ad3-9c60-4186-84b2-fb9b67af47ed
📒 Files selected for processing (8)
app/src/components/oauth/OAuthProviderButton.tsxapp/src/components/oauth/__tests__/OAuthProviderButton.test.tsxapp/src/services/__tests__/backendHealth.test.tsapp/src/services/backendHealth.tsapp/test/OAuthDiscord.test.tsxapp/test/OAuthGitHub.test.tsxapp/test/OAuthLoginSection.test.tsxapp/test/OAuthTwitter.test.tsx
| const preflight = await checkBackendHealthy({ timeoutMs: OAUTH_PREFLIGHT_TIMEOUT_MS }); | ||
| if (!preflight.healthy) { | ||
| console.warn(`[oauth-button][${provider.id}] preflight → backend unhealthy`, { | ||
| reason: preflight.reason, | ||
| latencyMs: preflight.latencyMs, | ||
| status: 'status' in preflight ? preflight.status : undefined, | ||
| }); | ||
| setStartupError(BACKEND_UNAVAILABLE_MESSAGE); | ||
| setIsLoading(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Guard pre-flight probe with try/catch to avoid a stuck loading state on unexpected throws.
If checkBackendHealthy() throws unexpectedly, isLoading is never reset and the button can remain disabled.
Proposed patch
- const preflight = await checkBackendHealthy({ timeoutMs: OAUTH_PREFLIGHT_TIMEOUT_MS });
- if (!preflight.healthy) {
- console.warn(`[oauth-button][${provider.id}] preflight → backend unhealthy`, {
- reason: preflight.reason,
- latencyMs: preflight.latencyMs,
- status: 'status' in preflight ? preflight.status : undefined,
- });
- setStartupError(BACKEND_UNAVAILABLE_MESSAGE);
- setIsLoading(false);
- return;
- }
+ let preflight;
+ try {
+ preflight = await checkBackendHealthy({ timeoutMs: OAUTH_PREFLIGHT_TIMEOUT_MS });
+ } catch (error) {
+ setStartupError(BACKEND_UNAVAILABLE_MESSAGE);
+ setIsLoading(false);
+ return;
+ }
+ if (!preflight.healthy) {
+ console.warn(`[oauth-button][${provider.id}] preflight → backend unhealthy`, {
+ reason: preflight.reason,
+ latencyMs: preflight.latencyMs,
+ status: 'status' in preflight ? preflight.status : undefined,
+ });
+ setStartupError(BACKEND_UNAVAILABLE_MESSAGE);
+ setIsLoading(false);
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const preflight = await checkBackendHealthy({ timeoutMs: OAUTH_PREFLIGHT_TIMEOUT_MS }); | |
| if (!preflight.healthy) { | |
| console.warn(`[oauth-button][${provider.id}] preflight → backend unhealthy`, { | |
| reason: preflight.reason, | |
| latencyMs: preflight.latencyMs, | |
| status: 'status' in preflight ? preflight.status : undefined, | |
| }); | |
| setStartupError(BACKEND_UNAVAILABLE_MESSAGE); | |
| setIsLoading(false); | |
| return; | |
| } | |
| let preflight; | |
| try { | |
| preflight = await checkBackendHealthy({ timeoutMs: OAUTH_PREFLIGHT_TIMEOUT_MS }); | |
| } catch (error) { | |
| setStartupError(BACKEND_UNAVAILABLE_MESSAGE); | |
| setIsLoading(false); | |
| return; | |
| } | |
| if (!preflight.healthy) { | |
| console.warn(`[oauth-button][${provider.id}] preflight → backend unhealthy`, { | |
| reason: preflight.reason, | |
| latencyMs: preflight.latencyMs, | |
| status: 'status' in preflight ? preflight.status : undefined, | |
| }); | |
| setStartupError(BACKEND_UNAVAILABLE_MESSAGE); | |
| setIsLoading(false); | |
| return; | |
| } |
🤖 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/components/oauth/OAuthProviderButton.tsx` around lines 171 - 181, The
preflight call to checkBackendHealthy can throw and leave the component in a
loading state; wrap the await checkBackendHealthy({ timeoutMs:
OAUTH_PREFLIGHT_TIMEOUT_MS }) in a try/catch, log the error (including
provider.id), call setStartupError(BACKEND_UNAVAILABLE_MESSAGE), ensure
setIsLoading(false) is called in the catch, and return early so the button
doesn't stay disabled; keep the existing healthy-branch logic intact.
There was a problem hiding this comment.
Skipping — checkBackendHealthy() is contract-bound to never throw. Both code paths that could reject are caught internally:
getBackendUrl()rejection → caught atbackendHealth.ts:34→ returns{healthy: false, reason: 'resolve-failure'}fetch()rejection (AbortError / TypeError / etc.) → caught atbackendHealth.ts:63→ returns{healthy: false, reason: 'timeout' | 'network'}
Adding an outer try/catch in the click handler would be defensive code for an unreachable state. The resolve-failure reason case is also covered by a unit test (backendHealth.test.ts) so the contract is regression-protected.
There was a problem hiding this comment.
@CodeGhost21 Fair point — the AI summary of backendHealth.ts confirms both rejection paths are converted to result objects internally, so the "never throws" behaviour holds for the current implementation.
One lightweight safeguard worth considering: a JSDoc annotation on checkBackendHealthy would make the contract explicit for future maintainers (and TypeScript-aware editors) so a refactor inside the service doesn't silently invalidate the no-try/catch assumption at call sites:
/**
* Probes the backend `/health` endpoint.
*
* **Never throws.** All network, timeout, and URL-resolution failures are
* caught internally and surfaced as `{ healthy: false, reason: … }` results.
*/
export async function checkBackendHealthy(options: CheckOptions = {}): Promise<BackendHealthResult> {That way the contract is documented at the source of truth rather than implicitly relied upon by callers. Up to you whether it's worth adding.
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧
🧠 Learnings used
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 167
File: app/src/components/OnboardingOverlay.tsx:30-39
Timestamp: 2026-04-01T13:52:17.936Z
Learning: In React components, do not perform synchronous `setState` (or other state-updating calls) directly inside `useEffect` bodies. The lint rule `react-hooks/set-state-in-effect` (enforced in this codebase) disallows patterns such as resetting state flags at the top of an effect before returning. Refactor to avoid calling state setters in the effect; prefer deriving state from props/render, using refs, or restructuring logic so state updates occur outside `useEffect`.
| const response = await fetchImpl(`${backendUrl}/health`, { | ||
| method: 'GET', | ||
| cache: 'no-store', | ||
| credentials: 'omit', | ||
| signal: controller.signal, | ||
| }); |
There was a problem hiding this comment.
Replace raw fetch() with in-process core relay for backend health probing.
This fetch call violates the frontend service-call policy and reintroduces CORS/network-path variability for a core status check.
Suggested direction
- const response = await fetchImpl(`${backendUrl}/health`, {
- method: 'GET',
- cache: 'no-store',
- credentials: 'omit',
- signal: controller.signal,
- });
+ // Route through invoke('core_rpc_relay', ...) via the existing core client path
+ // used for service-status style checks, then map response status into
+ // BackendHealthResult the same way.As per coding guidelines: “Always call the in-process core via invoke('core_rpc_relay', ...) — never use raw fetch() (CORS preflight) or callCoreRpc() for service-status calls when the socket may not be connected yet.”
🤖 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/backendHealth.ts` around lines 43 - 48, Replace the raw
fetchImpl call used to probe `${backendUrl}/health` with an in-process core
relay invocation using invoke('core_rpc_relay', ...) so the health check goes
through the local core socket (not CORS/network). Locate the fetchImpl usage in
this file (the call that passes method: 'GET', cache: 'no-store', credentials:
'omit', signal: controller.signal) and change it to call the core relay with the
equivalent HTTP method and path ('/health'), forward the abort signal/timeout
behavior from controller.signal, and handle the response/parsing the same way;
remove reliance on backendUrl and fetchImpl for this status probe so it always
uses the in-process core socket. Ensure error handling and timeouts remain
consistent with the previous logic in this module (use the same controller and
response handling code paths).
There was a problem hiding this comment.
Skipping — core_rpc_relay / callCoreRpc proxies to the local in-process core at http://127.0.0.1:<port>/rpc (see coreRpcClient.ts:335-378 — it builds a JSON-RPC payload, fetches getCoreRpcUrl(), and authenticates with the local-only OPENHUMAN_CORE_TOKEN bearer). It is not a generic HTTP egress proxy and it cannot reach api.tinyhumans.ai.
Issue #1985 is specifically about the external backend api.tinyhumans.ai returning Cloudflare 504. Probing the local core's /health would only confirm the local core is alive — which says nothing about whether the external host is reachable. The pre-flight has to hit the external host directly to detect the reported failure mode.
CORS — verified with curl -H "Origin: tauri://localhost" https://api.tinyhumans.ai/health -D -:
HTTP/2 200
access-control-allow-origin: *
access-control-allow-credentials: true
content-type: application/json; charset=utf-8
so the direct fetch from the WebView is fully permitted; no CORS workaround is needed.
If we want to abstract this through Rust later (e.g. for offline scenarios), the right primitive would be a new dedicated Tauri command (like the existing exchange_token / openhuman.config_resolve_api_url), not core_rpc_relay which is RPC-specific.
There was a problem hiding this comment.
@CodeGhost21, you're right — I was wrong on this one. core_rpc_relay / callCoreRpc is strictly a local JSON-RPC tunnel to 127.0.0.1:<port> and has no path to api.tinyhumans.ai, so routing the external health probe through it would completely defeat the purpose. The CORS evidence (access-control-allow-origin: *) also closes the loop on why a direct fetch() from the WebView is safe here. The note about a future dedicated Tauri command being the right abstraction layer (rather than core_rpc_relay) is a good one to keep in mind if this ever needs a Rust-side egress path.
I'll resolve my comment. Would you like me to go ahead and do that now?
✏️ Learnings added
Learnt from: CodeGhost21
Repo: tinyhumansai/openhuman PR: 2147
File: app/src/services/backendHealth.ts:43-48
Timestamp: 2026-05-18T20:17:22.924Z
Learning: In the OpenHuman desktop app (Tauri, TypeScript/React), `core_rpc_relay` / `callCoreRpc` (defined in `app/src/services/coreRpcClient.ts` around lines 335-378) is an in-process RPC tunnel that proxies JSON-RPC payloads to the local core at `http://127.0.0.1:<port>/rpc` using a local `OPENHUMAN_CORE_TOKEN` bearer. It is NOT a generic HTTP egress proxy and cannot reach external services such as `api.tinyhumans.ai`. Never suggest routing external health/status checks through `core_rpc_relay`.
Learnt from: CodeGhost21
Repo: tinyhumansai/openhuman PR: 2147
File: app/src/services/backendHealth.ts:43-48
Timestamp: 2026-05-18T20:17:22.924Z
Learning: If a Rust-side abstraction for external HTTP egress is ever needed in OpenHuman (e.g., for offline/proxy scenarios), the correct pattern is a new dedicated Tauri command (similar to existing commands `exchange_token` or `openhuman.config_resolve_api_url`), not `core_rpc_relay`, which is RPC-specific.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Per review on tinyhumansai#2147: replace the implicit reliance on the global services/backendUrl mock from app/src/test/setup.ts with an explicit local vi.mock so the suite controls getBackendUrl behavior end-to-end, including the rejection branch the 'resolve-failure' case asserts.
Re: review feedback — verification summaryVerified each suggestion against the codebase before acting. Applied (1 of 5):
Skipped, with reasoning (see inline replies for the two "Potential issue" threads):
Suite still green: |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Solid work — clean approach to a real UX pain point. Pre-flight + post-return probes are well-structured, discriminated result type keeps callers honest, and the test coverage is thorough (13 new test cases across service + component). Both #1985 and #1998 are addressed correctly. Two minor nits below, neither blocking.
| // (issue #1985) instead of sending the user into a system browser that | ||
| // lands on a gateway-error page with no path back into the app. | ||
| const preflight = await checkBackendHealthy({ timeoutMs: OAUTH_PREFLIGHT_TIMEOUT_MS }); | ||
| if (!preflight.healthy) { |
There was a problem hiding this comment.
[minor] getBackendUrl() is resolved twice on the happy path — once inside checkBackendHealthy() to build the /health URL, and again here to build the login URL. Not a correctness issue, but if getBackendUrl() does any async work (Tauri command invoke, etc.) it's a redundant round-trip on every click.
One option: have checkBackendHealthy accept an optional pre-resolved URL, or return the resolved URL in the healthy result so you can reuse it here.
| if (skipDuringDeepLink('focus')) return; | ||
| console.debug(`[oauth-button][${provider.id}] window focus → reset isLoading`); | ||
| reset(); | ||
| probeBackendOnReturn('focus'); |
There was a problem hiding this comment.
[minor] focus and visibilitychange can fire back-to-back in the same event-loop turn when the user switches back to the app (macOS does this). Both handlers call reset() then probeBackendOnReturn(), so you can get two concurrent probes. Harmless (second probe sets the same state), but a simple probed flag on the ref would deduplicate:
if (!browserOpenedRef.current) return;
browserOpenedRef.current = false; // consume the flagThat way the first handler to fire claims the probe and the second is a no-op.
Addresses two minor maintainer (@graycyrus) review comments and one CodeRabbit follow-up nit on PR tinyhumansai#2147: - focus + visibilitychange can fire back-to-back on macOS when the user returns from the system browser. probeBackendOnReturn now consumes browserOpenedRef on first invocation so the second event is a no-op instead of triggering a redundant concurrent probe. - The click handler used to call getBackendUrl() twice on the happy path (once inside checkBackendHealthy, once to build the loginUrl). The healthy result now carries the resolved backendUrl so the click handler can reuse it — saving a redundant Tauri IPC round-trip per click. - Documented the "never throws" contract on checkBackendHealthy via JSDoc so future maintainers don't accidentally invalidate the no-try/catch assumption at call sites. Tests updated to reflect that getBackendUrl is no longer called directly from the OAuth click handler (the mocked checkBackendHealthy owns it). The four OAuth integration test harnesses now mirror the real checkBackendHealthy contract — a mockGetBackendUrl rejection becomes a resolve-failure result instead of an unhandled throw.
…inyhumansai#1985) (tinyhumansai#2147) Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
…inyhumansai#1985) (tinyhumansai#2147) Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
…inyhumansai#1985) (tinyhumansai#2147) Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
3 participants
Summary
Fixes #1985 (also refs #1998) — when
api.tinyhumans.aireturns a Cloudflare 504 Gateway Time-out on OAuth login, the desktop app currently opens the system browser to a broken page and then silently re-enables the button after 90s with no user feedback. There is no way to tell service down from user cancelled. The same UX gap also makes #1998 (users in China hitting NXDOMAIN / 530 / 503 on the auth + API hosts) look like silent failure. This PR adds:/healthpre-flight on click that blocksopenUrl()and surfaces a clear "OpenHuman cloud sign-in is temporarily unavailable" banner when the backend is unreachable.What changed
New:
app/src/services/backendHealth.tscheckBackendHealthy({ timeoutMs?, fetchImpl? }) → Promise<BackendHealthResult>.AbortControllerwith a 6s default budget (4s for the inline click pre-flight) againstGET /health.reason ∈ 'timeout' | 'network' | 'http-5xx' | 'resolve-failure'./healththat happens to be 404 in some environment still proves the edge is reachable.fetch+AbortController.app/src/components/oauth/OAuthProviderButton.tsxopenUrl(loginUrl), runcheckBackendHealthy({ timeoutMs: 4_000 }). Unhealthy → show banner, skip the browser open, re-enable the button.checkBackendHealthy()to attribute the "didn't complete" signal. Healthy → silent reset (same as today, user cancelled). Unhealthy → same banner.browserOpenedRefso a pre-flight failure doesn't trigger a redundant probe via the focus listener that gets armed whileisLoading=true.Tests
app/src/services/__tests__/backendHealth.test.ts— 7 cases: 200 happy / 404 (still healthy) / 504 → reasonhttp-5xx/AbortError→ reasontimeout/TypeError→ reasonnetwork/getBackendUrl()rejection → reasonresolve-failure/ default-timeout regression guard.app/src/components/oauth/__tests__/OAuthProviderButton.test.tsx— 6 new cases: parametrized pre-flight failure (eachreason) blocksopenUrland shows banner; post-return probe shows banner when backend is unhealthy; stays silent when backend is healthy (user cancelled); 90s-timeout probe surfaces banner; no double-probe when pre-flight already blocked the browser open.app/test/OAuthDiscord.test.tsx,OAuthGitHub.test.tsx,OAuthLoginSection.test.tsx,OAuthTwitter.test.tsx— mockservices/backendHealthwith a default healthy result so their existing happy-path flows aren't blocked by the new pre-flight.Why this approach
reason: 'resolve-failure'/'network'/'http-5xx'all funnel into the same banner, so DNS NXDOMAIN onauth.tinyhumans.aiand 530/503 onapp.openhuman.ai/api.tinyhumans.ainow produce a clear user-visible signal instead of a hung sign-in button.Out of scope
auth.tinyhumans.aiNXDOMAIN + Cloudflare 530 in Unable to sign in from China - auth server unreachable (auth.tinyhumans.ai DNS NXDOMAIN, app.openhuman.ai returns 530) #1998), not in this repo.Test plan
pnpm debug unit— full suite green locally (268 files / 2622 tests passed, including the 4 OAuth files that previously failed before the mock update).pnpm lint— no new errors/warnings introduced by the new files.pnpm compile(tsc --noEmit) — clean.Manual smoke (recommended before merge)
These exercise the user-visible behavior and cannot be covered by unit tests alone:
api.tinyhumans.aireachable → click Google → system browser opens normally to OAuth (no perceptible added latency).VITE_BACKEND_URL=http://127.0.0.1:1) → click Google → red banner appears, browser does not open.Refs #1985, #1998.
Summary by CodeRabbit
New Features
Tests