-
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
Conversation
- Add new taskSyncEnabled setting to control task content syncing - Keep remoteControlEnabled for Roomote Control functionality - Task Sync controls whether task content is sent to cloud - Roomote Control controls whether cloud can send instructions back - Roomote Control now depends on Task Sync being enabled - Usage metrics (tokens, cost) always reported regardless of settings - Update UI with two separate toggles and clear descriptions - Add info text explaining usage metrics are always reported
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.
Reviewed my own code. Found it surprisingly coherent for something I wrote.
| 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() |
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.
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 providerRef.deref() might return undefined between operations?
| } catch (error) { | ||
| provider.log(`Failed to update cloud settings for task sync: ${error}`) | ||
| } | ||
| await updateGlobalState("taskSyncEnabled", message.bool ?? true) |
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.
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?
| setTaskSyncEnabled(newValue) | ||
| vscode.postMessage({ type: "taskSyncEnabled", bool: newValue }) | ||
| // If disabling task sync, also disable remote control | ||
| if (!newValue && remoteControlEnabled) { |
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.
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.
|
@roomote-agent Implement the PR Feeback. |
|
I see the PR feedback and will implement the following fixes:
Working on these changes now... |
|
I have successfully implemented all the requested PR feedback: ✅ Completed Fixes
📋 Testing
|
97ca569 to
b6020ac
Compare
|
@roomote-agent add the missing translations |
|
Hi @brunobergher! I see your request to add the missing translations. I'm checking out the PR now to identify and add the missing translation strings. I'll update you once complete. |
…ings - Added taskSync, taskSyncDescription, remoteControlRequiresTaskSync, and usageMetricsAlwaysReported keys to all non-English cloud.json files - Updated cloudBenefit keys to match English structure - Ensured all languages have consistent translation keys for the new Task Sync and Roomote Control features
7ee0da1 to
74c0943
Compare
|
Hi @brunobergher! ✅ I've successfully added the missing translations for the Task Sync and Roomote Control settings. Changes made:
Files updated:All cloud.json files in the following locales:
The chat.json files already had the All tests are passing locally. The PR checks are currently running and should complete shortly. |
|
Closing this as stale. Reopen if needed |
This PR separates the Task Sync and Roomote Control (Remote Control) functionality into two independent settings, allowing users to have more granular control over their cloud integration.
Changes
New Settings Structure
taskSyncEnabled): Controls whether task content (messages, code, etc.) is sent to the cloudremoteControlEnabled): Controls whether the cloud can send instructions back to the extensionKey Implementation Details
taskSyncEnabledsetting to types and global settings (defaults totruefor backward compatibility)taskSyncEnabledbefore sending task messagesFiles Modified
packages/types/src/cloud.ts- Added taskSyncEnabled to user settings schemapackages/types/src/global-settings.ts- Added taskSyncEnabled with default valuesrc/core/task/Task.ts- Check taskSyncEnabled before capturing task messagessrc/core/webview/webviewMessageHandler.ts- Handle taskSyncEnabled messagesrc/shared/ExtensionMessage.ts- Include taskSyncEnabled in ExtensionStatesrc/shared/WebviewMessage.ts- Add taskSyncEnabled message typewebview-ui/src/components/cloud/CloudView.tsx- UI with two toggleswebview-ui/src/context/ExtensionStateContext.tsx- State management for taskSyncEnabledwebview-ui/src/i18n/locales/en/cloud.json- Translation stringsTesting
npm test)npm run lint)Screenshots
The UI now shows two separate toggles:
Backward Compatibility
taskSyncEnableddefaults totrueto maintain existing behaviorImportant
This PR separates Task Sync and Roomote Control settings, adding independent toggles and ensuring backward compatibility by defaulting Task Sync to enabled.
taskSyncEnabledandremoteControlEnabledsettings for granular control over cloud integration.remoteControlEnableddepends ontaskSyncEnabledbeing true.taskSyncEnabledtouserSettingsConfigSchemaincloud.tsandglobalSettingsSchemainglobal-settings.ts.Task.tsto checktaskSyncEnabledbefore capturing task messages.webviewMessageHandler.tsto handletaskSyncEnabledmessages.ExtensionStateandWebviewMessageto includetaskSyncEnabled.CloudView.tsxto add toggles for Task Sync and Roomote Control.npm test).This description was created by
for aa37e37. You can customize this summary. It will automatically update as commits are pushed.