fix: handle Roo provider authentication errors gracefully #7298
Conversation
dosubot
bot
added
size:L
|
@roomote-agent use the switch mode tool to switch to pr-fixer mode and solve the conflicts on this PR |
|
Hi @daniel-lxs! I see your request to resolve the merge conflicts on this PR. I'll switch to pr-fixer mode now to handle the conflicts on the fix/roo-auth-error-handling branch. Working on it! 🔧 |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Review
Thank you for this comprehensive fix for the Roo provider authentication issue! The implementation effectively addresses the UI lockup problem with a well-structured approach.
✅ Strengths
- Well-structured error handling with custom
RooAuthenticationErrorclass - Comprehensive UI state management preventing lockups
- Complete internationalization across all 18 locales
- Smart pre-flight authentication checks before task creation
⚠️ Critical Issue
This PR has merge conflicts that must be resolved before merging. Please rebase or merge the latest main branch.
📊 Overall Assessment
Score: 8/10 - Solid implementation that effectively solves the reported issue. Once conflicts are resolved and minor improvements considered, this should be ready to merge.
src/api/index.ts
Outdated
There was a problem hiding this comment.
Suggestion: Consider using the i18n system for this fallback error message to maintain consistency with other user-facing messages:
const errorMessage = error instanceof Error ? error.message : t('common:errors.roo.authenticationRequired')
src/api/index.ts
Outdated
There was a problem hiding this comment.
Minor: Since you're throwing the error on line 161, the console.error might be redundant. Consider either:
- Removing the console.error, or
- Using a more structured logging approach if debugging info is needed
src/api/index.ts
Outdated
There was a problem hiding this comment.
Enhancement: Consider adding JSDoc documentation for better code maintainability:
/**
* Custom error class for Roo provider authentication failures.
* Thrown when the Roo provider is selected but user is not authenticated.
*/
export class RooAuthenticationError extends Error {
constructor(message: string) {
super(message)
this.name = "RooAuthenticationError"
// Ensure proper prototype chain for instanceof checks
Object.setPrototypeOf(this, RooAuthenticationError.prototype)
}
}There was a problem hiding this comment.
Type Safety: Consider using instanceof for better type safety:
import { RooAuthenticationError } from "../../api"
// Then use:
if (error instanceof RooAuthenticationError) {
// Handle authentication error
}This provides better type checking and is more maintainable than string comparison.
src/core/webview/ClineProvider.ts
Outdated
There was a problem hiding this comment.
Potential Enhancement: Consider implementing a retry mechanism that could store the pending task and automatically retry after successful authentication:
if (selection === signInText) {
// Store pending task for retry
this.pendingTaskAfterAuth = { text, images, parentTask, options }
await this.postMessageToWebview({ type: "action", action: "accountButtonClicked" })
}This would improve UX by not requiring users to manually restart their task.
- Add RooAuthenticationError custom error class for specific error handling - Pre-check Roo authentication before task creation to prevent UI lock-up - Send taskCreationFailed message to unlock UI when auth fails - Add translated error messages and button labels for all 18 locales - Show user-friendly dialog with Sign In and Settings options - Prevent UI from getting stuck when Roo provider is selected without authentication This ensures users can switch providers or sign in when Roo authentication fails, preventing the stuck state that occurred when selecting Roo without being logged in.
roomote
bot
force-pushed
the
fix/roo-auth-error-handling
branch
from
829b61a to
64c79e6
Compare
|
✅ Merge conflicts resolved successfully! I have successfully resolved the merge conflicts in this PR. The conflicts were in the translation files where both the main branch and this PR added new content to the MDM section. Summary of changes:
The PR is ready for review and merge. |
daniel-lxs
moved this from PR [Changes Requested]
to PR [Needs Review]
in Roo Code Roadmap
github-project-automation
bot
moved this from PR [Needs Review]
to Done
in Roo Code Roadmap
Summary
This PR fixes the issue where the UI would get stuck when selecting the Roo provider without being authenticated. The provider selector would become grayed out and unresponsive, preventing users from switching to a different provider.
Problem
When users selected the "Roo" provider without being logged into Roo Code Cloud, the RooHandler constructor would throw an error. While this error was caught, the UI would still become disabled, leaving users unable to switch providers or recover from the error state.
Solution
1. Custom Error Handling
RooAuthenticationErrorclass for specific Roo authentication failures2. Pre-flight Authentication Check
ClineProvider.createTask3. UI State Management
taskCreationFailedmessage type to unlock UI on auth failurenewChat4. Internationalization
Changes
Testing
Result
Users can now:
Fixes the stuck UI state when selecting Roo provider without authentication.
Important
This PR enhances Roo provider authentication handling by introducing specific error handling, UI state management improvements, and internationalization for user-friendly error messages.
RooAuthenticationErrorinindex.tsto handle specific authentication failures for the Roo provider.ClineProvider.createTaskto prevent task creation if Roo authentication fails.taskCreationFailedmessage type inExtensionMessage.tsto handle UI unlocking on auth failure.ChatView.tsxto processtaskCreationFailedmessages, re-enabling UI controls.This description was created by
for 17cbc21. You can customize this summary. It will automatically update as commits are pushed.