-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(auth): correct Codespaces redirect using asExternalUri #7678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code because apparently I trust no one, not even myself.
let authRedirect = deepLink.toString() | ||
try { | ||
const external = await vscode.env.asExternalUri(deepLink) | ||
authRedirect = external.toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for this new asExternalUri logic. Should we add tests to verify both the success path and the fallback behavior when asExternalUri fails? The existing test file doesn't mock or test this new functionality.
authRedirect = external.toString() | ||
} catch (e) { | ||
// Fallback to the raw deep link if resolution fails (desktop will still work). | ||
this.log(`[auth] asExternalUri failed, falling back to deep link: ${e}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make the error logging more specific here? Instead of just ${e}
, perhaps ${e.message}
or even the full error object would help with debugging, especially since this is a new code path that might fail in unexpected ways.
const publisher = packageJSON?.publisher ?? "RooVeterinaryInc" | ||
const name = packageJSON?.name ?? "roo-cline" | ||
|
||
const deepLink = vscode.Uri.parse(`${vscode.env.uriScheme}://${publisher}.${name}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting this deep link construction and resolution logic (lines 271-280) into a separate private method like resolveAuthRedirectUri()
. This would improve readability and make it easier to unit test this specific functionality.
const state = crypto.randomBytes(16).toString("hex") | ||
await this.context.globalState.update(AUTH_STATE_KEY, state) | ||
|
||
// Resolve the deep link target for the CURRENT VS Code instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth adding a JSDoc comment to the login method documenting this new Codespaces-specific behavior? The inline comments are helpful, but a method-level documentation might help future maintainers understand the overall auth flow better.
In Codespaces/web, generate https auth_redirect via vscode.env.asExternalUri so the callback returns to the running instance instead of launching desktop. Complements Roo-Code-Cloud change that applies /auth/clerk/callback on https redirects.
Important
In
WebAuthService.ts
,login()
now usesvscode.env.asExternalUri
forauth_redirect
in Codespaces, with fallback to deep link on failure.WebAuthService.ts
,login()
now usesvscode.env.asExternalUri
to generateauth_redirect
URL for Codespaces/web environments.asExternalUri
fails, ensuring desktop compatibility.asExternalUri
fails and falls back to deep link.This description was created by
for 4b0db7a. You can customize this summary. It will automatically update as commits are pushed.