fix(vscode): show actionable message when cloud sign-in has no linked…(#1299)#1464
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe VS Code extension now resolves environment-specific RocketRide Cloud URLs and distinguishes successful, waitlisted, and inactive-account outcomes during OAuth token exchange. Users without an active API key are prompted to open RocketRide Cloud, while waitlisted users receive an informational message. ChangesCloud sign-in result handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
apps/vscode/src/auth/CloudAuthProvider.tsOops! Something went wrong! :( ESLint: 9.39.4 TypeError: expand is not a function 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 |
kairavb
changed the title
|
Thanks for picking this up — the old "no token received" toast was genuinely useless, and the direction here (actionable message + button) is exactly right. Before we merge, though, I traced the server side of this flow and the premise behind the new message doesn't hold up. A resolved
In both cases the new message sends the user the wrong way. A waitlisted user who clicks "Create Account", signs up, and signs in again lands right back on the same error — a frustrating loop with no exit. A revoked-keys user is told to create an account they already have. Suggested change: branch on
A few smaller things while you're in there:
Happy to walk through the server-side trace if useful. The structure of the change is solid — it's just the interpretation of the empty-token branch that needs adjusting. |
a062260 to
9275910
Compare
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
|
Thanks for the detailed review and the server-side trace—it clarified where my assumption was wrong. I've updated the implementation to branch on result.waitlisted, show a pending-access message for waitlisted users, distinguish the inactive API key case, derive the Cloud URL from the deployment configuration instead of hardcoding it, and separate openExternal() failures from the sign-in flow. I'd appreciate another look when you have a chance. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/vscode/src/auth/CloudAuthProvider.ts`:
- Around line 43-56: Update resolveCloudAppUrl so its parse-failure path no
longer returns the hardcoded production URL; instead fail safely by returning
the configured cloudApiUrl unchanged or using the existing flow to suppress the
Cloud button. Preserve the current URL transformation for valid configured
deployments.
- Around line 214-225: Separate the authentication and cleanup flows in the
method containing the temporary RocketRideClient: keep connect() failures
handled by the existing sign-in error path, but run tempClient.disconnect() in
independent cleanup handling after a successful connection. Ensure disconnect
errors do not return early or prevent the subsequent storeToken and
storeUserName operations from processing the successful ConnectResult.
- Around line 137-174: Update handleGoogleOAuth so error and missing-token
responses notify the matching pending Google OAuth waiter before returning,
rather than deleting it without resolution. Add or reuse an explicit failure
callback/error response alongside the success callback established by
setPendingGoogleOAuth, while preserving nodeId matching and sole-waiter fallback
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 445719a7-6940-4c2c-a2c5-0ff25a9c6b43
📒 Files selected for processing (1)
apps/vscode/src/auth/CloudAuthProvider.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 `@apps/vscode/src/auth/CloudAuthProvider.ts`:
- Around line 43-56: Update resolveCloudAppUrl so its parse-failure path no
longer returns the hardcoded production URL; instead fail safely by returning
the configured cloudApiUrl unchanged or using the existing flow to suppress the
Cloud button. Preserve the current URL transformation for valid configured
deployments.
- Around line 214-225: Separate the authentication and cleanup flows in the
method containing the temporary RocketRideClient: keep connect() failures
handled by the existing sign-in error path, but run tempClient.disconnect() in
independent cleanup handling after a successful connection. Ensure disconnect
errors do not return early or prevent the subsequent storeToken and
storeUserName operations from processing the successful ConnectResult.
- Around line 137-174: Update handleGoogleOAuth so error and missing-token
responses notify the matching pending Google OAuth waiter before returning,
rather than deleting it without resolution. Add or reuse an explicit failure
callback/error response alongside the success callback established by
setPendingGoogleOAuth, while preserving nodeId matching and sole-waiter fallback
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 445719a7-6940-4c2c-a2c5-0ff25a9c6b43
📒 Files selected for processing (1)
apps/vscode/src/auth/CloudAuthProvider.ts
🛑 Comments failed to post (3)
apps/vscode/src/auth/CloudAuthProvider.ts (3)
43-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fallback still hardcodes production, contrary to the PR feedback.
resolveCloudAppUrlderives the URL from the configured deployment on the happy path, but on parse failure it falls back to a hardcoded'https://cloud.rocketride.ai/'. IfROCKETRIDE_URIis set but malformed on a staging/dev deployment, this still sends the user to production — the exact issue Rod-Christensen flagged. Prefer failing safe (e.g., return the rawcloudApiUrlunmodified, or skip the "Open RocketRide Cloud" button) rather than defaulting to production.🔧 Suggested fix
} catch { - return 'https://cloud.rocketride.ai/'; + // Fall back to the raw configured endpoint rather than assuming production. + return cloudApiUrl; }📝 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.function resolveCloudAppUrl(cloudApiUrl: string): string { try { const url = new URL(RocketRideClient.normalizeUri(cloudApiUrl)); if (url.hostname.startsWith('api.')) { url.hostname = `cloud.${url.hostname.slice('api.'.length)}`; } url.pathname = '/'; url.search = ''; url.hash = ''; return url.toString(); } catch { // Fall back to the raw configured endpoint rather than assuming production. return cloudApiUrl; } }🤖 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 `@apps/vscode/src/auth/CloudAuthProvider.ts` around lines 43 - 56, Update resolveCloudAppUrl so its parse-failure path no longer returns the hardcoded production URL; instead fail safely by returning the configured cloudApiUrl unchanged or using the existing flow to suppress the Cloud button. Preserve the current URL transformation for valid configured deployments.
137-174: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== CloudAuthProvider outline ==\n' ast-grep outline apps/vscode/src/auth/CloudAuthProvider.ts --view expanded || true printf '\n== Relevant usages of setPendingGoogleOAuth / pendingGoogleOAuth ==\n' rg -n "setPendingGoogleOAuth|pendingGoogleOAuth|handleGoogleOAuth|GoogleOAuth" apps/vscode/src -A3 -B3 || true printf '\n== File size ==\n' wc -l apps/vscode/src/auth/CloudAuthProvider.tsRepository: rocketride-org/rocketride-server
Length of output: 6076
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,260p' apps/vscode/src/auth/CloudAuthProvider.ts | cat -nRepository: rocketride-org/rocketride-server
Length of output: 11652
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,260p' apps/vscode/src/auth/CloudAuthProvider.ts | cat -nRepository: rocketride-org/rocketride-server
Length of output: 11652
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '470,560p' apps/vscode/src/providers/ProjectProvider.ts | cat -nRepository: rocketride-org/rocketride-server
Length of output: 4470
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '470,560p' apps/vscode/src/providers/ProjectProvider.ts | cat -nRepository: rocketride-org/rocketride-server
Length of output: 4470
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== project:oauthTokens handling ==\n' rg -n "project:oauthTokens|oauthTokens|Google sign-in|waiting for sign-in|sign-in" apps/vscode/src/providers/ProjectProvider.ts apps/vscode/src -A4 -B4 || true printf '\n== nearby webview messaging flow ==\n' sed -n '560,760p' apps/vscode/src/providers/ProjectProvider.ts | cat -nRepository: rocketride-org/rocketride-server
Length of output: 33660
Handle Google sign-in failures before clearing the waiter.
setPendingGoogleOAuth()only feedsproject:oauthTokens, so theerror/!tokensreturns here drop the one-shot callback and leave the panel waiting forever. Send a failure response before returning, or add an explicit error callback.🤖 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 `@apps/vscode/src/auth/CloudAuthProvider.ts` around lines 137 - 174, Update handleGoogleOAuth so error and missing-token responses notify the matching pending Google OAuth waiter before returning, rather than deleting it without resolution. Add or reuse an explicit failure callback/error response alongside the success callback established by setPendingGoogleOAuth, while preserving nodeId matching and sole-waiter fallback behavior.
214-225: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cleanup (
disconnect) failure is conflated with auth failure, masking a successful sign-in.
tempClient.disconnect()shares the sametryasconnect(). Ifconnect()succeeds butdisconnect()throws (e.g. transport already closed), execution jumps to thecatch, shows a misleading "RocketRide Cloud sign-in failed" message, and returns beforestoreToken/storeUserNamerun — even though the user did authenticate successfully. Cleanup errors shouldn't abort a successful auth result.🔧 Suggested fix
let result: ConnectResult; + const tempClient = new RocketRideClient({ persist: false }); try { - const tempClient = new RocketRideClient({ persist: false }); result = await tempClient.connect({ code, verifier, redirectUri: REDIRECT_URI }, { uri: cloudUrl }); - - // Disconnect immediately — we only needed the auth result - await tempClient.disconnect(); } catch (error) { const msg = error instanceof Error ? error.message : String(error); vscode.window.showErrorMessage(`RocketRide Cloud sign-in failed: ${msg}`); return; + } finally { + // Disconnect immediately — we only needed the auth result. Best-effort; + // don't let cleanup errors mask a successful auth result. + try { + await tempClient.disconnect(); + } catch { + /* ignore cleanup errors */ + } }📝 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.let result: ConnectResult; const tempClient = new RocketRideClient({ persist: false }); try { result = await tempClient.connect({ code, verifier, redirectUri: REDIRECT_URI }, { uri: cloudUrl }); } catch (error) { const msg = error instanceof Error ? error.message : String(error); vscode.window.showErrorMessage(`RocketRide Cloud sign-in failed: ${msg}`); return; } finally { // Disconnect immediately — we only needed the auth result. Best-effort; // don't let cleanup errors mask a successful auth result. try { await tempClient.disconnect(); } catch { /* ignore cleanup errors */ } }🤖 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 `@apps/vscode/src/auth/CloudAuthProvider.ts` around lines 214 - 225, Separate the authentication and cleanup flows in the method containing the temporary RocketRideClient: keep connect() failures handled by the existing sign-in error path, but run tempClient.disconnect() in independent cleanup handling after a successful connection. Ensure disconnect errors do not return early or prevent the subsequent storeToken and storeUserName operations from processing the successful ConnectResult.
2 participants
… account
Summary
catchblock.Type
Testing
./builder testpassesChecklist
Linked Issue
Fixes #1299
Summary by CodeRabbit