-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: separate Task Sync and Roomote Control settings #7634
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -607,7 +607,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| this.emit(RooCodeEventName.Message, { action: "created", message }) | ||
| await this.saveClineMessages() | ||
|
|
||
| const shouldCaptureMessage = message.partial !== true && CloudService.isEnabled() | ||
| // Check if we should capture the message | ||
| // Only capture if: not partial, cloud is enabled, and taskSyncEnabled is true | ||
| const state = await this.providerRef.deref()?.getState() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this intentional? We're fetching the provider state twice in quick succession (lines 612 and 649). Could we cache the state at the beginning of the method to avoid potential race conditions where |
||
| const taskSyncEnabled = state?.taskSyncEnabled ?? true // Default to true for backward compatibility | ||
| const shouldCaptureMessage = message.partial !== true && CloudService.isEnabled() && taskSyncEnabled | ||
|
|
||
| if (shouldCaptureMessage) { | ||
| CloudService.instance.captureEvent({ | ||
|
|
@@ -640,7 +644,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| await provider?.postMessageToWebview({ type: "messageUpdated", clineMessage: message }) | ||
| this.emit(RooCodeEventName.Message, { action: "updated", message }) | ||
|
|
||
| const shouldCaptureMessage = message.partial !== true && CloudService.isEnabled() | ||
| // Check if we should capture the message | ||
| // Only capture if: not partial, cloud is enabled, and taskSyncEnabled is true | ||
| const state = await this.providerRef.deref()?.getState() | ||
| const taskSyncEnabled = state?.taskSyncEnabled ?? true // Default to true for backward compatibility | ||
| const shouldCaptureMessage = message.partial !== true && CloudService.isEnabled() && taskSyncEnabled | ||
|
|
||
| if (shouldCaptureMessage) { | ||
| CloudService.instance.captureEvent({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -965,6 +965,17 @@ export const webviewMessageHandler = async ( | |
| await provider.remoteControlEnabled(message.bool ?? false) | ||
| await provider.postStateToWebview() | ||
| break | ||
| case "taskSyncEnabled": | ||
| try { | ||
| await CloudService.instance.updateUserSettings({ | ||
| taskSyncEnabled: message.bool ?? true, | ||
| }) | ||
| } catch (error) { | ||
| provider.log(`Failed to update cloud settings for task sync: ${error}`) | ||
| } | ||
| await updateGlobalState("taskSyncEnabled", message.bool ?? true) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error is logged but the local state is still updated even if the cloud update fails. Should we consider showing a user-facing error message or reverting the local state to maintain consistency between local and cloud settings? |
||
| await provider.postStateToWebview() | ||
| break | ||
| case "refreshAllMcpServers": { | ||
| const mcpHub = provider.getMcpHub() | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ type CloudViewProps = { | |
|
|
||
| export const CloudView = ({ userInfo, isAuthenticated, cloudApiUrl, onDone }: CloudViewProps) => { | ||
| const { t } = useAppTranslation() | ||
| const { remoteControlEnabled, setRemoteControlEnabled } = useExtensionState() | ||
| const { remoteControlEnabled, setRemoteControlEnabled, taskSyncEnabled, setTaskSyncEnabled } = useExtensionState() | ||
| const wasAuthenticatedRef = useRef(false) | ||
|
|
||
| const rooLogoUri = (window as any).IMAGES_BASE_URI + "/roo-logo.svg" | ||
|
|
@@ -75,6 +75,17 @@ export const CloudView = ({ userInfo, isAuthenticated, cloudApiUrl, onDone }: Cl | |
| vscode.postMessage({ type: "remoteControlEnabled", bool: newValue }) | ||
| } | ||
|
|
||
| const handleTaskSyncToggle = () => { | ||
| const newValue = !taskSyncEnabled | ||
| setTaskSyncEnabled(newValue) | ||
| vscode.postMessage({ type: "taskSyncEnabled", bool: newValue }) | ||
| // If disabling task sync, also disable remote control | ||
| if (!newValue && remoteControlEnabled) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When disabling Task Sync automatically disables Roomote Control, there's no visual feedback to inform the user that both settings were changed. Consider adding a brief toast notification or inline message to make this behavior more transparent. |
||
| setRemoteControlEnabled(false) | ||
| vscode.postMessage({ type: "remoteControlEnabled", bool: false }) | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <div className="flex flex-col h-full"> | ||
| <div className="flex justify-between items-center mb-6"> | ||
|
|
@@ -121,24 +132,56 @@ export const CloudView = ({ userInfo, isAuthenticated, cloudApiUrl, onDone }: Cl | |
| </div> | ||
| )} | ||
|
|
||
| {userInfo?.extensionBridgeEnabled && ( | ||
| <div className="border-t border-vscode-widget-border pt-4 mt-4"> | ||
| <div className="flex items-center gap-3 mb-2"> | ||
| <ToggleSwitch | ||
| checked={remoteControlEnabled} | ||
| onChange={handleRemoteControlToggle} | ||
| size="medium" | ||
| aria-label={t("cloud:remoteControl")} | ||
| data-testid="remote-control-toggle" | ||
| /> | ||
| <span className="font-medium text-vscode-foreground">{t("cloud:remoteControl")}</span> | ||
| </div> | ||
| <div className="text-vscode-descriptionForeground text-sm mt-1 mb-4 ml-8"> | ||
| {t("cloud:remoteControlDescription")} | ||
| </div> | ||
| <hr className="border-vscode-widget-border mb-4" /> | ||
| {/* Task Sync Toggle - Always shown when authenticated */} | ||
| <div className="border-t border-vscode-widget-border pt-4 mt-4"> | ||
| <div className="flex items-center gap-3 mb-2"> | ||
| <ToggleSwitch | ||
| checked={taskSyncEnabled} | ||
| onChange={handleTaskSyncToggle} | ||
| size="medium" | ||
| aria-label={t("cloud:taskSync")} | ||
| data-testid="task-sync-toggle" | ||
| /> | ||
| <span className="font-medium text-vscode-foreground">{t("cloud:taskSync")}</span> | ||
| </div> | ||
| )} | ||
| <div className="text-vscode-descriptionForeground text-sm mt-1 mb-4 ml-8"> | ||
| {t("cloud:taskSyncDescription")} | ||
| </div> | ||
|
|
||
| {/* Remote Control Toggle - Only shown when extensionBridgeEnabled is true */} | ||
| {userInfo?.extensionBridgeEnabled && ( | ||
| <> | ||
| <div className="flex items-center gap-3 mb-2"> | ||
| <ToggleSwitch | ||
| checked={remoteControlEnabled} | ||
| onChange={handleRemoteControlToggle} | ||
| size="medium" | ||
| aria-label={t("cloud:remoteControl")} | ||
| data-testid="remote-control-toggle" | ||
| disabled={!taskSyncEnabled} | ||
| /> | ||
| <span className="font-medium text-vscode-foreground"> | ||
| {t("cloud:remoteControl")} | ||
| </span> | ||
| </div> | ||
| <div className="text-vscode-descriptionForeground text-sm mt-1 mb-4 ml-8"> | ||
| {t("cloud:remoteControlDescription")} | ||
| {!taskSyncEnabled && ( | ||
| <div className="text-vscode-errorForeground mt-2"> | ||
| {t("cloud:remoteControlRequiresTaskSync")} | ||
| </div> | ||
| )} | ||
| </div> | ||
| </> | ||
| )} | ||
|
|
||
| {/* Info text about usage metrics */} | ||
| <div className="text-vscode-descriptionForeground text-sm mt-4 mb-4 ml-8 italic"> | ||
| {t("cloud:usageMetricsAlwaysReported")} | ||
| </div> | ||
|
|
||
| <hr className="border-vscode-widget-border mb-4" /> | ||
| </div> | ||
|
|
||
| <div className="flex flex-col gap-2 mt-4"> | ||
| <VSCodeButton appearance="secondary" onClick={handleVisitCloudWebsite} className="w-full"> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.