fix(oauth): stabilize first-launch OAuth CI readiness#2267
Conversation
…nsai#1689) First-launch Google/GitHub sign-in often failed because the auth deep link ran before BootCheckGate finished and the embedded core answered RPC. Gate OAuth on core mode + core.ping + bootstrap completion, start processing earlier from the Welcome buttons, and surface actionable errors instead of a generic failure message. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an OAuth readiness gate, a short desktop preflight, and deep-link lifecycle coordination: provider button starts and completes deep-link processing and awaits a quick readiness preflight on Tauri; the deep-link listener blocks when the OAuth gate reports not-ready; tests and re-exports updated. ChangesOAuth readiness gating and first-launch sign-in fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 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/oauthAuthReadiness.ts`:
- Line 32: Replace all direct console calls in oauthAuthReadiness.ts (e.g., the
console.debug(`${logPrefix} core.ping probe failed`, err) and the other
console.debug/console.warn/console.error occurrences noted around the referenced
ranges) with the project’s namespaced debug logger: import or obtain the
module-specific debug instance (e.g., const debug =
createDebug('oauth:authReadiness') or using the project helper) and call
debug(...) for normal messages and debug(... /* dev-only details */) or
debug.extend('warn')/debug.extend('error') (or the project’s recommended
pattern) for warnings/errors, passing the same message, logPrefix and err
objects; update every instance (lines referenced: 32, 46-49, 77, 88-92, 100-105,
135) to use that namespaced debug function so logs are consistent and grep-able.
- Line 5: Replace the local utils import of isTauri with the standardized export
from the webview account service: import isTauri from (or { isTauri } depending
on that module's export) app/src/services/webviewAccountService.ts and update
any existing calls to use that symbol; if the service exposes isTauri as a named
export use { isTauri }, if it is the default export import accordingly so all
runtime Tauri checks in oauthAuthReadiness.ts call the approved isTauri
implementation.
🪄 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: 803b757c-c487-44ef-a532-0421e513a429
📒 Files selected for processing (11)
app/src/components/oauth/OAuthProviderButton.tsxapp/src/components/oauth/__tests__/OAuthProviderButton.test.tsxapp/src/components/oauth/__tests__/oauthAuthReadiness.test.tsapp/src/components/oauth/oauthAuthReadiness.tsapp/src/utils/__tests__/desktopDeepLinkListener.test.tsapp/src/utils/desktopDeepLinkListener.tsapp/src/utils/oauthAppVersionGate.tsapp/test/OAuthDiscord.test.tsxapp/test/OAuthGitHub.test.tsxapp/test/OAuthLoginSection.test.tsxapp/test/OAuthTwitter.test.tsx
coderabbitai
Bot
added
the
feature
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils/__tests__/configPersistence.test.ts`:
- Around line 421-435: The test leaks module mocks if the import or assertion
throws; wrap the portion that imports '../configPersistence' and calls
getStoredCoreMode() in a try/finally so vi.doUnmock('../config') and
vi.resetModules() always run; specifically, around the block that imports
'../configPersistence' and calls mod.getStoredCoreMode(), ensure cleanup of the
mock and module registry in the finally, preserving removal of MODE_STORAGE_KEY
before import and using vi.doMock('../config') / vi.doUnmock('../config') and
vi.resetModules() in the finally to avoid mock leakage between tests.
🪄 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: fc35ed90-850d-4110-abb0-9e3c0570ff25
📒 Files selected for processing (2)
app/src/utils/__tests__/configPersistence.test.tsapp/src/utils/configPersistence.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/components/oauth/__tests__/OAuthProviderButton.test.tsx (1)
82-84: ⚡ Quick winCover the full deep-link lifecycle in this test.
These assertions prove the click path calls
beginDeepLinkAuthProcessing(), but the sharedgetDeepLinkAuthState()mock never transitions with it, so the suite still passes if the component forgets to clear the flag after launching the browser. Making the mock stateful—or asserting the matchingcompleteDeepLinkAuthProcessing()call—would turn this into a real regression guard.As per coding guidelines, "Prefer testing behavior over implementation details. ... Keep tests deterministic: avoid real network calls, time-sensitive flakes, or hidden global state."
🤖 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/__tests__/OAuthProviderButton.test.tsx` around lines 82 - 84, The test currently only asserts beginDeepLinkAuthProcessing/prepareOAuthLoginLaunch/checkBackendHealthy but never verifies the deep-link flag is cleared; update the test to cover the full lifecycle by either making the getDeepLinkAuthState() mock stateful (so it returns "processing" after beginDeepLinkAuthProcessing() and "not processing" after completeDeepLinkAuthProcessing()) or simply add an assertion that completeDeepLinkAuthProcessing() is called after the click; target the functions beginDeepLinkAuthProcessing, completeDeepLinkAuthProcessing, and getDeepLinkAuthState in your changes so the test fails if the component fails to clear the deep-link state.app/src/components/oauth/OAuthProviderButton.tsx (1)
89-109: ⚡ Quick winUse the frontend logger for these new probes.
These new
console.warn/console.debugcalls bypass the repo’s namespaced logging pattern, so they won’t get the usual filtering/correlation behavior duringpnpm dev.As per coding guidelines, "Use namespaced
debugfunction in TypeScript frontend with dev-only detail logging."Also applies to: 183-187
🤖 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 89 - 109, Replace the raw console calls in the checkBackendHealthy promise handler inside OAuthProviderButton (the block referencing checkBackendHealthy, provider.id, label, and setStartupError) with the repo's namespaced frontend logger: use the dev-only namespaced debug function for debug-level messages and the namespaced logger/warn equivalent for warnings so these probes participate in the app's logging/filtering; do the same for the other occurrences mentioned (the similar console.* calls around lines 183-187) and ensure you import/instantiate the same debug/logger symbol used elsewhere in the frontend.
🤖 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 194-196: The new preflight call to prepareOAuthLoginLaunch()
inside the OAuthProviderButton flow can throw a user-facing
readiness/app-version message that is currently swallowed by the generic catch;
update the catch in the OAuthProviderButton click/launch path to preserve and
surface the original error message from prepareOAuthLoginLaunch() (e.g., capture
err.message or the error object) instead of always replacing it with “Google
sign-in could not start.” Ensure the code paths that set the provider banner (or
call showProviderBanner) use the preserved error text when
prepareOAuthLoginLaunch() fails so the actionable guidance is visible to the
user.
- Around line 181-191: The deep-link lifecycle isn't cleared on the successful
handoff path, leaving the global isProcessing/skipDuringDeepLink state set if
the user cancels or never returns; after triggering the browser handoff in the
OAuth flow (the branches that call openUrl(...) or assign window.location.href),
call completeDeepLinkAuthProcessing() immediately (and ensure this is done in
both places identified around beginDeepLinkAuthProcessing()) so
skipDuringDeepLink() stops suppressing focus/visibility/timeout handling; apply
the same fix in the other similar block (lines around the second
openUrl/window.location.href usage referenced in the review).
---
Nitpick comments:
In `@app/src/components/oauth/__tests__/OAuthProviderButton.test.tsx`:
- Around line 82-84: The test currently only asserts
beginDeepLinkAuthProcessing/prepareOAuthLoginLaunch/checkBackendHealthy but
never verifies the deep-link flag is cleared; update the test to cover the full
lifecycle by either making the getDeepLinkAuthState() mock stateful (so it
returns "processing" after beginDeepLinkAuthProcessing() and "not processing"
after completeDeepLinkAuthProcessing()) or simply add an assertion that
completeDeepLinkAuthProcessing() is called after the click; target the functions
beginDeepLinkAuthProcessing, completeDeepLinkAuthProcessing, and
getDeepLinkAuthState in your changes so the test fails if the component fails to
clear the deep-link state.
In `@app/src/components/oauth/OAuthProviderButton.tsx`:
- Around line 89-109: Replace the raw console calls in the checkBackendHealthy
promise handler inside OAuthProviderButton (the block referencing
checkBackendHealthy, provider.id, label, and setStartupError) with the repo's
namespaced frontend logger: use the dev-only namespaced debug function for
debug-level messages and the namespaced logger/warn equivalent for warnings so
these probes participate in the app's logging/filtering; do the same for the
other occurrences mentioned (the similar console.* calls around lines 183-187)
and ensure you import/instantiate the same debug/logger symbol used elsewhere in
the frontend.
🪄 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: fa9b58ef-fd79-42ee-b14f-32d567b01d14
📒 Files selected for processing (8)
app/src/components/oauth/OAuthProviderButton.tsxapp/src/components/oauth/__tests__/OAuthProviderButton.test.tsxapp/src/utils/__tests__/configPersistence.test.tsapp/src/utils/configPersistence.tsapp/test/OAuthDiscord.test.tsxapp/test/OAuthGitHub.test.tsxapp/test/OAuthLoginSection.test.tsxapp/test/OAuthTwitter.test.tsx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@senamakel #2267 已更新到最新 main，冲突已解决，CI 全部通过（22 success / 2 expected skipped），CodeRabbit 最新 review 已 approved 且无未解决 thread。麻烦再看一下。 |
# Conflicts: # .github/tauri-cef-expected-sha
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@senamakel #2267 已再次同步最新 main，冲突已解决；当前 head |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Good work stabilizing the first-launch OAuth flow and CI. The deep-link readiness gate in desktopDeepLinkListener.ts is solid — it properly checks the result and blocks when not ready. Test coverage is thorough. However, the button-click path has a critical gap where the readiness check silently no-ops.
| Area | Files | Verdict |
|---|---|---|
| OAuth readiness | oauthAuthReadiness.ts, OAuthProviderButton.tsx |
1 critical |
| Deep-link listener | desktopDeepLinkListener.ts |
Clean |
| Config persistence | configPersistence.ts |
Clean |
| E2E helpers | deep-link-helpers.ts |
Clean |
| Legacy OAuth tests | OAuth*.test.tsx |
Clean (mock plumbing correct) |
| Tauri CEF SHA | .github/tauri-cef-expected-sha |
Trivial newline fix |
|
@graycyrus addressed both review threads in cdf0ed9 and replied inline with details. Ready for another pass when you have a chance. |
|
huge thanks @YOMXXX 🙌 nailing the first-launch oauth flow while keeping the readiness gate intact is super clean work. really appreciate you stabilizing those legacy provider tests too, this one's a solid unblock 🚀 |
4 participants
Summary
window.__simulateDeepLinkhelper fire-and-forget, matching productiononOpenUrlbehavior so WebDriver does not block on auth readiness.Problem
browser.execute(async () => await window.__simulateDeepLink(...))waits on the full auth callback path, including readiness waits.Solution
prepareOAuthLoginLaunch()in the legacy Google/GitHub/Discord/Twitter OAuth tests, preserving their existing URL/openUrl assertions while isolating the readiness preflight.__simulateDeepLinkto schedulehandleDeepLinkUrls()without awaiting it, which matches the real desktop deep-link listener's fire-and-forget handler.YOMXXXdespitemaintainerCanModify=true.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.## Related— N/A: no matrix feature IDs changed.docs/RELEASE-MANUAL-SMOKE.md) — N/A: no release smoke procedure changed.Closes #NNNin the## RelatedsectionImpact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/2247-oauth-ci-readiness3367058b145a37566dcd377198b2881a977ce3cdValidation Run
pnpm --filter openhuman-app format:check(via pre-push hook)pnpm typecheckpnpm debug unit test/OAuthTwitter.test.tsxpnpm debug unit src/utils/__tests__/desktopDeepLinkListener.test.tspnpm debug unit test/OAuthGitHub.test.tsx test/OAuthDiscord.test.tsx test/OAuthLoginSection.test.tsxpnpm debug unit src/components/oauth/__tests__/OAuthProviderButton.test.tsx src/components/oauth/__tests__/oauthAuthReadiness.test.tspnpm debug unit src/components/oauth/__tests__/oauthAuthReadiness.test.ts src/utils/__tests__/desktopDeepLinkListener.test.ts src/components/oauth/__tests__/OAuthProviderButton.test.tsx test/OAuthTwitter.test.tsxpnpm debug unit src/utils/__tests__/configPersistence.test.ts src/components/oauth/__tests__/oauthAuthReadiness.test.ts src/utils/__tests__/desktopDeepLinkListener.test.tspnpm debug unitpnpm test:coveragepnpm --dir app exec eslint src/utils/desktopDeepLinkListener.ts src/utils/__tests__/desktopDeepLinkListener.test.ts test/OAuthTwitter.test.tsx test/OAuthGitHub.test.tsx test/OAuthDiscord.test.tsx test/OAuthLoginSection.test.tsx --ext .ts,.tsx --max-warnings=0cargo fmt --manifest-path ../Cargo.toml --all --check,cargo fmt --manifest-path src-tauri/Cargo.toml --all --check, andGGML_NATIVE=OFF pnpm rust:checkvia pre-push hookcargo fmt --manifest-path app/src-tauri/Cargo.toml --all --checkandGGML_NATIVE=OFF pnpm rust:checkvia pre-push hookValidation Blocked
command:N/Aerror:N/Aimpact:N/A. Local macOS/Tahoerust:checkrequires the documentedGGML_NATIVE=OFFworkaround forwhisper-rs-sys/-mcpu=native; validation passed with that env var.Behavior Changes
Parity Contract
openUrl()URL construction remain unchanged.Duplicate / Superseded PR Handling
git push git@github.com:CodeGhost21/openhuman.git HEAD:cursor/a02-1689-signin-failed-first-timewas rejected withPermission denied to YOMXXX.Summary by CodeRabbit
New Features
Tests