-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix authentication in Firebase Studio IDE #6371
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
- Add Firebase Studio IDE environment detection in URI handler - Enhance WebAuthService with cloud IDE specific handling - Improve user agent detection to include environment info - Add comprehensive logging for authentication debugging - Provide better user feedback for cloud IDE environments Fixes #6369
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.
Code Review Summary
This PR addresses Firebase Studio IDE authentication issues by implementing enhanced environment detection and improved callback handling. While the core functionality looks solid, there are several areas that need attention for code quality and maintainability.
Critical Issues (Must Fix)
1. Code Duplication - DRY Violation
The isFirebaseStudioIDE() function is duplicated across 4 files with identical logic. This violates DRY principles and creates maintenance overhead. Consider extracting this to a shared utility module.
2. Missing Test Coverage
No tests exist for the new Firebase Studio IDE detection functionality, which is a core feature of this PR. This reduces confidence in the implementation and makes future changes risky.
3. Inconsistent Error Handling
Firebase Studio IDE gets special error handling in some places but not others. The error handling pattern should be consistent across all authentication flows.
Important Suggestions (Should Consider)
4. Environment Variable Security
The detection logic checks process.env.FIREBASE_PROJECT_ID and process.env.IDX_WORKSPACE_ID, but these could be set in any environment. Should this be more restrictive to avoid false positives?
5. User Experience
Multiple information messages are shown to users in Firebase Studio IDE environments. Could this become overwhelming? Consider consolidating notifications.
6. Logging Inconsistency
Some files use console.log while others use the injected logger. Should standardize logging approach across all files.
Minor Improvements (Nice to Have)
7. JSDoc Documentation
The isFirebaseStudioIDE() function lacks comprehensive JSDoc explaining the detection criteria and return conditions.
8. Type Safety
Consider creating a TypeScript enum or constant for the cloud IDE types instead of using string literals like "firebase_studio".
9. Performance
The environment detection runs on every authentication operation. Consider caching the result since environment does not change during runtime.
Overall Assessment
The PR successfully addresses the reported issue and the implementation approach is sound. However, the code quality issues, particularly the duplication and missing tests, should be addressed before merging to maintain codebase health.
| if (isCloudIDE) { | ||
| // Show additional guidance for Firebase Studio IDE users | ||
| vscode.window.showInformationMessage( | ||
| "Opening authentication in Firebase Studio IDE. After signing in, the callback should be automatically handled.", |
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.
User‐facing messages (e.g. the authentication guidance) are hardcoded. Use the translation (i18n) function instead to support localization.
| "Opening authentication in Firebase Studio IDE. After signing in, the callback should be automatically handled.", | |
| t("Opening authentication in Firebase Studio IDE. After signing in, the callback should be automatically handled."), |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
| await this.storeCredentials(credentials) | ||
|
|
||
| vscode.window.showInformationMessage("Successfully authenticated with Roo Code Cloud") | ||
| const successMessage = "Successfully authenticated with Roo Code Cloud" |
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.
For consistency with naming guidelines, consider using 'Roo Code' instead of 'Roo Code Cloud' when referring to the product in user-facing messages.
This comment was generated because it violated a code review rule: irule_VrRKWqywZ2YV2SOE.
|
Closing, the issue is not properly scoped yet |
This PR fixes authentication issues in Firebase Studio IDE (formerly IDX Google) by implementing enhanced environment detection and improved callback handling.
Changes Made
🔍 Enhanced Environment Detection
🔧 Improved Authentication Flow
WebAuthServicewith Firebase Studio IDE specific handling📊 Enhanced User Agent
🐛 Better Error Handling
Testing
Impact
This fix should resolve the issue where users cannot login to app.roocode.com via Firebase Studio IDE because "after confirmation the IDE not detected the confirmation."
Fixes #6369
Important
Enhances Firebase Studio IDE authentication by improving environment detection, callback handling, and logging in
WebAuthService,utils.ts,handleUri.ts, andextension.ts.isFirebaseStudioIDE()function to detect Firebase Studio IDE inWebAuthService,utils.ts,handleUri.ts, andextension.ts.WebAuthServicewith Firebase Studio IDE specific handling.WebAuthService.WebAuthServiceandhandleUri.ts.getUserAgent()inutils.tsto include environment information.WebAuthService,handleUri.ts, andextension.ts.This description was created by
for 782ec3e. You can customize this summary. It will automatically update as commits are pushed.